Re: [PATCH] gpiolib: cdev: fix stream_open.cocci warnings

[Kent Gibson]

On Thu, May 05, 2022 at 08:24:50PM +0800, Kent Gibson wrote:
> On Thu, May 05, 2022 at 02:18:48PM +0200, Bartosz Golaszewski wrote:
> > On Thu, May 5, 2022 at 1:50 PM Yihao Han <hanyihao@xxxxxxxx> wrote:
> > >
> > > ./drivers/gpio/gpiolib-cdev.c:2498:7-23: WARNING:
> > > gpio_fileops: .read() has stream semantic;
> > > safe to change nonseekable_open -> stream_open.
> > >
> > > Generated by: scripts/coccinelle/api/stream_open.cocci
> > >
> > > Signed-off-by: Yihao Han <hanyihao@xxxxxxxx>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index f5aa5f93342a..d03c8e012c8c 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -2495,7 +2495,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
> > >         get_device(&gdev->dev);
> > >         file->private_data = cdev;
> > >
> > > -       ret = nonseekable_open(inode, file);
> > > +       ret = stream_open(inode, file);
> > >         if (ret)
> > >                 goto out_unregister_notifier;
> > >
> > > --
> > > 2.17.1
> > >
> > 
> > Cc'ing Kent.
> > 
> > This patch doesn't seem to target current master or rc1.
> > 
> > It also can't be right - we specifically mark all filesystem objects
> > exposed by the GPIO character device as non-seekable.
> > 
> 
> Agreed.  I took the coccinelle warning as being a false positive.
> If I recall correctly I had a quick look to see if it could be disabled,
> but didn't find anything, and so just ignored it.
> 

Further to this...

The coccinelle warning says:

// Search for stream-like files that are using nonseekable_open and convert
// them to stream_open. A stream-like file is a file that does not use ppos in
// its read and write. Rationale for the conversion is to avoid deadlock in
// between read and write.

The rationale is not applicable here, as all cdev files are non-writeable,
as well as being non-seekable.
But switching to stream_open(), as coccinelle suggests, is probably fine.

The doco for stream_open() says:

 * stream_open is used by subsystems that want stream-like file descriptors.
 * Such file descriptors are not seekable and don't have notion of position
 * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL).
 * Contrary to file descriptors of other regular files, .read() and .write()
 * can run simultaneously.

so it would also produce nonseekable files.
Comparing nonseekable_open() with stream_open(), the latter clears
FMODE_ATOMIC_POS and sets FMODE_STREAM, neither of which are relevant for
the cdev use cases, so that should be ok.
So I would be ok with the patch if it were updated to apply to the
current gpiolib-cdev.c.

Cheers,
Kent.