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@xxxxxxxxxxx>
>
> 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
