Re: register_driver with 0000 mode

[Petro Karashchenko]

Hi,

If I understand correctly we can add one single "dummy_read" implementation
for all drivers that do not supply the read method. This will ensure that
O_RDOK is always supported if file permissions allow it. By doing so we can
relax check in inode_checkflags() to:

int inode_checkflags(FAR struct inode *inode, int oflags)
{
  if ((oflags == 0) ||
  (oflags & O_WROK) != 0 && !inode->u.i_ops->write)
{
  return -EACCES;
}
  else
{
  return OK;
}
}

Also the "0" oflags will not be allowed.

If yes, then I'm happy to submit the change. Otherwise please correct me if
I'm wrong.

Best regards,
Petro

сб, 2 квіт. 2022 р. о 08:06 Jukka Laitinen  пише:

> As Greg said, this is a traditional way. I'd maybe allow dummy
> reads/writes if permissions allow, but keep that logic in vfs by checking
> if the pointer is initialized. Then you don't need to provide the dummy
> implementation in every driver.
>
> Petro Karashchenko kirjoitti lauantai 2. huhtikuuta 2022:
> > Hello Gregory,
> >
> > Do you recommend not to allow registration of a driver that does not have
> > read method and make a requirement for the driver to provide at least
> dummy
> > read?
> >
> > Best regards,
> > Petro
> >
> > On Fri, Apr 1, 2022, 9:48 PM Gregory Nutt  wrote:
> >
> > >
> > > > One option, conforming standard, would be that you just always give
> > > O_RDWR (same flags as what linux devices have), but then when calling
> > > read/write you check if the pointer is non-null. If the driver doesn't
> > > define read or write, those operations are allowed on the device, but
> act
> > > as no-op.
> > >
> > > If you can't think of anything useful to do with read() or write(),
> > > thenthis has been historically handled is by including a dummy read
> > > method in the driver that just returns zero (EOF).  For example, the
> > > loop driver:
> > >
> > >
> > >
> /
> > >   * Name: loop_read
> > >
> > >
>  /
> > >
> > > static ssize_t loop_read(FAR struct file *filep, FAR char *buffer,
> > >   size_t len)
> > > {
> > >return 0; /* Return EOF */
> > > }
> > > *
> > > *
> > >
> >

Re: register_driver with 0000 mode

[Gregory Nutt]

If I understand correctly we can add one single "dummy_read" implementation
> for all drivers that do not supply the read method. This will ensure that
> O_RDOK is always supported if file permissions allow it.
>

There aren't any file permissions.  The entire access is controlled by NULL
and non-NULL read/write methods.

That is the way that it has worked.  if you check, I think you will find
that all of the existing IOCTL drivers already have dummy read and
sometimes write methods.  Being consistent is important to the integrity of
the OS.  Whatever you all decide to do, make sure that is is consistent
across all drivers.

The semantics are:  NULL read -> write only, NULL write -> read only.  Both
NULL -> the driver is broken.

I don't understand how checking if the pointer is NULL would support
read-only, or write-only behavior.  I think we would be losing
functionality and this is important in other drivers where read and write
actually matter.

Re: register_driver with 0000 mode

[Xiang Xiao]

Many functionality is accessed through ioctl callback, what permission is
needed before invoking ioctl?

On Sat, Apr 2, 2022 at 8:45 PM Gregory Nutt  wrote:

> If I understand correctly we can add one single "dummy_read" implementation
> > for all drivers that do not supply the read method. This will ensure that
> > O_RDOK is always supported if file permissions allow it.
> >
>
> There aren't any file permissions.  The entire access is controlled by NULL
> and non-NULL read/write methods.
>
> That is the way that it has worked.  if you check, I think you will find
> that all of the existing IOCTL drivers already have dummy read and
> sometimes write methods.  Being consistent is important to the integrity of
> the OS.  Whatever you all decide to do, make sure that is is consistent
> across all drivers.
>
> The semantics are:  NULL read -> write only, NULL write -> read only.  Both
> NULL -> the driver is broken.
>
> I don't understand how checking if the pointer is NULL would support
> read-only, or write-only behavior.  I think we would be losing
> functionality and this is important in other drivers where read and write
> actually matter.
>

Re: register_driver with 0000 mode

[Gregory Nutt]

> By doing so we can relax check in inode_checkflags() to:
>
> int inode_checkflags(FAR struct inode *inode, int oflags)
> {
>   if ((oflags == 0) ||
>   (oflags & O_WROK) != 0 && !inode->u.i_ops->write)
> {
>   return -EACCES;
> }
>   else
> {
>   return OK;
> }
> }
>
> No, this would be an error.  This would break any file, filesystem, or
driver that really needs to support write-only behavior. In that case,
there is a write method and no read method.

 This would be a major loss of functionality.  These IOCTL-only drivers are
degenerate cases and perhaps for them you can cut corners.  Bu, we need to
support all existing behavior for all VFS entities.

To permit opening with O_RDWR, some IOCTL drivers have dummy write methods
too.  The loop driver again has:

/
 * Name: loop_write
 /
static ssize_t loop_write(FAR struct file *filep, FAR const char *buffer,
  size_t len)
{
  return len; /* Say that everything was written */
}

Re: register_driver with 0000 mode

[Petro Karashchenko]

Hi,

Here are a few examples of "broken" drivers:
 - opamp_fops
 - lcddev_fops
 - g_pca9635pw_fileops
 - g_foc_fops
 - notectl_fops
 - powerled_fops
 - g_rptun_devops
 - g_video_fops

The list is not complete. But since the "register_driver()" does not return
an error if both fops read and write pointers are NULL we are still in the
middle of discussion.

I just want to understand the algorithm to get it fixed. So let me
summarise again and if nobody has any objections I will implement the
change:
1. Zero "oflags" should be considered as illegal  and "open" call should
return "-EACCES" in this case.
2. The "register_driver()" should check if both "fops" read and write
pointers are NULL and return an error if such a situation is detected.
3. Driver register should check if "mode" value is consistent with provided
"fops" and if "read" method is NULL but "mode" contains "r" the error
should be returned. The same for write and "w" permission.
4. Update all the drivers that have both read and write methods as NULL and
add "dummy_read()" or "dummy_write" handler. Optionally in order to save
space the "fops_dummy_read" and "fops_dummy_write" can be added into
"include/nuttx/fs/fs.h" so drivers will not need to duplicate the code and
can reference a common implementation.

Please reply with your thoughts.

Best regards,
Petro

сб, 2 квіт. 2022 р. о 15:08 Gregory Nutt  пише:

> > By doing so we can relax check in inode_checkflags() to:
> >
> > int inode_checkflags(FAR struct inode *inode, int oflags)
> > {
> >   if ((oflags == 0) ||
> >   (oflags & O_WROK) != 0 && !inode->u.i_ops->write)
> > {
> >   return -EACCES;
> > }
> >   else
> > {
> >   return OK;
> > }
> > }
> >
> > No, this would be an error.  This would break any file, filesystem, or
> driver that really needs to support write-only behavior. In that case,
> there is a write method and no read method.
>
>  This would be a major loss of functionality.  These IOCTL-only drivers are
> degenerate cases and perhaps for them you can cut corners.  Bu, we need to
> support all existing behavior for all VFS entities.
>
> To permit opening with O_RDWR, some IOCTL drivers have dummy write methods
> too.  The loop driver again has:
>
>
> /
>  * Name: loop_write
>
>  /
> static ssize_t loop_write(FAR struct file *filep, FAR const char *buffer,
>   size_t len)
> {
>   return len; /* Say that everything was written */
> }
>

Re: register_driver with 0000 mode

[Gregory Nutt]

Here are a few examples of "broken" drivers:
  - opamp_fops
  - lcddev_fops
  - g_pca9635pw_fileops
  - g_foc_fops
  - notectl_fops
  - powerled_fops
  - g_rptun_devops
  - g_video_fops

The list is not complete. But since the "register_driver()" does not return
an error if both fops read and write pointers are NULL we are still in the
middle of discussion.

I just want to understand the algorithm to get it fixed. So let me
summarise again and if nobody has any objections I will implement the
change:
1. Zero "oflags" should be considered as illegal  and "open" call should
return "-EACCES" in this case.


Yes, POSIX requires this: 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html "


   /"... Applications shall specify exactly one of the first five
   values (file access modes) below in the value of //oflag//:/

   /O_EXEC/
   /Open for execute only (non-directory files). The result is
   unspecified if this flag is applied to a directory./
   /O_RDONLY/
   /Open for reading only./
   /O_RDWR/
   /Open for reading and writing. The result is undefined if this
   flag is applied to a FIFO./
   /O_SEARCH/
   /Open directory for search only. The result is unspecified if
   this flag is applied to a non-directory file./
   /O_WRONLY/
   /Open for writing only."/

(Assuming that the above are all non-zero, of course).  O_EXEC and 
O_SEARCH are required by the current POSIX spec but is not implemented 
in NuttX.



2. The "register_driver()" should check if both "fops" read and write
pointers are NULL and return an error if such a situation is detected.
I don't believe this is necessary in general.  Perhaps only if 
DEBUG_FEATURES is enabled?  Or perhaps a DEBUG assertion?

3. Driver register should check if "mode" value is consistent with provided
"fops" and if "read" method is NULL but "mode" contains "r" the error
should be returned. The same for write and "w" permission.


An alternative, more POSIX-like implementation would flesh out the mode 
logic.  fs_registerdriver.s, for example, has a mode argument that is 
retained in the inode.  The mode could be used to determine if VFS 
entity is read-able or write-able.


That would be a lot of work and most file systems do not support 
permissions but would take us a long way toward a Unix-like security 
system with proper permissions.



4. Update all the drivers that have both read and write methods as NULL and
add "dummy_read()" or "dummy_write" handler. Optionally in order to save
space the "fops_dummy_read" and "fops_dummy_write" can be added into
"include/nuttx/fs/fs.h" so drivers will not need to duplicate the code and
can reference a common implementation.


NOTE that the dummy read method returns EOF, but the dummy write returns 
the write size.  write never returns zero unless nbyte is zero: 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html


Greg

Re: register_driver with 0000 mode

[Gregory Nutt]

On 4/2/2022 7:03 AM, Xiang Xiao wrote:

Many functionality is accessed through ioctl callback, what permission is
needed before invoking ioctl?


By permissions, you mean open mode?  Per POSIX, O_WRONLY, O_RDONLY, or 
O_RDWR is required on any success open call (or O_EXEC or O_SEARCH).


But I did run across this non-standard Linux behavior in this man page: 
https://man7.org/linux/man-pages/man2/open.2.html:


   Under Linux, the*O_NONBLOCK *flag is sometimes used in cases where
   one wants to open but does not necessarily have the intention to
   read or write.  For example, this may be used to open a device in
   order to get a file descriptor for use withioctl(2)  
.

Re: register_driver with 0000 mode

[Xiang Xiao]

Yes, for example if I want to set the serial device baud rate(TCSETS),
should I open it with O_WRONLY?

On Sat, Apr 2, 2022 at 10:56 PM Gregory Nutt  wrote:

> On 4/2/2022 7:03 AM, Xiang Xiao wrote:
> > Many functionality is accessed through ioctl callback, what permission is
> > needed before invoking ioctl?
>
> By permissions, you mean open mode?  Per POSIX, O_WRONLY, O_RDONLY, or
> O_RDWR is required on any success open call (or O_EXEC or O_SEARCH).
>
> But I did run across this non-standard Linux behavior in this man page:
> https://man7.org/linux/man-pages/man2/open.2.html:
>
> Under Linux, the*O_NONBLOCK *flag is sometimes used in cases where
> one wants to open but does not necessarily have the intention to
> read or write.  For example, this may be used to open a device in
> order to get a file descriptor for use withioctl(2)  <
> https://man7.org/linux/man-pages/man2/ioctl.2.html>.
>
>
>

Re: register_driver with 0000 mode

[Gregory Nutt]

Yes, for example if I want to set the serial device baud rate(TCSETS),
should I open it with O_WRONLY?


Linux has the concept of read IOCTLs and write IOCTLs as determined by 
the IOCTL macros.  My understanding is that you much provide O_RDONLY or 
O_RDWR to use the read IOCTLs and O_WRONLY or O_RDWR to use the write 
IOCTLs.


But I am not sure of that and there are a lot of things I don't fully 
understand.  I have seen references to some mysterious Bit 3 in the open 
flags that can be used open IOCTL-only drivers.  Like 
https://github.com/NoHomey/open-ioctl:


   "Opens device file in non-blocking ioctl mode. File is opend with
   flags 3 | O_NONBLOCK.

   "Flag 3 means that only ioctl calls can be made for comunication
   with the device driver (remember read/write operations are expensive
   this is why open-ioctl was made in first place to make it easer for
   performance and command oriented device drivers)."

My gut feeling is that these things are too non-standard and poorly 
thought out to be useful.

Re: register_driver with 0000 mode

[Petro Karashchenko]

For what I see in Linux headers
#define O_ACCMODE 0003
#define O_RDONLY 
#define O_WRONLY 0001
 #define O_RDWR 0002

So 3 seems to be a reserved value and because  O_RDONLY in Linux is defined
to 0 the 3 value from Linux is similar to 0 value in NuttX.

Regarding "Yes, for example if I want to set the serial device baud
rate(TCSETS), should I open it with O_WRONLY?" -- I do not have a complete
answer right now due to my competence in Linux is not strong, But what I
can find in the code that _IOC_WRITE and _IOC_READ bits are not checked
against the opened file mode, but specify the direction of data transfer
during ioctl call itself if the pointer is passed as an arg so in
combination of "#define _IOC_TYPECHECK(t) (sizeof(t))" and "_IOC_SIZESHIFT"
the callee know how many bytes need to be copied. Moreover there is a
comment before _IOC_READ and _IOC_WRITE definition:

/* * Direction bits, which any architecture can choose to override *
before including this file. */

So Linux FS layer can't rely on those definitions to do a f_mode checking
and some drivers that I inspected also didn't check the R/W mode of the
file. I most probably will not have time this week to prove it, but maybe
Mr. Xiang Xiao you can find some good Linux experts at Xiaomi and clarify
this question faster than me.

My personal opinion is the following:
1. The POSIX standard clearly defines that one of the five flags should be
passed (also fopen("test.txt", ""); will return NULL) so that should be
fixed and 0 should be invalid value for oflags.
2. "register_driver()" (fs_registerdriver) interface should sanitize and
ensure that at least one of read or write is non-NULL and assert otherwise.
This should be done to ensure that "inode_checkflags()" passes.
3. "register_driver()" (fs_registerdriver) interface should sanitize and
ensure that inode "mode" parameter does not contradict with the presence of
read and write methods otherwise it should assert. In other words if "mode"
contains "r" bits set while read method is NULL or "mode" contains "w" bits
while write is NULL should lead to an assert.
4. The VFS layer should provide the "dummy" read and write handlers that
can be reused by drivers. Dummy read should always return EOF while dummy
write should return that all data have been written.

Best regards,
Petro

сб, 2 квіт. 2022 р. о 17:17 Gregory Nutt  пише:

>
> > Yes, for example if I want to set the serial device baud rate(TCSETS),
> > should I open it with O_WRONLY?
>
> Linux has the concept of read IOCTLs and write IOCTLs as determined by
> the IOCTL macros.  My understanding is that you much provide O_RDONLY or
> O_RDWR to use the read IOCTLs and O_WRONLY or O_RDWR to use the write
> IOCTLs.
>
> But I am not sure of that and there are a lot of things I don't fully
> understand.  I have seen references to some mysterious Bit 3 in the open
> flags that can be used open IOCTL-only drivers.  Like
> https://github.com/NoHomey/open-ioctl:
>
> "Opens device file in non-blocking ioctl mode. File is opend with
> flags 3 | O_NONBLOCK.
>
> "Flag 3 means that only ioctl calls can be made for comunication
> with the device driver (remember read/write operations are expensive
> this is why open-ioctl was made in first place to make it easer for
> performance and command oriented device drivers)."
>
> My gut feeling is that these things are too non-standard and poorly
> thought out to be useful.
>
>

Re: [EXT] Re: TPL support in nuttx for daisy chain battery cell monitoring?

[Sathish Touch energy]

Hello Cis van Mierlo,
THe BCC driver software package as the link below has TPL communication
based on DMA mode, whereas the Nuttx SPI drivers are in non DMA mode.
We tried extending this to TPL mode in Nuttx in Non DMA mode. But currently
we are facing some synchronization issues, want to get your comment on
whether TPL mode driver can be built for Non DMA mode.

OR DMA mode is the only option  for TPL mode as given in the reference
driver below.

In Non DMA mode make sure that receive SPI is setup & ready to receive the
echo frame & the receive response before initiating the Transmit. What we
are noticing is that on the Receive though we are able to receive the echo
frame but its not synchronized and also some data is missing. Please advise.


Regards,
Sathish

On Wed, 1 Sept 2021 at 13:32, Cis van Mierlo  wrote:

> Hi Sathish,
>
>
>
> We currently do not use TPL in NuttX for our example application.
> But this can easily be adopted.
>
> Bare metal example code and drivers for the BCC (MC3377xx), including SPI
> transfers for TPL communication can be found on the NXP website.
>
> https://www.nxp.com/design/analog-expert-software-and-tools/sdk-analog-expert-drivers/embedded-sw-battery-cell-controller-software-driver-for-mc33771b-mc33772b:EMBEDDED-SW-BCC?tab=Design_Tools_Tab
>
> You could add this to your application and together with the NuttX SPI
> drivers.
>
> For an example on how to do that, you could check the BMS772 example
> software at https://github.com/NXPHoverGames/RDDRONE-BMS772/.
>
> This example software communicates using an S32K144 to a single MC33772
> BCC using SPI.
>
>
>
> Kind regards,
>
>
>
> Cis van Mierlo
> Systems & Applications Engineer
>
> NXP Semiconductors, CTO Systems Innovations
>
>
> High Tech Campus 46, room 0.64, 5656 AE Eindhoven, The Netherlands
> E-mail: cis.van.mie...@nxp.com
> Internet: http://www.nxp.com
>
>
>
> *From:* Sathish Touch energy 
> *Sent:* Saturday, August 28, 2021 4:09 AM
> *To:* dev@nuttx.apache.org
> *Cc:* Peter van der Perk ; Cis van Mierlo <
> cis.van.mie...@nxp.com>
> *Subject:* [EXT] Re: TPL support in nuttx for daisy chain battery cell
> monitoring?
>
>
>
> *Caution: *EXT Email
>
> Hi Alan,
>
>
>
> Thanks for your quick response.
>
>
>
> Yes. It requires TWO SPI WORKING at the same time.
>
>
>
> Hi Peter and Cis,
>
>
>
> Do we have TPL code in nuttx for daisy chain battery cell monitoring?
>
>
>
> or how do we proceed into the nuttx porting work from the TPL code
> available at
>
>
> https://community.nxp.com/t5/NXP-Model-Based-Design-Tools/BMS-amp-MBDT-MC33771B-MC33772B-TPL-communication-with-S32K/ta-p/1153459
> 
>
>
>
> Thanks and Regards,
> Sathish.
>
>
>
>
>
> On Sat, 28 Aug 2021 at 01:10, Alan Carvalho de Assis 
> wrote:
>
> Hi Sathish,
>
> I think it will need the driver for MC3377x or similar battery monitor
> chip:
>
>
> https://community.nxp.com/t5/NXP-Model-Based-Design-Tools/BMS-amp-MBDT-MC33771B-MC33772B-TPL-communication-with-S32K/ta-p/1153459
> 
>
> If I read it correctly it will require two SPIs working at same time
> to support TPL.
>
> I'm CC Peter and Cis, they are the guys working on it and could give
> us more info.
>
> BR,
>
> Alan
>
> On 8/27/21, Sathish Touch energy  wrote:
> > Hi,
> >
> > I am using nuttx, with an S32K144 eval board.
> >
> > nuttx supports SPI driver for battery cell monitoring.
> >
> > In the same way, is there TPL support in nuttx for daisy chain battery
> cell
> > monitoring?
> >
> > Thanks and Regards,
> > Sathish.
> >
>
>