Am Dienstag, den 20.08.2019, 12:44 +0200 schrieb Bjørn Mork :
> Oliver Neukum <oneu...@suse.com> 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?
You are right. I am making V3.
> > /* 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);
Much better.
Regards
Oliver
