On Tue, Mar 02, 2021 at 11:12:54PM -0600, Pete Zaitcev wrote:
> On Tue, 2 Mar 2021 07:41:07 +0000
> "Zhang, Qiang" <Qiang.Zhang@xxxxxxxxxxxxx> wrote:
>
> > and also I find similar usage in usb/class/usbtmc.c
>
> Seems like a bug indeed, but I don't like the example in usbtmc.c.
> Please let me know if the following is acceptable:
>
> commit 83591ac63bc666a44f250b43af6c0f5a1e001841
> Author: Pete Zaitcev <zaitcev@xxxxxxxxxxxxxxxxx>
> Date: Tue Mar 2 23:00:28 2021 -0600
>
> usblp: fix a hang in poll() if disconnected
>
> Apparently an application that opens a device and calls select()
> on it, will hang if the decice is disconnected. It's a little
> surprising that we had this bug for 15 years, but apparently
> nobody ever uses select() with a printer: only write() and read(),
> and those work fine. Well, you can also select() with a timeout.
>
> The fix is modeled after devio.c. A few drivers check the
> condition first, then do not add the wait queue in case the
> device is disconnected. We doubt that's completely race-free.
> So, this patch adds the process first, then locks properly
> and checks for the disconnect.
>
> Reported-by: Zqiang <qiang.zhang@xxxxxxxxxxxxx>
> Signed-off-by: Pete Zaitcev <zaitcev@xxxxxxxxxx>
>
> diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
> index fd87405adbed..5733a0067f5b 100644
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -494,16 +494,24 @@ static int usblp_release(struct inode *inode, struct file *file)
> /* No kernel lock - fine */
> static __poll_t usblp_poll(struct file *file, struct poll_table_struct *wait)
> {
> - __poll_t ret;
> + struct usblp *usblp = file->private_data;
> + __poll_t ret = 0;
> unsigned long flags;
>
> - struct usblp *usblp = file->private_data;
> /* Should we check file->f_mode & FMODE_WRITE before poll_wait()? */
> poll_wait(file, &usblp->rwait, wait);
> poll_wait(file, &usblp->wwait, wait);
> +
> + mutex_lock(&usblp->mut);
> + if (!usblp->present)
> + ret != EPOLLHUP;
Typo: ! instead of |. You have to look closely to see the difference.
alan Stern
> + mutex_unlock(&usblp->mut);
> +
> spin_lock_irqsave(&usblp->lock, flags);
> - ret = ((usblp->bidir && usblp->rcomplete) ? EPOLLIN | EPOLLRDNORM : 0) |
> - ((usblp->no_paper || usblp->wcomplete) ? EPOLLOUT | EPOLLWRNORM : 0);
> + if (usblp->bidir && usblp->rcomplete)
> + ret |= EPOLLIN | EPOLLRDNORM;
> + if (usblp->no_paper || usblp->wcomplete)
> + ret |= EPOLLOUT | EPOLLWRNORM;
> spin_unlock_irqrestore(&usblp->lock, flags);
> return ret;
> }
>
