Re: [PATCH] usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot

[Felipe Balbi]

Hi,

gustavo panizzo <gfa@xxxxxxxxxxxx> writes:
>>>gustavo panizzo <gfa@xxxxxxxxxxxx> writes:
>>>>>>>>>>>---
>>>>>>>>>>>drivers/usb/dwc3/core.c | 33 +++++++++++++++++++++++++++++++++
>>>>>>>>>>>1 file changed, 33 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>>>>>>>index 326b302fc440..f92dfe213d89 100644
>>>>>>>>>>>--- a/drivers/usb/dwc3/core.c
>>>>>>>>>>>+++ b/drivers/usb/dwc3/core.c
>>>>>>>>>>>@@ -1259,6 +1259,38 @@ static int dwc3_probe(struct platform_device
>>>>>>>>>>>*pdev)
>>>>>>>>>>>       return ret;
>>>>>>>>>>>}
>>>>>>>>>>>
>>>>>>>>>>>+static void dwc3_shutdown(struct platform_device *pdev)
>>>>>>>>>>>+{
>>>>>>>>>>>+       struct dwc3     *dwc = platform_get_drvdata(pdev);
>>>>>>>>>>>+       struct resource *res = platform_get_resource(pdev,
>>>>>>>>>>>IORESOURCE_MEM, 0);
>>>>>>>>>>>+
>>>>>>>>>>>+       pm_runtime_get_sync(&pdev->dev);
>>>>>>>>>>>+       /*
>>>>>>>>>>>+        * restore res->start back to its original value so that, in case
>>>>>>>>>>>the
>>>>>>>>>>>+        * probe is deferred, we don't end up getting error in request
>>>>>>>>>>>the
>>>>>>>>>>>+        * memory region the next time probe is called.
>>>>>>>>>>>+        */
>>>>>>>>>>>+       res->start -= DWC3_GLOBALS_REGS_START;
>>>>>>>>>>>+
>>>>>>>>>>>+       dwc3_debugfs_exit(dwc);
>>>>>>>>>>>+       dwc3_core_exit_mode(dwc);
>>>>>>>>>>>+       dwc3_event_buffers_cleanup(dwc);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>What about dwc3_event_buffers_cleanup? should I remove it from
>>>>>>>>>dwc3_shutdown()?
>>>>>>>>>It is already in dwc3_core_exit()
>>>>>>>>
>>>>>>>>I think so. We should avoid duplicate code.
>>>>>>>>
>>>>>>>>>>>+       dwc3_free_event_buffers(dwc);
>>>>>>>>>>>+
>>>>>>>>>>>+       usb_phy_set_suspend(dwc->usb2_phy, 1);
>>>>>>>>>>>+       usb_phy_set_suspend(dwc->usb3_phy, 1);
>>>>>>>>>>>+
>>>>>>>>>>>+       phy_power_off(dwc->usb2_generic_phy);
>>>>>>>>>>>+       phy_power_off(dwc->usb3_generic_phy);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>We've done these in dwc3_core_exit().
>>>>>>>
>>>>>>>This is the patch after testing on a Odroid XU4, on top of linux-next
>>>>>>>964bcc1b4f57028d56dace7d9bc5924f2eb43f36 which translates to 4.13.0-rc1-next-20170717+
>>>>>>>I tested this patch for a week without problems with heavy USB and NIC usage.
>>>>>>>Please consider merging it
>>>>
>>>>Author: Brian Kim <brian.kim@xxxxxxxxxxxxxx>
>>>>Date:   Wed Jul 12 11:26:55 2017 +0800
>>>>
>>>>   usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot
>>>>   The dwc3 could not release resources when the module is built-in
>>>>   because this module does not have shutdown method. This causes the USB
>>>>   3.0 hub is not able to detect after warm boot.
>>>>   Original patch by Brian Kim, updated and submitted upstream by gustavo
>>>>   panizzo.
>>>>   Also see https://bugs.debian.org/843448
>>>>   Signed-off-by: Brian Kim <brian.kim@xxxxxxxxxxxxxx>
>>>>   Signed-off-by: gustavo panizzo <gfa@xxxxxxxxxxxx>
>>>>
>>>>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>index 326b302fc440..09de37d47ee7 100644
>>>>--- a/drivers/usb/dwc3/core.c
>>>>+++ b/drivers/usb/dwc3/core.c
>>>>@@ -1259,6 +1259,32 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>       	return ret;
>>>>}
>>>>
>>>>+static void dwc3_shutdown(struct platform_device *pdev)
>>>>+{
>>>>+       struct dwc3     *dwc = platform_get_drvdata(pdev);
>>>>+       struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>+
>>>>+       pm_runtime_get_sync(&pdev->dev);
>>>>+       /*
>>>>+        * restore res->start back to its original value so that, in case the
>>>>+        * probe is deferred, we don't end up getting error in request the
>>>>+        * memory region the next time probe is called.
>>>>+        */
>>>>+       res->start -= DWC3_GLOBALS_REGS_START;
>>>>+
>>>>+       dwc3_debugfs_exit(dwc);
>>>>+       dwc3_core_exit_mode(dwc);
>>>>+       dwc3_event_buffers_cleanup(dwc);
>>>>+       dwc3_free_event_buffers(dwc);
>>>>+
>>>>+       dwc3_core_exit(dwc);
>>>>+       dwc3_ulpi_exit(dwc);
>>>>+
>>>>+       pm_runtime_put_sync(&pdev->dev);
>>>>+       pm_runtime_allow(&pdev->dev);
>>>>+       pm_runtime_disable(&pdev->dev);
>>>>+}
>>>>+
>>>>static int dwc3_remove(struct platform_device *pdev)
>>>>{
>>>>       	struct dwc3     *dwc = platform_get_drvdata(pdev);
>>>>@@ -1488,6 +1514,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
>>>>static struct platform_driver dwc3_driver = {
>>>>        .probe          = dwc3_probe,
>>>>        .remove         = dwc3_remove,
>>>>+	.shutdown	= dwc3_shutdown,
>>>>        .driver         = {
>>>>                .name   = "dwc3",
>>>>                .of_match_table = of_match_ptr(of_dwc3_match),
>>>>
>>>>Patch applies cleanly on top of c6be5a0e3cebc145127d46a58350e05d2bcf6323 from linux-next
>>>>Can you _please_ merge it?
>>>
>>>why are you upset? You didn't do the changes I requested until now. It's
>>
>>I'm not upset
>>
>>>too late for v4.14 merge window and you didn't even send this as a
>>>proper patch. I also have no evidence that you've been testing mainline
>>>kernel, the commits you pointed me to are against a v4.9 vendor kernel.
>>
>>the commit i sent was tested, and is still running, on top linux-next
>>964bcc1b4f57028d56dace7d9bc5924f2eb43f36, which gives an uname -r
>>4.13.0-rc1-next-20170717+
>>
>>
>>>
>>>Test this against a vanilla tree (v4.13 was tagged days ago) and give me
>>>logs showing the problem without your commit.
>
> the patch does not work on top of 4.13.0 :(
>
> could you draft a patch i can test?

why me? I don't have this platform. I have no idea what you need. You
(or the patch author) should be the one fixing comments to the patch.

Good luck

-- 
balbi

Attachment: signature.asc
Description: PGP signature