| Message ID | 20240527115419.92606-4-warthog618@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] gpiolib: cdev: Add INIT_KFIFO() for linereq events | expand |
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..."? > + * 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? Bart > } > > if (copy_to_user(buf + bytes_read, &le, sizeof(le))) > @@ -1995,16 +1992,13 @@ static ssize_t lineevent_read(struct file *file, char __user *buf, > return ret; > } > > - ret = kfifo_out(&le->events, &ge, 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(&le->events, &ge, 1) != 1) > + /* > + * This should never happen - we hold the > + * lock from the moment we learned the fifo > + * is no longer empty until now. > + */ > + return -EIO; > } > > if (copy_to_user(buf + bytes_read, &ge, ge_size)) > @@ -2707,12 +2701,13 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, > if (count < event_size) > return -EINVAL; > #endif > - ret = kfifo_out(&cdev->events, &event, 1); > - } > - if (ret != 1) { > - ret = -EIO; > - break; > - /* We should never get here. See lineevent_read(). */ > + if (kfifo_out(&cdev->events, &event, 1) != 1) > + /* > + * This should never happen - we hold the > + * lock from the moment we learned the fifo > + * is no longer empty until now. > + */ > + return -EIO; > } > > #ifdef CONFIG_GPIO_CDEV_V1 > -- > 2.39.2 >
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.
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 + * lock from the moment we learned the fifo + * is no longer empty until now. + */ + return -EIO; } if (copy_to_user(buf + bytes_read, &le, sizeof(le))) @@ -1995,16 +1992,13 @@ static ssize_t lineevent_read(struct file *file, char __user *buf, return ret; } - ret = kfifo_out(&le->events, &ge, 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(&le->events, &ge, 1) != 1) + /* + * This should never happen - we hold the + * lock from the moment we learned the fifo + * is no longer empty until now. + */ + return -EIO; } if (copy_to_user(buf + bytes_read, &ge, ge_size)) @@ -2707,12 +2701,13 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, if (count < event_size) return -EINVAL; #endif - ret = kfifo_out(&cdev->events, &event, 1); - } - if (ret != 1) { - ret = -EIO; - break; - /* We should never get here. See lineevent_read(). */ + if (kfifo_out(&cdev->events, &event, 1) != 1) + /* + * This should never happen - we hold the + * lock from the moment we learned the fifo + * is no longer empty until now. + */ + return -EIO; } #ifdef CONFIG_GPIO_CDEV_V1
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(-)