Re: [PATCH 3/3] gpiolib: cdev: Cleanup kfifo_out() error handling

[Kent Gibson <warthog618@gmail.com>]

On Wed, May 29, 2024 at 01:24:45PM +0200, Bartosz Golaszewski wrote:
> On Mon, May 27, 2024 at 1:55 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The handling of kfifo_out() errors in read functions obscures any error.
> > The error condition should never occur but, while a ret is set to -EIO, it
> > is subsequently ignored and the read functions instead return the number
> > of bytes copied to that point, potentially masking the fact that any error
> > occurred.
> >
> > Return -EIO in the case of a kfifo_out() error to make it clear something
> > very odd is going on here.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 47 +++++++++++++++++--------------------
> >  1 file changed, 21 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index c7218c9f2c5e..6a986d7f1f2f 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -1642,16 +1642,13 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
> >                                         return ret;
> >                         }
> >
> > -                       ret = kfifo_out(&lr->events, &le, 1);
> > -               }
> > -               if (ret != 1) {
> > -                       /*
> > -                        * This should never happen - we were holding the
> > -                        * lock from the moment we learned the fifo is no
> > -                        * longer empty until now.
> > -                        */
> > -                       ret = -EIO;
> > -                       break;
> > +                       if (kfifo_out(&lr->events, &le, 1) != 1)
> > +                               /*
> > +                                * This should never happen - we hold the
>
> I'm not a native speaker but this looks odd to me - shouldn't it be
> "we held the lock from the moment..."?
>

Unlike the original, it is within the scoped_guard here, and we still hold the
lock, so using the past tense would be incorrect.

> > +                                * lock from the moment we learned the fifo
> > +                                * is no longer empty until now.
> > +                                */
> > +                               return -EIO;
>
> Since this is so unlikely maybe a WARN() would be justified here too?
>

Yeah, that makes sense.  I'll add them for v2.

Cheers,
Kent.