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

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

[Yihao Han]

./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@vivo.com>
---
 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

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

[Bartosz Golaszewski]

On Thu, May 5, 2022 at 1:50 PM Yihao Han <hanyihao@vivo.com> 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@vivo.com>
> ---
>  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.

Bart

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

[Kent Gibson]

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@vivo.com> 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@vivo.com>
> > ---
> >  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.

Cheers,
Kent.

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@vivo.com> 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@vivo.com>
> > > ---
> > >  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.