RE: [PATCH V1 1/1] usb: hub: introduce delay after usb hub mutex release

["Chugh, Sanjeev" <Sanjeev_Chugh@mentor.com>]

Hello Greg,

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, December 3, 2021 6:22 PM
> To: Chugh, Sanjeev <Sanjeev_Chugh@mentor.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [PATCH V1 1/1] usb: hub: introduce delay after usb hub mutex
> release
> 
> On Wed, Dec 01, 2021 at 06:28:08AM +0530, Sanjeev Chugh wrote:
> > Rogue usb sticks can cause endless port connect change events due to
> > unstable electric connection between usb chip and the port. Port
> > connect change will cause device enumeration for the new device
> > connection and new device processing is done with the usb hub mutex
> > acquired. This can cause very short time gap between unlock/lock of
> > hub device mutex. So if some thread is running at low priority than
> > hub_thread, it can face hub device mutex starvation.
> >
> > This commit introduces a sleep of 25ms after the hub device mutex is
> > unlocked in hub_events so that if hub_thread is stuck in a endless
> > loop, all other threads will get fair amount of chance to acquire the
> > usb hub mutex.
> >
> > Signed-off-by: Sanjeev Chugh <sanjeev_chugh@mentor.com>
> > ---
> >  drivers/usb/core/hub.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > 00070a8..0be2acc 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -5763,6 +5763,17 @@ static void hub_event(struct work_struct *work)
> >  out_hdev_lock:
> >  	usb_unlock_device(hdev);
> >
> > +	/*
> > +	 * Rogue usb sticks can cause endless device connection
> > +	 * events due to unstable electric connection.
> 
> What is a "usb stick"?  Any usb device could cause this, right?

In my test case, I have used a usb pen drive of Sandisk Make of 32 GB capacity. But yes, any faulty usb device or even faulty usb cable used for the usb device connection can cause this issue.
> 
> > This
> > +	 * can cause very short time gap between unlock/lock
> > +	 * of hub device mutex thus causing mutex starvation
> > +	 * for some other lower priority thread. Thus sleep
> > +	 * would give fair chance to all other threads to
> > +	 * acquire the usb hub mutex.
> > +	 */
> > +	msleep(25);
> 
> What normal code path did you just slow down here?  Why not slow down the
> disconnect path instead of the connect path?
> 
In case of faulty usb device, USB device enumeration doesn't succeed at all and comes out of hub_port_init() from a failure path. There is no device disconnection path in this flow. Therefore, this delay is added after hub_port_connect_change() is done trying to handle the port connect change event.

> thanks,
> 
> greg k-h

Thanks !

-Sanjeev