Re: [PATCH] USB: usblp: Add device status detection in usblp_poll()

[Pete Zaitcev <zaitcev@xxxxxxxxxx>]

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;
+	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;
 }