On 2021/01/22 0:30, Oliver Neukum wrote:
> Hi,
>
> you have moved kill_urbs() below
>
> cancel_work_sync(&desc->rxwork);
> cancel_work_sync(&desc->service_outs_intr);
>
> to close a race, as
>
> rv = usb_submit_urb(desc->response, GFP_KERNEL);
>
> in service_outstanding_interrupt() would submit the response URB,
> right?
Right. Shouldn't remaining
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
cancel_work_sync(&desc->service_outs_intr);
sequence in wdm_suspend() and wdm_pre_reset() be updated as well?
> Unfortunately we have in wdm_in_callback() the following code path
>
> if (desc->rerr) {
> /*
> * Since there was an error, userspace may decide to not read
> * any data after poll'ing.
> * We should respond to further attempts from the device to send
> * data, so that we can get unstuck.
> */
> schedule_work(&desc->service_outs_intr);
>
> It looks to me like we have a circular dependency here and this needs some
> change to break. What do you think about the attached patch?
I don't know how poisoning works. But why can't we simply use test_bit() on
WDM_SUSPENDING/WDM_RESETTING/WDM_DISCONNECTING flags, for schedule_work() in
wdm_in_callback() is called with desc->iuspin (which serializes setting of
these flags) held.
By the way, since someone might interpret "broken" as "out of order / not working",
I expect not using "This needs to be broken." in the commit message. There would be
some better idiom.
