RE: [PATCH 2/2] usb: host: ehci-platform: add a quirk to avoid stuck

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 21 Jan 2020, Yoshihiro Shimoda wrote:

> Hi Alan,
> 
> > From: Alan Stern, Sent: Tuesday, January 21, 2020 12:12 AM
> > 
> > On Mon, 20 Jan 2020, Yoshihiro Shimoda wrote:
> > 
> > > > > +static void ehci_platform_quirk_poll_work(struct work_struct *work)
> > > > > +{
> > > > > +	struct ehci_platform_priv *priv =
> > > > > +		container_of(work, struct ehci_platform_priv, poll_work);
> > > > > +	struct ehci_hcd *ehci = container_of((void *)priv, struct ehci_hcd,
> > > > > +					     priv);
> > > > > +	int i;
> > > > > +
> > > > > +	usleep_range(4000, 8000);
> > > >
> > > > You have just waited 1000 ms for the timer.  Why will sleeping an
> > > > additional 4 - 8 ms make any difference?
> > >
> > > This sleeping can avoid a misdetection between this work function and
> > > reconnection. If user reconnects the usb within 4 ms, the PORTSC
> > > condition is possible to be the same as the issue's condition.
> > > I think I should have described this information into the code.
> > >
> > > However, if I used schedule_delayed_work() instead, we can remove
> > > the usleep_range().
> > 
> > Why not just make the timer delay be 1004 or 1008 ms instead of adding
> > this extra delay here?
> 
> My concern is a race condition when the issue doesn't happen. If
> the workaround code has an extra delay, we can detect misdetection like below.
> This is related to the EHCI/OHCI controllers on R-Car Gen3 SoCs though,
> updating the CCS status is possible to be delayed. To be clear of the reason,
> I should have described this CCS status behavior too.
> 
> Timer routine		workqueue		EHCI PORTSC	USB connection
> 								disconnect
> 						CCS=0		connect (within 4 ms)
> condition = true (misdetection)			CCS=0
> 			usleep_range(4000,8000)	CCS=1
> 			condition = false

Okay, now I understand.  I misread the code in the original patch.
But now it looks like the code does roughly this:

Timer routine:	if (ehci_platform_quirk_poll_check_condition(ehci))
			schedule_work();

Work routine:	usleep_range(4000, 8000);
		udelay(10);
		if (!ehci_platform_quirk_poll_check_condition(ehci))
			return;
		udelay(10);
		if (!ehci_platform_quirk_poll_check_condition(ehci))
			return;
		ehci_platform_quirk_poll_rebind_companion(ehci);

So there are three calls to quirk_poll_check_condition, with 4 - 8 ms
between the first and second, and 10 us between the second and third.
Do you really need to have this combination of a long delay followed by
a short delay?  Wouldn't two check_condition calls with only one delay
be good enough?

Alan Stern




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux