Message-ID: <CAO9qdTH_+syAOBXUYT61VZMovDJYjCD1b-Who16Aqj4BXq8GLA@mail.gmail.com>
Date: Tue, 17 Sep 2024 19:01:35 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, colin.i.king@...il.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

Oliver Neukum <oneukum@...e.com> wrote:
>
> On 17.09.24 08:23, Jeongjun Park wrote:
> > Oliver Neukum <oneukum@...e.com> wrote:
>
> > Okay. But O_NONBLOCK flag check already exists, and I don't know
> > if we need to branch separately to mutex_trylock just because O_NONBLOCK
> > flag exists. I think mutex_lock_interruptible is enough.
>
> It will still block.
>
> > And the point of locking is too late. I think it would be more appropriate to
> > read file->private_data and then lock it right away.
>
> You are right. dev->present should be checked under the lock only.
>
> > I think this patch is a more appropriate patch:
> >
> > ---
> >   drivers/usb/misc/iowarrior.c | 41 +++++++++++++++++++++++++++---------
> >   1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> > index 6d28467ce352..6fb4ecebbc15 100644
> > --- a/drivers/usb/misc/iowarrior.c
> > +++ b/drivers/usb/misc/iowarrior.c
> > @@ -277,28 +277,40 @@ static ssize_t iowarrior_read(struct file *file,
> > char __user *buffer,
> >      struct iowarrior *dev;
> >      int read_idx;
> >      int offset;
> > +   int retval = 0;
> >
> >      dev = file->private_data;
> >
> > +   if (mutex_lock_interruptible(&dev->mutex)) {
>
> This blocks. To quote the man page:
>
>         O_NONBLOCK or O_NDELAY
>                When  possible,  the file is opened in nonblocking mode.
>                 Neither the open() nor any subsequent I/O operations on the file descriptor which is
>                returned will cause the calling process to wait.
>
>

Okay, I understand. Then I think it would be appropriate to do
the patch below to prevent blocking, but I have one question.

Currently, many misc usb drivers do not seem to handle the
O_NONBLOCK flag when using mutex_lock. If this is really
necessary code, I think it would require code modifications to
other functions inside iowarrior and many misc usb drivers.

What do you think about this?

Regards,
Jeongjun Park

---
drivers/usb/misc/iowarrior.c | 46 ++++++++++++++++++++++++++++--------
1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 6d28467ce352..dbf0ed04f7c3 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -277,28 +277,45 @@ static ssize_t iowarrior_read(struct file *file,
char __user *buffer,
struct iowarrior *dev;
int read_idx;
int offset;
+ int retval = 0;
dev = file->private_data;
+ if (file->f_flags & O_NONBLOCK) {
+ retval = mutex_trylock(&dev->mutex);
+ if (!retval)
+ return -EAGAIN;
+ } else {
+ retval = mutex_lock_interruptible(&dev->mutex);
+ if (retval)
+ return -ERESTARTSYS;
+ }
+
/* verify that the device wasn't unplugged */
- if (!dev || !dev->present)
- return -ENODEV;
+ if (!dev->present) {
+ retval = -ENODEV;
+ goto exit;
+ }
dev_dbg(&dev->interface->dev, "minor %d, count = %zd\n",
dev->minor, count);
/* read count must be packet size (+ time stamp) */
if ((count != dev->report_size)
- && (count != (dev->report_size + 1)))
- return -EINVAL;
+ && (count != (dev->report_size + 1))) {
+ retval = -EINVAL;
+ goto exit;
+ }
/* repeat until no buffer overrun in callback handler occur */
do {
atomic_set(&dev->overflow_flag, 0);
if ((read_idx = read_index(dev)) == -1) {
/* queue empty */
- if (file->f_flags & O_NONBLOCK)
- return -EAGAIN;
+ if (file->f_flags & O_NONBLOCK) {
+ retval = -EAGAIN;
+ goto exit;
+ }
else {
//next line will return when there is either new data, or the device
is unplugged
int r = wait_event_interruptible(dev->read_wait,
@@ -309,28 +326,37 @@ static ssize_t iowarrior_read(struct file *file,
char __user *buffer,
-1));
if (r) {
//we were interrupted by a signal
- return -ERESTART;
+ retval = -ERESTART;
+ goto exit;
}
if (!dev->present) {
//The device was unplugged
- return -ENODEV;
+ retval = -ENODEV;
+ goto exit;
}
if (read_idx == -1) {
// Can this happen ???
- return 0;
+ retval = 0;
+ goto exit;
}
}
}
offset = read_idx * (dev->report_size + 1);
if (copy_to_user(buffer, dev->read_queue + offset, count)) {
- return -EFAULT;
+ retval = -EFAULT;
+ goto exit;
}
} while (atomic_read(&dev->overflow_flag));
read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx;
atomic_set(&dev->read_idx, read_idx);
+ mutex_unlock(&dev->mutex);
return count;
+
+exit:
+ mutex_unlock(&dev->mutex);
+ return retval;
}
/*
--

>
> [..]
> > +unlock_exit:
> > +   mutex_unlock(&dev->mutex);
> > +exit:
> > +   return retval;
>
> The rest looks good to me.
>
>         Regards
>                 Oliver
>

Powered by blists - more mailing lists