Re: circular submissions in cdc-wdm and how to break them on disconnect

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

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.