| Message ID | 1616527652-7937-1-git-send-email-sanm@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: dwc3: core: Add shutdown callback for dwc3 | expand |
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,
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
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,
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
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
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
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
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...
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
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
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
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
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
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),
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(-)