[PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sandeep Maheswaram]

This patch adds a shutdown callback to USB DWC core driver to ensure that
it is properly shutdown in reboot/shutdown path. This is required
where SMMU address translation is enabled like on SC7180
SoC and few others. If the hardware is still accessing memory after
SMMU translation is disabled as part of SMMU shutdown callback in
system reboot or shutdown path, then IOVAs(I/O virtual address)
which it was using will go on the bus as the physical addresses which
might result in unknown crashes (NoC/interconnect errors).

Previously this was added in dwc3 qcom glue driver.
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
But observed kernel panic as glue driver shutdown getting called after
iommu shutdown. As we are adding iommu nodes in dwc core node
in device tree adding shutdown callback in core driver seems correct.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 drivers/usb/dwc3/core.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 94fdbe5..777b2b5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1634,11 +1634,9 @@ static int dwc3_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int dwc3_remove(struct platform_device *pdev)
+static void __dwc3_teardown(struct dwc3 *dwc)
 {
-	struct dwc3	*dwc = platform_get_drvdata(pdev);
-
-	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_get_sync(dwc->dev);
 
 	dwc3_debugfs_exit(dwc);
 	dwc3_core_exit_mode(dwc);
@@ -1646,19 +1644,32 @@ static int dwc3_remove(struct platform_device *pdev)
 	dwc3_core_exit(dwc);
 	dwc3_ulpi_exit(dwc);
 
-	pm_runtime_disable(&pdev->dev);
-	pm_runtime_put_noidle(&pdev->dev);
-	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_disable(dwc->dev);
+	pm_runtime_put_noidle(dwc->dev);
+	pm_runtime_set_suspended(dwc->dev);
 
 	dwc3_free_event_buffers(dwc);
 	dwc3_free_scratch_buffers(dwc);
 
 	if (dwc->usb_psy)
 		power_supply_put(dwc->usb_psy);
+}
+
+static int dwc3_remove(struct platform_device *pdev)
+{
+	struct dwc3	*dwc = platform_get_drvdata(pdev);
+
+	__dwc3_teardown(dwc);
 
 	return 0;
 }
 
+static void dwc3_shutdown(struct platform_device *pdev)
+{
+	struct dwc3	*dwc = platform_get_drvdata(pdev);
+
+	__dwc3_teardown(dwc);
+}
 #ifdef CONFIG_PM
 static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 {
@@ -1976,6 +1987,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),
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Stephen Boyd]

Quoting Sandeep Maheswaram (2021-03-23 12:27:32)
> This patch adds a shutdown callback to USB DWC core driver to ensure that
> it is properly shutdown in reboot/shutdown path. This is required
> where SMMU address translation is enabled like on SC7180
> SoC and few others. If the hardware is still accessing memory after
> SMMU translation is disabled as part of SMMU shutdown callback in
> system reboot or shutdown path, then IOVAs(I/O virtual address)
> which it was using will go on the bus as the physical addresses which
> might result in unknown crashes (NoC/interconnect errors).
> 
> Previously this was added in dwc3 qcom glue driver.
> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> But observed kernel panic as glue driver shutdown getting called after
> iommu shutdown. As we are adding iommu nodes in dwc core node
> in device tree adding shutdown callback in core driver seems correct.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>  drivers/usb/dwc3/core.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 94fdbe5..777b2b5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1634,11 +1634,9 @@ static int dwc3_probe(struct platform_device *pdev)
>         return ret;
>  }
>  
> -static int dwc3_remove(struct platform_device *pdev)
> +static void __dwc3_teardown(struct dwc3 *dwc)
>  {
> -       struct dwc3     *dwc = platform_get_drvdata(pdev);
> -
> -       pm_runtime_get_sync(&pdev->dev);
> +       pm_runtime_get_sync(dwc->dev);
>  
>         dwc3_debugfs_exit(dwc);
>         dwc3_core_exit_mode(dwc);
> @@ -1646,19 +1644,32 @@ static int dwc3_remove(struct platform_device *pdev)
>         dwc3_core_exit(dwc);
>         dwc3_ulpi_exit(dwc);
>  
> -       pm_runtime_disable(&pdev->dev);
> -       pm_runtime_put_noidle(&pdev->dev);
> -       pm_runtime_set_suspended(&pdev->dev);
> +       pm_runtime_disable(dwc->dev);
> +       pm_runtime_put_noidle(dwc->dev);
> +       pm_runtime_set_suspended(dwc->dev);
>  
>         dwc3_free_event_buffers(dwc);
>         dwc3_free_scratch_buffers(dwc);
>  
>         if (dwc->usb_psy)
>                 power_supply_put(dwc->usb_psy);
> +}
> +
> +static int dwc3_remove(struct platform_device *pdev)
> +{
> +       struct dwc3     *dwc = platform_get_drvdata(pdev);
> +
> +       __dwc3_teardown(dwc);
>  
>         return 0;
>  }
>  
> +static void dwc3_shutdown(struct platform_device *pdev)
> +{
> +       struct dwc3     *dwc = platform_get_drvdata(pdev);
> +
> +       __dwc3_teardown(dwc);
> +}

Can't this be

	static void dwc3_shutdown(struct platform_device *pdev)
	{
	       dwc3_remove(pdev);
	}

and then there's nothing else to change? Basically ignore return value
of dwc3_remove() to make shutdown and remove harmonize. I also wonder if
this is more common than we think and a struct driver flag could be set
to say "call remove for shutdown" and then have driver core swizzle on
that and save some duplicate functions.

>  #ifdef CONFIG_PM
>  static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  {
> @@ -1976,6 +1987,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
>  static struct platform_driver dwc3_driver = {
>         .probe          = dwc3_probe,
>         .remove         = dwc3_remove,
> +       .shutdown   = dwc3_shutdown,

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sandeep Maheswaram]

On 3/24/2021 9:01 AM, Stephen Boyd wrote:
> Quoting Sandeep Maheswaram (2021-03-23 12:27:32)
>> This patch adds a shutdown callback to USB DWC core driver to ensure that
>> it is properly shutdown in reboot/shutdown path. This is required
>> where SMMU address translation is enabled like on SC7180
>> SoC and few others. If the hardware is still accessing memory after
>> SMMU translation is disabled as part of SMMU shutdown callback in
>> system reboot or shutdown path, then IOVAs(I/O virtual address)
>> which it was using will go on the bus as the physical addresses which
>> might result in unknown crashes (NoC/interconnect errors).
>>
>> Previously this was added in dwc3 qcom glue driver.
>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>> But observed kernel panic as glue driver shutdown getting called after
>> iommu shutdown. As we are adding iommu nodes in dwc core node
>> in device tree adding shutdown callback in core driver seems correct.
>>
>> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
>> ---
>>   drivers/usb/dwc3/core.c | 26 +++++++++++++++++++-------
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 94fdbe5..777b2b5 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1634,11 +1634,9 @@ static int dwc3_probe(struct platform_device *pdev)
>>          return ret;
>>   }
>>   
>> -static int dwc3_remove(struct platform_device *pdev)
>> +static void __dwc3_teardown(struct dwc3 *dwc)
>>   {
>> -       struct dwc3     *dwc = platform_get_drvdata(pdev);
>> -
>> -       pm_runtime_get_sync(&pdev->dev);
>> +       pm_runtime_get_sync(dwc->dev);
>>   
>>          dwc3_debugfs_exit(dwc);
>>          dwc3_core_exit_mode(dwc);
>> @@ -1646,19 +1644,32 @@ static int dwc3_remove(struct platform_device *pdev)
>>          dwc3_core_exit(dwc);
>>          dwc3_ulpi_exit(dwc);
>>   
>> -       pm_runtime_disable(&pdev->dev);
>> -       pm_runtime_put_noidle(&pdev->dev);
>> -       pm_runtime_set_suspended(&pdev->dev);
>> +       pm_runtime_disable(dwc->dev);
>> +       pm_runtime_put_noidle(dwc->dev);
>> +       pm_runtime_set_suspended(dwc->dev);
>>   
>>          dwc3_free_event_buffers(dwc);
>>          dwc3_free_scratch_buffers(dwc);
>>   
>>          if (dwc->usb_psy)
>>                  power_supply_put(dwc->usb_psy);
>> +}
>> +
>> +static int dwc3_remove(struct platform_device *pdev)
>> +{
>> +       struct dwc3     *dwc = platform_get_drvdata(pdev);
>> +
>> +       __dwc3_teardown(dwc);
>>   
>>          return 0;
>>   }
>>   
>> +static void dwc3_shutdown(struct platform_device *pdev)
>> +{
>> +       struct dwc3     *dwc = platform_get_drvdata(pdev);
>> +
>> +       __dwc3_teardown(dwc);
>> +}
> Can't this be
>
> 	static void dwc3_shutdown(struct platform_device *pdev)
> 	{
> 	       dwc3_remove(pdev);
> 	}
>
> and then there's nothing else to change? Basically ignore return value
> of dwc3_remove() to make shutdown and remove harmonize. I also wonder if
> this is more common than we think and a struct driver flag could be set
> to say "call remove for shutdown" and then have driver core swizzle on
> that and save some duplicate functions.

I was referring to similar patch 
https://patchwork.kernel.org/project/linux-usb/patch/20190817174140.6394-1-vicencb@gmail.com/

>>   #ifdef CONFIG_PM
>>   static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>>   {
>> @@ -1976,6 +1987,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
>>   static struct platform_driver dwc3_driver = {
>>          .probe          = dwc3_probe,
>>          .remove         = dwc3_remove,
>> +       .shutdown   = dwc3_shutdown,

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sergei Shtylyov]

Hello!

On 23.03.2021 22:27, Sandeep Maheswaram wrote:

> This patch adds a shutdown callback to USB DWC core driver to ensure that
> it is properly shutdown in reboot/shutdown path. This is required
> where SMMU address translation is enabled like on SC7180
> SoC and few others. If the hardware is still accessing memory after
> SMMU translation is disabled as part of SMMU shutdown callback in
> system reboot or shutdown path, then IOVAs(I/O virtual address)

   Space before (, please.

> which it was using will go on the bus as the physical addresses which
> might result in unknown crashes (NoC/interconnect errors).
> 
> Previously this was added in dwc3 qcom glue driver.
> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> But observed kernel panic as glue driver shutdown getting called after
> iommu shutdown. As we are adding iommu nodes in dwc core node
> in device tree adding shutdown callback in core driver seems correct.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>   drivers/usb/dwc3/core.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 94fdbe5..777b2b5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[...]
> @@ -1976,6 +1987,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
>   static struct platform_driver dwc3_driver = {
>   	.probe		= dwc3_probe,
>   	.remove		= dwc3_remove,
> +	.shutdown   = dwc3_shutdown,

    Please indent = with tabs as above and below.

>   	.driver		= {
>   		.name	= "dwc3",
>   		.of_match_table	= of_match_ptr(of_dwc3_match),

MBR, Sergei

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Greg Kroah-Hartman]

On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
> This patch adds a shutdown callback to USB DWC core driver to ensure that
> it is properly shutdown in reboot/shutdown path. This is required
> where SMMU address translation is enabled like on SC7180
> SoC and few others. If the hardware is still accessing memory after
> SMMU translation is disabled as part of SMMU shutdown callback in
> system reboot or shutdown path, then IOVAs(I/O virtual address)
> which it was using will go on the bus as the physical addresses which
> might result in unknown crashes (NoC/interconnect errors).
> 
> Previously this was added in dwc3 qcom glue driver.
> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> But observed kernel panic as glue driver shutdown getting called after
> iommu shutdown. As we are adding iommu nodes in dwc core node
> in device tree adding shutdown callback in core driver seems correct.

So shouldn't you also remove this from the qcom glue driver at the same
time?  Please submit both as a patch series.

thanks,

greg k-h

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sandeep Maheswaram]

On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
>> This patch adds a shutdown callback to USB DWC core driver to ensure that
>> it is properly shutdown in reboot/shutdown path. This is required
>> where SMMU address translation is enabled like on SC7180
>> SoC and few others. If the hardware is still accessing memory after
>> SMMU translation is disabled as part of SMMU shutdown callback in
>> system reboot or shutdown path, then IOVAs(I/O virtual address)
>> which it was using will go on the bus as the physical addresses which
>> might result in unknown crashes (NoC/interconnect errors).
>>
>> Previously this was added in dwc3 qcom glue driver.
>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>> But observed kernel panic as glue driver shutdown getting called after
>> iommu shutdown. As we are adding iommu nodes in dwc core node
>> in device tree adding shutdown callback in core driver seems correct.
> So shouldn't you also remove this from the qcom glue driver at the same
> time?  Please submit both as a patch series.
>
> thanks,
>
> greg k-h

Hi Greg,

The qcom glue driver patch is not merged yet. I have just mentioned for it for reference.

Regards
Sandeep

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Greg Kroah-Hartman]

On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
> 
> On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
> > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
> > > This patch adds a shutdown callback to USB DWC core driver to ensure that
> > > it is properly shutdown in reboot/shutdown path. This is required
> > > where SMMU address translation is enabled like on SC7180
> > > SoC and few others. If the hardware is still accessing memory after
> > > SMMU translation is disabled as part of SMMU shutdown callback in
> > > system reboot or shutdown path, then IOVAs(I/O virtual address)
> > > which it was using will go on the bus as the physical addresses which
> > > might result in unknown crashes (NoC/interconnect errors).
> > > 
> > > Previously this was added in dwc3 qcom glue driver.
> > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> > > But observed kernel panic as glue driver shutdown getting called after
> > > iommu shutdown. As we are adding iommu nodes in dwc core node
> > > in device tree adding shutdown callback in core driver seems correct.
> > So shouldn't you also remove this from the qcom glue driver at the same
> > time?  Please submit both as a patch series.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> The qcom glue driver patch is not merged yet. I have just mentioned for it for reference.

You know that we can not add callbacks for no in-kernel user, so what
good is this patch for now?

confused,

greg k-h

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sai Prakash Ranjan]

On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
> On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
>> 
>> On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
>> > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
>> > > This patch adds a shutdown callback to USB DWC core driver to ensure that
>> > > it is properly shutdown in reboot/shutdown path. This is required
>> > > where SMMU address translation is enabled like on SC7180
>> > > SoC and few others. If the hardware is still accessing memory after
>> > > SMMU translation is disabled as part of SMMU shutdown callback in
>> > > system reboot or shutdown path, then IOVAs(I/O virtual address)
>> > > which it was using will go on the bus as the physical addresses which
>> > > might result in unknown crashes (NoC/interconnect errors).
>> > >
>> > > Previously this was added in dwc3 qcom glue driver.
>> > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>> > > But observed kernel panic as glue driver shutdown getting called after
>> > > iommu shutdown. As we are adding iommu nodes in dwc core node
>> > > in device tree adding shutdown callback in core driver seems correct.
>> > So shouldn't you also remove this from the qcom glue driver at the same
>> > time?  Please submit both as a patch series.
>> >
>> > thanks,
>> >
>> > greg k-h
>> 
>> Hi Greg,
>> 
>> The qcom glue driver patch is not merged yet. I have just mentioned 
>> for it for reference.
> 
> You know that we can not add callbacks for no in-kernel user, so what
> good is this patch for now?
> 

What in-kernel user? Since when does shutdown callback need an in-kernel
user? When you reboot or shutdown a system, it gets called. The reason
why the shutdown callback is needed is provided in the commit text.

Confused,
Sai Prakash Ranjan

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Greg Kroah-Hartman]

On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
> On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
> > On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
> > > 
> > > On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
> > > > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
> > > > > This patch adds a shutdown callback to USB DWC core driver to ensure that
> > > > > it is properly shutdown in reboot/shutdown path. This is required
> > > > > where SMMU address translation is enabled like on SC7180
> > > > > SoC and few others. If the hardware is still accessing memory after
> > > > > SMMU translation is disabled as part of SMMU shutdown callback in
> > > > > system reboot or shutdown path, then IOVAs(I/O virtual address)
> > > > > which it was using will go on the bus as the physical addresses which
> > > > > might result in unknown crashes (NoC/interconnect errors).
> > > > >
> > > > > Previously this was added in dwc3 qcom glue driver.
> > > > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> > > > > But observed kernel panic as glue driver shutdown getting called after
> > > > > iommu shutdown. As we are adding iommu nodes in dwc core node
> > > > > in device tree adding shutdown callback in core driver seems correct.
> > > > So shouldn't you also remove this from the qcom glue driver at the same
> > > > time?  Please submit both as a patch series.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > 
> > > Hi Greg,
> > > 
> > > The qcom glue driver patch is not merged yet. I have just mentioned
> > > for it for reference.
> > 
> > You know that we can not add callbacks for no in-kernel user, so what
> > good is this patch for now?
> > 
> 
> What in-kernel user? Since when does shutdown callback need an in-kernel
> user? When you reboot or shutdown a system, it gets called. The reason
> why the shutdown callback is needed is provided in the commit text.

As I can't see the patch here, I have no idea...

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sai Prakash Ranjan]

On 2021-03-30 16:46, Greg Kroah-Hartman wrote:
> On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
>> On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
>> > On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
>> > >
>> > > On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
>> > > > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
>> > > > > This patch adds a shutdown callback to USB DWC core driver to ensure that
>> > > > > it is properly shutdown in reboot/shutdown path. This is required
>> > > > > where SMMU address translation is enabled like on SC7180
>> > > > > SoC and few others. If the hardware is still accessing memory after
>> > > > > SMMU translation is disabled as part of SMMU shutdown callback in
>> > > > > system reboot or shutdown path, then IOVAs(I/O virtual address)
>> > > > > which it was using will go on the bus as the physical addresses which
>> > > > > might result in unknown crashes (NoC/interconnect errors).
>> > > > >
>> > > > > Previously this was added in dwc3 qcom glue driver.
>> > > > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>> > > > > But observed kernel panic as glue driver shutdown getting called after
>> > > > > iommu shutdown. As we are adding iommu nodes in dwc core node
>> > > > > in device tree adding shutdown callback in core driver seems correct.
>> > > > So shouldn't you also remove this from the qcom glue driver at the same
>> > > > time?  Please submit both as a patch series.
>> > > >
>> > > > thanks,
>> > > >
>> > > > greg k-h
>> > >
>> > > Hi Greg,
>> > >
>> > > The qcom glue driver patch is not merged yet. I have just mentioned
>> > > for it for reference.
>> >
>> > You know that we can not add callbacks for no in-kernel user, so what
>> > good is this patch for now?
>> >
>> 
>> What in-kernel user? Since when does shutdown callback need an 
>> in-kernel
>> user? When you reboot or shutdown a system, it gets called. The reason
>> why the shutdown callback is needed is provided in the commit text.
> 
> As I can't see the patch here, I have no idea...

You are replying now to the same patch which adds this shutdown callback 
:)
Anyways the qcom dwc3 driver patch which is abandoned which is also 
mentioned
in the commit text is here [1] and the new shutdown callback patch which 
we
are both replying to is in here [2]

[1] 
https://lore.kernel.org/lkml/1605162619-10064-1-git-send-email-sanm@codeaurora.org/

[2] 
https://lore.kernel.org/lkml/1616527652-7937-1-git-send-email-sanm@codeaurora.org/

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Greg Kroah-Hartman]

On Tue, Mar 30, 2021 at 06:18:43PM +0530, Sai Prakash Ranjan wrote:
> On 2021-03-30 16:46, Greg Kroah-Hartman wrote:
> > On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
> > > On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
> > > > >
> > > > > On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
> > > > > > > This patch adds a shutdown callback to USB DWC core driver to ensure that
> > > > > > > it is properly shutdown in reboot/shutdown path. This is required
> > > > > > > where SMMU address translation is enabled like on SC7180
> > > > > > > SoC and few others. If the hardware is still accessing memory after
> > > > > > > SMMU translation is disabled as part of SMMU shutdown callback in
> > > > > > > system reboot or shutdown path, then IOVAs(I/O virtual address)
> > > > > > > which it was using will go on the bus as the physical addresses which
> > > > > > > might result in unknown crashes (NoC/interconnect errors).
> > > > > > >
> > > > > > > Previously this was added in dwc3 qcom glue driver.
> > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> > > > > > > But observed kernel panic as glue driver shutdown getting called after
> > > > > > > iommu shutdown. As we are adding iommu nodes in dwc core node
> > > > > > > in device tree adding shutdown callback in core driver seems correct.
> > > > > > So shouldn't you also remove this from the qcom glue driver at the same
> > > > > > time?  Please submit both as a patch series.
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Hi Greg,
> > > > >
> > > > > The qcom glue driver patch is not merged yet. I have just mentioned
> > > > > for it for reference.
> > > >
> > > > You know that we can not add callbacks for no in-kernel user, so what
> > > > good is this patch for now?
> > > >
> > > 
> > > What in-kernel user? Since when does shutdown callback need an
> > > in-kernel
> > > user? When you reboot or shutdown a system, it gets called. The reason
> > > why the shutdown callback is needed is provided in the commit text.
> > 
> > As I can't see the patch here, I have no idea...
> 
> You are replying now to the same patch which adds this shutdown callback :)
> Anyways the qcom dwc3 driver patch which is abandoned which is also
> mentioned
> in the commit text is here [1] and the new shutdown callback patch which we
> are both replying to is in here [2]
> 
> [1] https://lore.kernel.org/lkml/1605162619-10064-1-git-send-email-sanm@codeaurora.org/
> 
> [2] https://lore.kernel.org/lkml/1616527652-7937-1-git-send-email-sanm@codeaurora.org/

Thanks, so, what am I supposed to do here?  The patch is long gone from
my queue...

greg k-h

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sai Prakash Ranjan]

On 2021-03-30 19:02, Greg Kroah-Hartman wrote:
> On Tue, Mar 30, 2021 at 06:18:43PM +0530, Sai Prakash Ranjan wrote:
>> On 2021-03-30 16:46, Greg Kroah-Hartman wrote:
>> > On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
>> > > On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
>> > > > On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
>> > > > >
>> > > > > On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
>> > > > > > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
>> > > > > > > This patch adds a shutdown callback to USB DWC core driver to ensure that
>> > > > > > > it is properly shutdown in reboot/shutdown path. This is required
>> > > > > > > where SMMU address translation is enabled like on SC7180
>> > > > > > > SoC and few others. If the hardware is still accessing memory after
>> > > > > > > SMMU translation is disabled as part of SMMU shutdown callback in
>> > > > > > > system reboot or shutdown path, then IOVAs(I/O virtual address)
>> > > > > > > which it was using will go on the bus as the physical addresses which
>> > > > > > > might result in unknown crashes (NoC/interconnect errors).
>> > > > > > >
>> > > > > > > Previously this was added in dwc3 qcom glue driver.
>> > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>> > > > > > > But observed kernel panic as glue driver shutdown getting called after
>> > > > > > > iommu shutdown. As we are adding iommu nodes in dwc core node
>> > > > > > > in device tree adding shutdown callback in core driver seems correct.
>> > > > > > So shouldn't you also remove this from the qcom glue driver at the same
>> > > > > > time?  Please submit both as a patch series.
>> > > > > >
>> > > > > > thanks,
>> > > > > >
>> > > > > > greg k-h
>> > > > >
>> > > > > Hi Greg,
>> > > > >
>> > > > > The qcom glue driver patch is not merged yet. I have just mentioned
>> > > > > for it for reference.
>> > > >
>> > > > You know that we can not add callbacks for no in-kernel user, so what
>> > > > good is this patch for now?
>> > > >
>> > >
>> > > What in-kernel user? Since when does shutdown callback need an
>> > > in-kernel
>> > > user? When you reboot or shutdown a system, it gets called. The reason
>> > > why the shutdown callback is needed is provided in the commit text.
>> >
>> > As I can't see the patch here, I have no idea...
>> 
>> You are replying now to the same patch which adds this shutdown 
>> callback :)
>> Anyways the qcom dwc3 driver patch which is abandoned which is also
>> mentioned
>> in the commit text is here [1] and the new shutdown callback patch 
>> which we
>> are both replying to is in here [2]
>> 
>> [1] 
>> https://lore.kernel.org/lkml/1605162619-10064-1-git-send-email-sanm@codeaurora.org/
>> 
>> [2] 
>> https://lore.kernel.org/lkml/1616527652-7937-1-git-send-email-sanm@codeaurora.org/
> 
> Thanks, so, what am I supposed to do here?  The patch is long gone from
> my queue...
> 

The patch was just posted about 7 days ago, maybe Sandeep can send again 
if
you prefer that.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Sandeep Maheswaram]

On 3/30/2021 7:02 PM, Greg Kroah-Hartman wrote:
> On Tue, Mar 30, 2021 at 06:18:43PM +0530, Sai Prakash Ranjan wrote:
>> On 2021-03-30 16:46, Greg Kroah-Hartman wrote:
>>> On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
>>>> On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
>>>>> On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
>>>>>> On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
>>>>>>>> This patch adds a shutdown callback to USB DWC core driver to ensure that
>>>>>>>> it is properly shutdown in reboot/shutdown path. This is required
>>>>>>>> where SMMU address translation is enabled like on SC7180
>>>>>>>> SoC and few others. If the hardware is still accessing memory after
>>>>>>>> SMMU translation is disabled as part of SMMU shutdown callback in
>>>>>>>> system reboot or shutdown path, then IOVAs(I/O virtual address)
>>>>>>>> which it was using will go on the bus as the physical addresses which
>>>>>>>> might result in unknown crashes (NoC/interconnect errors).
>>>>>>>>
>>>>>>>> Previously this was added in dwc3 qcom glue driver.
>>>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
>>>>>>>> But observed kernel panic as glue driver shutdown getting called after
>>>>>>>> iommu shutdown. As we are adding iommu nodes in dwc core node
>>>>>>>> in device tree adding shutdown callback in core driver seems correct.
>>>>>>> So shouldn't you also remove this from the qcom glue driver at the same
>>>>>>> time?  Please submit both as a patch series.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> greg k-h
>>>>>> Hi Greg,
>>>>>>
>>>>>> The qcom glue driver patch is not merged yet. I have just mentioned
>>>>>> for it for reference.
>>>>> You know that we can not add callbacks for no in-kernel user, so what
>>>>> good is this patch for now?
>>>>>
>>>> What in-kernel user? Since when does shutdown callback need an
>>>> in-kernel
>>>> user? When you reboot or shutdown a system, it gets called. The reason
>>>> why the shutdown callback is needed is provided in the commit text.
>>> As I can't see the patch here, I have no idea...
>> You are replying now to the same patch which adds this shutdown callback :)
>> Anyways the qcom dwc3 driver patch which is abandoned which is also
>> mentioned
>> in the commit text is here [1] and the new shutdown callback patch which we
>> are both replying to is in here [2]
>>
>> [1] https://lore.kernel.org/lkml/1605162619-10064-1-git-send-email-sanm@codeaurora.org/
>>
>> [2] https://lore.kernel.org/lkml/1616527652-7937-1-git-send-email-sanm@codeaurora.org/
> Thanks, so, what am I supposed to do here?  The patch is long gone from
> my queue...
>
> greg k-h

Hi Greg,

Should I resend this patch ? If so let me know your about opinion about 
Stephen's comment on just calling dwc3_remove in

dwc3_shutdown and ignoring return value.

https://lore.kernel.org/patchwork/patch/1401242/#1599316

Thanks

Sandeep

Re: [PATCH v1] usb: dwc3: core: Add shutdown callback for dwc3

[Greg Kroah-Hartman]

On Thu, Apr 08, 2021 at 10:22:57AM +0530, Sandeep Maheswaram wrote:
> 
> On 3/30/2021 7:02 PM, Greg Kroah-Hartman wrote:
> > On Tue, Mar 30, 2021 at 06:18:43PM +0530, Sai Prakash Ranjan wrote:
> > > On 2021-03-30 16:46, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 30, 2021 at 03:25:58PM +0530, Sai Prakash Ranjan wrote:
> > > > > On 2021-03-30 14:37, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Mar 30, 2021 at 02:12:04PM +0530, Sandeep Maheswaram wrote:
> > > > > > > On 3/26/2021 7:07 PM, Greg Kroah-Hartman wrote:
> > > > > > > > On Wed, Mar 24, 2021 at 12:57:32AM +0530, Sandeep Maheswaram wrote:
> > > > > > > > > This patch adds a shutdown callback to USB DWC core driver to ensure that
> > > > > > > > > it is properly shutdown in reboot/shutdown path. This is required
> > > > > > > > > where SMMU address translation is enabled like on SC7180
> > > > > > > > > SoC and few others. If the hardware is still accessing memory after
> > > > > > > > > SMMU translation is disabled as part of SMMU shutdown callback in
> > > > > > > > > system reboot or shutdown path, then IOVAs(I/O virtual address)
> > > > > > > > > which it was using will go on the bus as the physical addresses which
> > > > > > > > > might result in unknown crashes (NoC/interconnect errors).
> > > > > > > > > 
> > > > > > > > > Previously this was added in dwc3 qcom glue driver.
> > > > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=382449
> > > > > > > > > But observed kernel panic as glue driver shutdown getting called after
> > > > > > > > > iommu shutdown. As we are adding iommu nodes in dwc core node
> > > > > > > > > in device tree adding shutdown callback in core driver seems correct.
> > > > > > > > So shouldn't you also remove this from the qcom glue driver at the same
> > > > > > > > time?  Please submit both as a patch series.
> > > > > > > > 
> > > > > > > > thanks,
> > > > > > > > 
> > > > > > > > greg k-h
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > The qcom glue driver patch is not merged yet. I have just mentioned
> > > > > > > for it for reference.
> > > > > > You know that we can not add callbacks for no in-kernel user, so what
> > > > > > good is this patch for now?
> > > > > > 
> > > > > What in-kernel user? Since when does shutdown callback need an
> > > > > in-kernel
> > > > > user? When you reboot or shutdown a system, it gets called. The reason
> > > > > why the shutdown callback is needed is provided in the commit text.
> > > > As I can't see the patch here, I have no idea...
> > > You are replying now to the same patch which adds this shutdown callback :)
> > > Anyways the qcom dwc3 driver patch which is abandoned which is also
> > > mentioned
> > > in the commit text is here [1] and the new shutdown callback patch which we
> > > are both replying to is in here [2]
> > > 
> > > [1] https://lore.kernel.org/lkml/1605162619-10064-1-git-send-email-sanm@codeaurora.org/
> > > 
> > > [2] https://lore.kernel.org/lkml/1616527652-7937-1-git-send-email-sanm@codeaurora.org/
> > Thanks, so, what am I supposed to do here?  The patch is long gone from
> > my queue...
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Should I resend this patch ? If so let me know your about opinion about
> Stephen's comment on just calling dwc3_remove in
> 
> dwc3_shutdown and ignoring return value.
> 
> https://lore.kernel.org/patchwork/patch/1401242/#1599316

Please resend as again, it's not in my queue of patches to review at
all...

And yes, Stephen's comment does make sense, why ignore that?

thanks,

greg k-h