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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Alan,

> From: Alan Stern, Sent: Wednesday, January 22, 2020 12:10 AM
<snip>
> 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?

I had implemented this code by using hardware team's suggestion without
any doubt. So, I asked hardware team about this combination of delays.
The hardware team said this combination can reduce misdetection ratio
from noise and so on. They also said we can wait single 5 ms instead
this combination (but this cannot reduce misdetection ratio).

So, now I'm thinking that the following process (single wait) is
enough and it can improve readability. But, what do you think?

Timer routine:	if (ehci_platform_quirk_poll_check_condition(ehci))
 			schedule_delayed_work(5 ms);

Delayed work routine:
		if (!ehci_platform_quirk_poll_check_condition(ehci))
 			return;
 		ehci_platform_quirk_poll_rebind_companion(ehci);

Best regards,
Yoshihiro Shimoda

> Alan Stern