Re: [PATCH] usb: core: Prevent infinite loops when usb_reset_device() unbinds/binds

[Doug Anderson]

Hi,

On Fri, Oct 20, 2023 at 9:23 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 20, 2023 at 08:59:42AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 20, 2023 at 8:46 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote:
> > > > When we call usb_reset_device() and a driver doesn't implement
> > > > pre_reset() and post_reset() methods then the USB core will attempt to
> > > > unbind and rebind the driver in order to make reset work. This is a
> > > > great general solution, but it has the potential to loop forever.
> > > > Specifically, if the USB device is in a state that the USB device
> > > > driver issues another usb_reset_device() after each rebind then we'll
> > > > just continually unbind and rebind with no end.
> > > >
> > > > It's difficult to address this condition in a USB device driver
> > > > because it's hard for the driver to keep state across each
> > > > unbind/bind.
> > >
> > > How about just adding appropriate pre_reset() and post_reset() methods?
> > > This is precisely what they are meant for.  Then the the unbind/rebind
> > > loop wouldn't ever get started.
> >
> > Right, and we have pre_reset() and post_reset() in the r1852 driver.
> > The issue is that we are seeing occasional control transfer errors
> > while the r8152 driver is still running its probe() routine and we
> > want to reset in response to those. It is relatively difficult to have
> > the pre_reset() and post_reset() methods work properly if failures
> > happen and probe() hasn't finished yet.
>
> Why is that?
>
> >  The current proposal I have
> > for the r8152 driver is to have the pre_reset() routine return -EIO if
> > we saw errors during probe, which tells the USB core to unbind/rebind
> > us. This gets us to a known good state.
>
> Don't you also get to a known good state if pre_reset() and post_reset()
> both return 0?  Then there's no unbinding, so the driver can just jump
> back to the start of its probe() routine.  Or fail the probe, if there
> have been too many errors.
>
> >  If we need to do a reset later
> > on (after probe finished successfully) then pre_reset() and
> > post_reset() can avoid the unbind/bind.
> >
> > The worry was that this could cause an infinite loop. Hence this patch. ;-)
>
> With no unbinding/rebinding, any loops that occur will be entirely under
> the driver's control.  Then it should easily be able to avoid looping
> forever.

Hmmm, OK. Let's see. I guess the most annoying thing would be dealing
with anything "devm". I guess in the case of the r8152 driver, though,
there is no use of devm. Thus I should be able to do this. Let me give
it a shot and see how it looks.

-Doug