Oliver Neukum <oneukum@xxxxxxxx> writes:
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 1656f5155ab8..a341081a5f47 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> {
> struct wdm_device *desc = file->private_data;
>
> - wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> + wait_event(desc->wait,
> + /*
> + * needs both flags. We cannot do with one
> + * because resetting it would cause a race
> + * with write() yet we need to signal
> + * a disconnect
> + */
> + !test_bit(WDM_IN_USE, &desc->flags) &&
> + !test_bit(WDM_DISCONNECTING, &desc->flags));
Makes sense. But isn't the WDM_DISCONNECTING test inverted?
> /* cannot dereference desc->intf if WDM_DISCONNECTING */
> if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> desc->werr);
>
> - return usb_translate_errors(desc->werr);
> + return test_bit(WDM_DISCONNECTING, &desc->flags) ?
> + -ENODEV :
> + usb_translate_errors(desc->werr);
> }
Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
here. How about
if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
if (desc->werr < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
return usb_translate_errors(desc->werr);
> static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
> @@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
> spin_lock_irqsave(&desc->iuspin, flags);
> set_bit(WDM_DISCONNECTING, &desc->flags);
> set_bit(WDM_READ, &desc->flags);
> - /* to terminate pending flushes */
> - clear_bit(WDM_IN_USE, &desc->flags);
> spin_unlock_irqrestore(&desc->iuspin, flags);
> wake_up_all(&desc->wait);
> mutex_lock(&desc->rlock);
Yes, this looks much better.
Bjørn
