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

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

Hi Alan,

Thank you for your review!

> From: Alan Stern, Sent: Saturday, January 18, 2020 1:27 AM
> 
> On Fri, 17 Jan 2020, Yoshihiro Shimoda wrote:
> 
> > Since EHCI/OHCI controllers on R-Car Gen3 SoCs are possible to
> > be getting stuck very rarely after a full/low usb device was
> > disconnected. To detect/recover from such a situation, the controllers
> > require a special way which poll the EHCI PORTSC register and changes
> > the OHCI functional state.
> >
> > So, this patch adds a polling timer into the ehci-platform driver,
> > and if the ehci driver detects the issue by the EHCI PORTSC register,
> > the ehci driver removes a companion device (= the OHCI controller)
> > to change the OHCI functional state to USB Reset once. And then,
> > the ehci driver adds the companion device again.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> The programming in this patch could be improved in several ways.
> 
> > ---
> >  drivers/usb/host/ehci-platform.c | 104 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 104 insertions(+)
> >
> > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> > index 769749c..fc6bb06 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> > +#include <linux/timer.h>
> >  #include <linux/usb.h>
> >  #include <linux/usb/hcd.h>
> >  #include <linux/usb/ehci_pdriver.h>
> > @@ -44,6 +45,9 @@ struct ehci_platform_priv {
> >  	struct clk *clks[EHCI_MAX_CLKS];
> >  	struct reset_control *rsts;
> >  	bool reset_on_resume;
> > +	bool quirk_poll;
> > +	struct timer_list poll_timer;
> > +	struct work_struct poll_work;
> >  };
> >
> >  static const char hcd_name[] = "ehci-platform";
> > @@ -118,6 +122,88 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
> >  	.power_off =		ehci_platform_power_off,
> >  };
> >
> > +static bool ehci_platform_quirk_poll_check_condition(struct ehci_hcd *ehci)
> 
> There should be a kerneldoc section above this line, explaining what
> the function does and why it is needed.  Otherwise people reading this
> code for the first time will have no idea what is going on.

I got it. I'll add a kerneldoc section.

> You don't really need the "ehci_platform_" at the start of the function
> name, because this is a static function.
> 
> Also, "quirk_poll_check_condition" suggests that this is the _only_
> condition that a quirk might need to poll for.  What if another similar
> quirk arises in the future?  The function name should indicate
> something about what condition is being checked.

I got it. I'll change the function name to quirk_poll_stuck_connection().

> > +{
> > +	u32 port_status = ehci_readl(ehci, &ehci->regs->port_status[0]);
> > +
> > +	if (!(port_status & PORT_OWNER) &&	/* PO == 0b */
> > +	    port_status & PORT_POWER &&		/* PP == 1b */
> > +	    !(port_status & PORT_CONNECT) &&	/* CCS == 0b */
> > +	    port_status & GENMASK(11, 10))	/* LS != 00b */
> 
> The comments are unnecessary.  Anyone reading the code will realize
> that !(port_status & PORT_OWNER) means that the PO value is 0b, and so
> on.
> 
> Also, I think the code would be a little clearer if all the tests were
> inside parentheses, not just the negated tests.
> 
> The GENMASK stuff is very obscure.  You could define a PORT_LS_MASK
> macro in include/linux/usb/ehci_defs.h to be (3<<10), and make the
> test:
> 
> 	(port_status & PORT_LS_MASK)

I got it. I'll fix them.

> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +static void ehci_platform_quirk_poll_rebind_companion(struct ehci_hcd *ehci)
> > +{
> > +	struct device *companion_dev;
> > +	struct usb_hcd *hcd = ehci_to_hcd(ehci);
> > +
> > +	companion_dev = usb_of_get_companion_dev(hcd->self.controller);
> > +	if (!companion_dev)
> > +		return;
> > +
> > +	device_release_driver(companion_dev);
> > +	if (device_attach(companion_dev) < 0)
> > +		ehci_err(ehci, "%s: failed\n", __func__);
> > +
> > +	put_device(companion_dev);
> > +}
> > +
> > +static void ehci_platform_quirk_poll_start_timer(struct ehci_platform_priv *p)
> > +{
> > +	mod_timer(&p->poll_timer, jiffies + msecs_to_jiffies(1000));
> > +}
> 
> Does this really need to be in a separate function?  Why not include it
> inline wherever it is used?
> 
> Also, instead of msecs_to_jiffies(1000) you can just write HZ.

I intended to avoid using magical number "1000", but using HZ and including it
inline are better. I'll fix it.

> > +
> > +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().

> > +
> > +	for (i = 0; i < 2; i++) {
> > +		udelay(10);
> > +		if (!ehci_platform_quirk_poll_check_condition(ehci))
> > +			goto out;
> > +	}
> 
> This will be clearer if you expand the loop and add a comment:
> 
> 	/* Make sure the condition persists for at least 10 us */
> 	if (!ehci_platform_quirk_poll_check_condition(ehci))
> 		return;
> 	udelay(10);
> 	if (!ehci_platform_quirk_poll_check_condition(ehci))
> 		return;

I think so. So, I'll fix it.

> > +
> > +	ehci_dbg(ehci, "%s: detected getting stuck. rebind now!\n", __func__);
> > +	ehci_platform_quirk_poll_rebind_companion(ehci);
> > +
> > +out:
> > +	ehci_platform_quirk_poll_start_timer(priv);
> 
> You don't need to restart the timer here ...
> 
> > +}
> > +
> > +static void ehci_platform_quirk_poll_timer(struct timer_list *t)
> > +{
> > +	struct ehci_platform_priv *priv = from_timer(priv, t, poll_timer);
> > +	struct ehci_hcd *ehci = container_of((void *)priv, struct ehci_hcd,
> > +					     priv);
> > +
> > +	if (ehci_platform_quirk_poll_check_condition(ehci))
> > +		schedule_work(&priv->poll_work);
> > +	else
> 
> ... if you simply remove this "else" line.

I think so. I was worried about a race condition between this timer function
and the work function. But, this will not happen because the timer is every 1 s.

> > +		ehci_platform_quirk_poll_start_timer(priv);
> 
> Also, it would be a lot cleaner if you run all the check_condition
> stuff inside the timer routine here, and use the work routine only for
> rebind_companion.

If using mdelay(4) in the timer routine here can be acceptable,
it would be a lot cleaner. But, Documentation/timers/timers-howto.rst
suggests to avoid using mdelay() in atomic.

Best regards.
Yoshihiro Shimoda

> Alan Stern