dwc3-exynos: Fix deferred probing storm.
diff mbox

Message ID E1b5Hwn-0005Be-UA@pannekake.samfundet.no
State Not Applicable
Headers show

Commit Message

Steinar H. Gunderson May 24, 2016, 6:13 p.m. UTC
dwc3-exynos has two problems during init if the regulators are slow
to come up (for instance if the I2C bus driver is not on the initramfs)
and return probe deferral. First, every time this happens, the driver
leaks the USB phys created; they need to be deallocated on error.

Second, since the phy devices are created before the regulators fail,
this means that there's a new device to re-trigger deferred probing,
which causes it to essentially go into a busy loop of re-probing the
device until the regulators come up.

Move the phy creation to after the regulators have succeeded, and also
fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
which doesn't contain the I2C driver), this reduces the number of probe
attempts (for each of the two controllers) from more than 2000 to eight.

Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
Signed-off-by: Steinar H. Gunderson <sesse@google.com>
---
 drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Comments

Krzysztof Kozlowski May 25, 2016, 7:54 a.m. UTC | #1
On 05/24/2016 08:13 PM, Steinar H. Gunderson wrote:
> dwc3-exynos has two problems during init if the regulators are slow
> to come up (for instance if the I2C bus driver is not on the initramfs)
> and return probe deferral. First, every time this happens, the driver
> leaks the USB phys created; they need to be deallocated on error.
> 
> Second, since the phy devices are created before the regulators fail,
> this means that there's a new device to re-trigger deferred probing,
> which causes it to essentially go into a busy loop of re-probing the
> device until the regulators come up.
> 
> Move the phy creation to after the regulators have succeeded, and also
> fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
> which doesn't contain the I2C driver), this reduces the number of probe
> attempts (for each of the two controllers) from more than 2000 to eight.
> 
> Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
> Signed-off-by: Steinar H. Gunderson <sesse@google.com>

This is the same person so no need for "Reported-by". The reported-by is
used if someone else submits patch after your bug report.

Please (when resubmitting or applying) add following tags:

Fixes: d720f057fda4 ("usb: dwc3: exynos: add nop transceiver support")
Cc: <stable@vger.kernel.org>

This will help downstream (like Debian) using stable kernels.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

> ---
>  drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index dd5cb55..2f1fb7e 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, exynos);
>  
> -	ret = dwc3_exynos_register_phys(exynos);
> -	if (ret) {
> -		dev_err(dev, "couldn't register PHYs\n");
> -		return ret;
> -	}
> -
>  	exynos->dev	= dev;
>  
>  	exynos->clk = devm_clk_get(dev, "usbdrd30");
> @@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  		goto err3;
>  	}
>  
> +	ret = dwc3_exynos_register_phys(exynos);
> +	if (ret) {
> +		dev_err(dev, "couldn't register PHYs\n");
> +		goto err4;
> +	}
> +
>  	if (node) {
>  		ret = of_platform_populate(node, NULL, NULL, dev);
>  		if (ret) {
>  			dev_err(dev, "failed to add dwc3 core\n");
> -			goto err4;
> +			goto err5;
>  		}
>  	} else {
>  		dev_err(dev, "no device node, failed to add dwc3 core\n");
>  		ret = -ENODEV;
> -		goto err4;
> +		goto err5;
>  	}
>  
>  	return 0;
>  
> +err5:
> +	platform_device_unregister(exynos->usb2_phy);
> +	platform_device_unregister(exynos->usb3_phy);
>  err4:
>  	regulator_disable(exynos->vdd10);
>  err3:
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Gautam May 27, 2016, 9:53 a.m. UTC | #2
On Tue, May 24, 2016 at 11:43 PM, Steinar H. Gunderson <sesse@google.com> wrote:
> dwc3-exynos has two problems during init if the regulators are slow
> to come up (for instance if the I2C bus driver is not on the initramfs)
> and return probe deferral. First, every time this happens, the driver
> leaks the USB phys created; they need to be deallocated on error.
>
> Second, since the phy devices are created before the regulators fail,
> this means that there's a new device to re-trigger deferred probing,
> which causes it to essentially go into a busy loop of re-probing the
> device until the regulators come up.
>
> Move the phy creation to after the regulators have succeeded, and also
> fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
> which doesn't contain the I2C driver), this reduces the number of probe
> attempts (for each of the two controllers) from more than 2000 to eight.
>
> Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
> Signed-off-by: Steinar H. Gunderson <sesse@google.com>
> ---

I don't have any concerns with the patch apart from the ones
Krzysztof has already pointed out.
LGTM.

After fixing the patch,
Reviewed-by: Vivek Gautam <gautam.vivek@samsung.com>

>  drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index dd5cb55..2f1fb7e 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, exynos);
>
> -       ret = dwc3_exynos_register_phys(exynos);
> -       if (ret) {
> -               dev_err(dev, "couldn't register PHYs\n");
> -               return ret;
> -       }
> -
>         exynos->dev     = dev;
>
>         exynos->clk = devm_clk_get(dev, "usbdrd30");
> @@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>                 goto err3;
>         }
>
> +       ret = dwc3_exynos_register_phys(exynos);
> +       if (ret) {
> +               dev_err(dev, "couldn't register PHYs\n");
> +               goto err4;
> +       }
> +
>         if (node) {
>                 ret = of_platform_populate(node, NULL, NULL, dev);
>                 if (ret) {
>                         dev_err(dev, "failed to add dwc3 core\n");
> -                       goto err4;
> +                       goto err5;
>                 }
>         } else {
>                 dev_err(dev, "no device node, failed to add dwc3 core\n");
>                 ret = -ENODEV;
> -               goto err4;
> +               goto err5;
>         }
>
>         return 0;
>
> +err5:
> +       platform_device_unregister(exynos->usb2_phy);
> +       platform_device_unregister(exynos->usb3_phy);
>  err4:
>         regulator_disable(exynos->vdd10);
>  err3:
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steinar H. Gunderson May 27, 2016, 11:46 a.m. UTC | #3
On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
> I don't have any concerns with the patch apart from the ones
> Krzysztof has already pointed out.
> LGTM.

Should I repost the patch, or will people just make these commit message
changes for me? I guess balbi@ is the right person to review and merge,
since he maintains the driver.

/* Steinar */
Felipe Balbi May 27, 2016, 1:12 p.m. UTC | #4
Hi,

Krzysztof Kozlowski <k.kozlowski@samsung.com> writes:
> On 05/27/2016 01:46 PM, Steinar H. Gunderson wrote:
>> On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
>>> I don't have any concerns with the patch apart from the ones
>>> Krzysztof has already pointed out.
>>> LGTM.
>> 
>> Should I repost the patch, or will people just make these commit message
>> changes for me? I guess balbi@ is the right person to review and merge,
>> since he maintains the driver.
>
> Although resend is not strictly needed because maintainer might do the
> changes by himself... but he might just miss the comments as well (and
> e.g. use patchwork to apply the patch).
>
> I think it would be useful if you send a v2 with fixes pointed by me and
> accumulated review-tags (mine and Vivek's Reviewed-by).

yes, please do that. Keep in mind, also, that we're still in the middle
of the merge window and nothing will really happen until v4.7-rc1 is
tagged.

cheers
Krzysztof Kozlowski May 27, 2016, 1:13 p.m. UTC | #5
On 05/27/2016 01:46 PM, Steinar H. Gunderson wrote:
> On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
>> I don't have any concerns with the patch apart from the ones
>> Krzysztof has already pointed out.
>> LGTM.
> 
> Should I repost the patch, or will people just make these commit message
> changes for me? I guess balbi@ is the right person to review and merge,
> since he maintains the driver.

Although resend is not strictly needed because maintainer might do the
changes by himself... but he might just miss the comments as well (and
e.g. use patchwork to apply the patch).

I think it would be useful if you send a v2 with fixes pointed by me and
accumulated review-tags (mine and Vivek's Reviewed-by).

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steinar H. Gunderson May 27, 2016, 1:25 p.m. UTC | #6
On Fri, May 27, 2016 at 04:12:59PM +0300, Felipe Balbi wrote:
> yes, please do that. Keep in mind, also, that we're still in the middle
> of the merge window and nothing will really happen until v4.7-rc1 is
> tagged.

Sent. As a fix, there's a chance it could go into 4.7, right?

/* Steinar */
Felipe Balbi May 27, 2016, 1:26 p.m. UTC | #7
Hi,

"Steinar H. Gunderson" <sgunderson@bigfoot.com> writes:
> On Fri, May 27, 2016 at 04:12:59PM +0300, Felipe Balbi wrote:
>> yes, please do that. Keep in mind, also, that we're still in the middle
>> of the merge window and nothing will really happen until v4.7-rc1 is
>> tagged.
>
> Sent. As a fix, there's a chance it could go into 4.7, right?

yup, shouldn't be a problem. But only after v4.7-rc1 is tagged.
Steinar H. Gunderson May 30, 2016, 5:46 p.m. UTC | #8
On Fri, May 27, 2016 at 04:26:42PM +0300, Felipe Balbi wrote:
>> Sent. As a fix, there's a chance it could go into 4.7, right?
> yup, shouldn't be a problem. But only after v4.7-rc1 is tagged.

Seemingly v4.7-rc1 is out today (I was surprised at how quick that was).

/* Steinar */

Patch
diff mbox

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index dd5cb55..2f1fb7e 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -128,12 +128,6 @@  static int dwc3_exynos_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, exynos);
 
-	ret = dwc3_exynos_register_phys(exynos);
-	if (ret) {
-		dev_err(dev, "couldn't register PHYs\n");
-		return ret;
-	}
-
 	exynos->dev	= dev;
 
 	exynos->clk = devm_clk_get(dev, "usbdrd30");
@@ -183,20 +177,29 @@  static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+	ret = dwc3_exynos_register_phys(exynos);
+	if (ret) {
+		dev_err(dev, "couldn't register PHYs\n");
+		goto err4;
+	}
+
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, dev);
 		if (ret) {
 			dev_err(dev, "failed to add dwc3 core\n");
-			goto err4;
+			goto err5;
 		}
 	} else {
 		dev_err(dev, "no device node, failed to add dwc3 core\n");
 		ret = -ENODEV;
-		goto err4;
+		goto err5;
 	}
 
 	return 0;
 
+err5:
+	platform_device_unregister(exynos->usb2_phy);
+	platform_device_unregister(exynos->usb3_phy);
 err4:
 	regulator_disable(exynos->vdd10);
 err3: