Re: [Patch v2] usb: core: stop USB enumeration if too many retries

[Alan Stern <stern@rowland.harvard.edu>]

On Sat, Sep 03, 2022 at 12:08:04AM +0800, Ray Chi wrote:
> On Fri, Sep 2, 2022 at 10:49 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, Sep 02, 2022 at 05:15:35PM +0800, Ray Chi wrote:
> > > If a broken accessory connected to a USB host, usbcore might
> > > keep doing enumeration retries and it will take a long time to
> > > cause system unstable.
> > >
> > > This patch provides a quirk to specific USB ports of the hub to
> > > stop USB enumeration if needed.
> >
> > This seems very awkward.  Why not have a quirk that prevents USB
> > enumeration completely, instead of after some number of retries?  After
> > all, if the port is connected to a broken accessory, there's no reason
> > to try enumerating it even once.
> >
> > For that matter, have you tried using the existing "disabled" port
> > attribute instead of adding a new quirk?  Does it already solve your
> > problem?
> >
> 
> Since we don't know if the connected accessory is normal or broken, doing port
> initialization is necessary.

I don't understand.  If you don't know whether the accessory is broken, 
how do you know whether to set the quirk?

On the other hand, if you always set the quirk even before you know 
whether the accessory is broken, why make it a quirk at all?  Why not 
make it the normal behavior of the driver?

> > > +              * Some USB hosts can't take a long time to keep doing enumeration
> > > +              * retry. After doing half of the retries, we would turn off the port
> > > +              * power to stop enumeration if the quirk is set.
> >
> > What made you decide that half of the retries was the right place to
> > stop?  Why not do all the retries?
> 
> Since some normal devices will be timeout in the first attempt, I set
> the condition to half
> of the retries. All the retries will take 12*timeout seconds. It is
> too long so that a watchdog
> timeout problem may happen.

Why not set CONFIG_USB_FEW_INIT_RETRIES instead?

> > If the quirk prevented enumeration completely then this function
> > wouldn't be needed.
> 
> The enumeration is still needed as above.
> 
> >
> > > +
> > >  /* Check if a port is power on */
> > >  int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus)
> > >  {
> > > @@ -4855,6 +4879,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> > >                                       buf->bMaxPacketSize0;
> > >                       kfree(buf);
> > >
> > > +                     if (r < 0 && (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM)) {
> >
> > How come this line tests the quirk but doesn't call
> > hub_port_stop_enumerate()?
> 
> Since the quirk is used to stop enumeration and reduce the total time.
> If the port has the quirk, I think the port doesn't need to do
> set_address after the port gets
> failures in the new scheme. It will add 2 attempts * timeout (defined
> in hc_driver) seconds.

I still can't tell what you're trying to accomplish.  You need to do a 
much better job of explaining the point of this.  For instance, you 
might describe in detail a situation where the quirk is needed, 
explaining what sort of behavior of the system would lead you to set the 
quirk, and why.

Alan Stern