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

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

[Yoshihiro Shimoda]

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.

Yoshihiro Shimoda (2):
  dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck
  usb: host: ehci-platform: add a quirk to avoid stuck

 .../devicetree/bindings/usb/generic-ehci.yaml      |   5 +
 drivers/usb/host/ehci-platform.c                   | 104 +++++++++++++++++++++
 2 files changed, 109 insertions(+)

-- 
2.7.4

[PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Yoshihiro Shimoda]

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.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/devicetree/bindings/usb/generic-ehci.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/generic-ehci.yaml b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
index 10edd05..c3d2dae 100644
--- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml
+++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
@@ -63,6 +63,11 @@ properties:
     description:
       Set this flag to force EHCI reset after resume.
 
+  needs-polling-to-avoid-stuck:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Set this flag to avoid getting EHCI stuck.
+
   companion:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Geert Uytterhoeven]

Hi Shimoda-san,

Thanks for your patch!

On Fri, Jan 17, 2020 at 11:54 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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.

Oops... Is this limited to the EHCI/OHCI implementation on R-Car Gen3?
Or can it happen on any EHCI/OHCI controller?

> --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> @@ -63,6 +63,11 @@ properties:
>      description:
>        Set this flag to force EHCI reset after resume.
>
> +  needs-polling-to-avoid-stuck:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Set this flag to avoid getting EHCI stuck.
> +
>    companion:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:

If this issue is specific to the EHCI/OHCI implementation on R-Car Gen3,
I don't think this is the best solution to handle it.
It might be better to add family/SoC-specific compatible values for the
EHCI/OHCI controllers in R-Car Gen3 SoCs, cfr. the (undocumented)
"ibm,usb-ehci-440epx" and "allwinner,sun4i-a10-ehci" compatible values
in the example in the DT bindings file (probably we should have done so
from the start, like for all other devices).
Then the driver can handle the issue based on the compatible value.

Besides, what about DT backwards compatibility?
To enable this quirk handling, the DT must be updated first.
This is true for solutions based on either a DT property or on a
different compatible value.
Of course, you can always use soc_device_match()...

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Saturday, January 18, 2020 1:03 AM
> 
> Hi Shimoda-san,
> 
> Thanks for your patch!
> 
> On Fri, Jan 17, 2020 at 11:54 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> 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.
> 
> Oops... Is this limited to the EHCI/OHCI implementation on R-Car Gen3?
> Or can it happen on any EHCI/OHCI controller?

This is limited on R-Car Gen3 (and perhaps RZ/A2 and RZ/G2).
But, R-Mobile A1 and R-Car Gen1/2 don't have this issue.
And I don't know any other EHCI/OHCI controller has this issue.

> > --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > @@ -63,6 +63,11 @@ properties:
> >      description:
> >        Set this flag to force EHCI reset after resume.
> >
> > +  needs-polling-to-avoid-stuck:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Set this flag to avoid getting EHCI stuck.
> > +
> >    companion:
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >      description:
> 
> If this issue is specific to the EHCI/OHCI implementation on R-Car Gen3,
> I don't think this is the best solution to handle it.
> It might be better to add family/SoC-specific compatible values for the
> EHCI/OHCI controllers in R-Car Gen3 SoCs, cfr. the (undocumented)
> "ibm,usb-ehci-440epx" and "allwinner,sun4i-a10-ehci" compatible values
> in the example in the DT bindings file (probably we should have done so
> from the start, like for all other devices).
> Then the driver can handle the issue based on the compatible value.

I understood it. And I'm also think adding family/SoC-specific compatible
values are better.

> Besides, what about DT backwards compatibility?
> To enable this quirk handling, the DT must be updated first.
> This is true for solutions based on either a DT property or on a
> different compatible value.
> Of course, you can always use soc_device_match()...

In this case, I should apply this quirk to some SoCs,
I think using DT is better than soc_device_match().

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

RE: [PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Yoshihiro Shimoda]

Hi Geert-san again,

> From: Yoshihiro Shimoda, Sent: Monday, January 20, 2020 5:05 PM
> > > --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > > @@ -63,6 +63,11 @@ properties:
> > >      description:
> > >        Set this flag to force EHCI reset after resume.
> > >
> > > +  needs-polling-to-avoid-stuck:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description:
> > > +      Set this flag to avoid getting EHCI stuck.
> > > +
> > >    companion:
> > >      $ref: /schemas/types.yaml#/definitions/phandle
> > >      description:
> >
> > If this issue is specific to the EHCI/OHCI implementation on R-Car Gen3,
> > I don't think this is the best solution to handle it.
> > It might be better to add family/SoC-specific compatible values for the
> > EHCI/OHCI controllers in R-Car Gen3 SoCs, cfr. the (undocumented)
> > "ibm,usb-ehci-440epx" and "allwinner,sun4i-a10-ehci" compatible values
> > in the example in the DT bindings file (probably we should have done so
> > from the start, like for all other devices).
> > Then the driver can handle the issue based on the compatible value.
> 
> I understood it. And I'm also think adding family/SoC-specific compatible
> values are better.

I'm trying to add some undocumented compatible values, but it seems hard
to add because:
- Some dts[i] files have undocumented compatible strings.
 # We can find it by using the following command:
 # $ grep "generic-ehci" `find -name "*.dts*"` | grep ","

- I tried to use "oneOf:" and "contains:" combination, but it failed.

- This generic-ehci.yaml uses "contains:" for the compatible now.
  So, even if compatible property has undocumented compatible string,
  make dtbs_check command succeeded (except node names).

- In my opinion, almost all user (excect R-Car SoCs) doesn't needs
  specific compatible values, so that adding such compatible values
  causes less usability in the future.

So, I suspended adding specific compatible values and I'll use
soc_device_match() for this workaround for now...

Best regards,
Yoshihiro Shimoda

Re: [PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Jan 23, 2020 at 9:17 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Yoshihiro Shimoda, Sent: Monday, January 20, 2020 5:05 PM
> > > > --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > > > @@ -63,6 +63,11 @@ properties:
> > > >      description:
> > > >        Set this flag to force EHCI reset after resume.
> > > >
> > > > +  needs-polling-to-avoid-stuck:
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +    description:
> > > > +      Set this flag to avoid getting EHCI stuck.
> > > > +
> > > >    companion:
> > > >      $ref: /schemas/types.yaml#/definitions/phandle
> > > >      description:
> > >
> > > If this issue is specific to the EHCI/OHCI implementation on R-Car Gen3,
> > > I don't think this is the best solution to handle it.
> > > It might be better to add family/SoC-specific compatible values for the
> > > EHCI/OHCI controllers in R-Car Gen3 SoCs, cfr. the (undocumented)
> > > "ibm,usb-ehci-440epx" and "allwinner,sun4i-a10-ehci" compatible values
> > > in the example in the DT bindings file (probably we should have done so
> > > from the start, like for all other devices).
> > > Then the driver can handle the issue based on the compatible value.
> >
> > I understood it. And I'm also think adding family/SoC-specific compatible
> > values are better.
>
> I'm trying to add some undocumented compatible values, but it seems hard
> to add because:
> - Some dts[i] files have undocumented compatible strings.
>  # We can find it by using the following command:
>  # $ grep "generic-ehci" `find -name "*.dts*"` | grep ","
>
> - I tried to use "oneOf:" and "contains:" combination, but it failed.
>
> - This generic-ehci.yaml uses "contains:" for the compatible now.
>   So, even if compatible property has undocumented compatible string,
>   make dtbs_check command succeeded (except node names).

Probably you'll have to write a separate DT binding doc file for R-Car Gen3,
referring to generic-ehci.yaml using $ref.

> - In my opinion, almost all user (excect R-Car SoCs) doesn't needs
>   specific compatible values, so that adding such compatible values
>   causes less usability in the future.
>
> So, I suspended adding specific compatible values and I'll use
> soc_device_match() for this workaround for now...

Which has the advantage that it will enable the quirk with old DTBs, too ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] dt-bindings: usb: generic-ehci: add a quirk property to avoid stuck

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, January 23, 2020 5:57 PM
<snip>
> > I'm trying to add some undocumented compatible values, but it seems hard
> > to add because:
> > - Some dts[i] files have undocumented compatible strings.
> >  # We can find it by using the following command:
> >  # $ grep "generic-ehci" `find -name "*.dts*"` | grep ","
> >
> > - I tried to use "oneOf:" and "contains:" combination, but it failed.
> >
> > - This generic-ehci.yaml uses "contains:" for the compatible now.
> >   So, even if compatible property has undocumented compatible string,
> >   make dtbs_check command succeeded (except node names).
> 
> Probably you'll have to write a separate DT binding doc file for R-Car Gen3,
> referring to generic-ehci.yaml using $ref.

I see.

> > - In my opinion, almost all user (excect R-Car SoCs) doesn't needs
> >   specific compatible values, so that adding such compatible values
> >   causes less usability in the future.
> >
> > So, I suspended adding specific compatible values and I'll use
> > soc_device_match() for this workaround for now...
> 
> Which has the advantage that it will enable the quirk with old DTBs, too ;-)

I think so :)

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

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

[Yoshihiro Shimoda]

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>
---
 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)
+{
+	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 */
+		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));
+}
+
+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);
+
+	for (i = 0; i < 2; i++) {
+		udelay(10);
+		if (!ehci_platform_quirk_poll_check_condition(ehci))
+			goto out;
+	}
+
+	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);
+}
+
+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
+		ehci_platform_quirk_poll_start_timer(priv);
+}
+
+static void ehci_platform_quirk_poll_init(struct ehci_platform_priv *priv)
+{
+	INIT_WORK(&priv->poll_work, ehci_platform_quirk_poll_work);
+	timer_setup(&priv->poll_timer, ehci_platform_quirk_poll_timer, 0);
+	ehci_platform_quirk_poll_start_timer(priv);
+}
+
+static void ehci_platform_quirk_poll_end(struct ehci_platform_priv *priv)
+{
+	del_timer_sync(&priv->poll_timer);
+	flush_work(&priv->poll_work);
+}
+
 static int ehci_platform_probe(struct platform_device *dev)
 {
 	struct usb_hcd *hcd;
@@ -176,6 +262,10 @@ static int ehci_platform_probe(struct platform_device *dev)
 					  "has-transaction-translator"))
 			hcd->has_tt = 1;
 
+		if (of_property_read_bool(dev->dev.of_node,
+					  "needs-polling-to-avoid-stuck"))
+			priv->quirk_poll = true;
+
 		for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
 			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
 			if (IS_ERR(priv->clks[clk])) {
@@ -247,6 +337,9 @@ static int ehci_platform_probe(struct platform_device *dev)
 	device_enable_async_suspend(hcd->self.controller);
 	platform_set_drvdata(dev, hcd);
 
+	if (priv->quirk_poll)
+		ehci_platform_quirk_poll_init(priv);
+
 	return err;
 
 err_power:
@@ -273,6 +366,9 @@ static int ehci_platform_remove(struct platform_device *dev)
 	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
 	int clk;
 
+	if (priv->quirk_poll)
+		ehci_platform_quirk_poll_end(priv);
+
 	usb_remove_hcd(hcd);
 
 	if (pdata->power_off)
@@ -297,9 +393,13 @@ static int ehci_platform_suspend(struct device *dev)
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct usb_ehci_pdata *pdata = dev_get_platdata(dev);
 	struct platform_device *pdev = to_platform_device(dev);
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
 	bool do_wakeup = device_may_wakeup(dev);
 	int ret;
 
+	if (priv->quirk_poll)
+		ehci_platform_quirk_poll_end(priv);
+
 	ret = ehci_suspend(hcd, do_wakeup);
 	if (ret)
 		return ret;
@@ -331,6 +431,10 @@ static int ehci_platform_resume(struct device *dev)
 	}
 
 	ehci_resume(hcd, priv->reset_on_resume);
+
+	if (priv->quirk_poll)
+		ehci_platform_quirk_poll_init(priv);
+
 	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.7.4

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

[Alan Stern]

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.

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.

> +{
> +	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)

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

> +
> +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?

> +
> +	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;

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

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

Alan Stern

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

[Yoshihiro Shimoda]

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

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

[Alan Stern]

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?

Alan Stern

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

[Yoshihiro Shimoda]

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

Best regards,
Yoshihiro Shimoda

> Alan Stern

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

[Alan Stern]

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

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

[Yoshihiro Shimoda]

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

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

[Alan Stern]

On Wed, 22 Jan 2020, Yoshihiro Shimoda wrote:

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

Sure, the more times you delay and recheck, the better the error rate.  
But you don't want to go too far.

> 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);

That looks good to me.

Alan Stern

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

[Yoshihiro Shimoda]

Hi Alan,

> From: Alan Stern, Sent: Wednesday, January 22, 2020 11:59 PM
> 
> On Wed, 22 Jan 2020, Yoshihiro Shimoda wrote:
> 
> > > 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).
> 
> Sure, the more times you delay and recheck, the better the error rate.
> But you don't want to go too far.

You're correct. However, my mind is changed a little...
I would like to remain rechecking as the same as before.

> > 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);
> 
> That looks good to me.

Thank you for your feedback! I'll submit v2 patch soon.

Best regards,
Yoshihiro Shimoda

> Alan Stern