| Message ID | c8c4ea38-c0f7-756d-01fb-cab51a3f8393@omp.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a5f928db59519a15e82ecba4ae3e7cbf5a44715a |
| Headers | show |
| Series | [v5] usb: host: xhci-plat: fix possible kernel oops while resuming | expand |
On 4.10.2023 23.47, Sergey Shtylyov wrote: > If this driver enables the xHC clocks while resuming from sleep, it calls > clk_prepare_enable() without checking for errors and blithely goes on to > read/write the xHC's registers -- which, with the xHC not being clocked, > at least on ARM32 usually causes an imprecise external abort exceptions > which cause kernel oops. Currently, the chips for which the driver does > the clock dance on suspend/resume seem to be the Broadcom STB SoCs, based > on ARM32 CPUs, as it seems... > > Found by Linux Verification Center (linuxtesting.org) with the Svace static > analysis tool. > > Fixes: 8bd954c56197 ("usb: host: xhci-plat: suspend and resume clocks") > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo... Looks good to me, adding to queue Thanks Mathias
Index: usb/drivers/usb/host/xhci-plat.c =================================================================== --- usb.orig/drivers/usb/host/xhci-plat.c +++ usb/drivers/usb/host/xhci-plat.c @@ -458,23 +458,38 @@ static int __maybe_unused xhci_plat_resu int ret; if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) { - clk_prepare_enable(xhci->clk); - clk_prepare_enable(xhci->reg_clk); + ret = clk_prepare_enable(xhci->clk); + if (ret) + return ret; + + ret = clk_prepare_enable(xhci->reg_clk); + if (ret) { + clk_disable_unprepare(xhci->clk); + return ret; + } } ret = xhci_priv_resume_quirk(hcd); if (ret) - return ret; + goto disable_clks; ret = xhci_resume(xhci, PMSG_RESUME); if (ret) - return ret; + goto disable_clks; pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); return 0; + +disable_clks: + if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) { + clk_disable_unprepare(xhci->clk); + clk_disable_unprepare(xhci->reg_clk); + } + + return ret; } static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
If this driver enables the xHC clocks while resuming from sleep, it calls clk_prepare_enable() without checking for errors and blithely goes on to read/write the xHC's registers -- which, with the xHC not being clocked, at least on ARM32 usually causes an imprecise external abort exceptions which cause kernel oops. Currently, the chips for which the driver does the clock dance on suspend/resume seem to be the Broadcom STB SoCs, based on ARM32 CPUs, as it seems... Found by Linux Verification Center (linuxtesting.org) with the Svace static analysis tool. Fixes: 8bd954c56197 ("usb: host: xhci-plat: suspend and resume clocks") Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo... Changes in version 5: - got rid of xhci_plat_disable_clocks() helper; - removed the passage on what's done by the patch from the patch description; - rebased the patch against the 'usb-next' branch. Changes in version 4: - resolved reject in xhci_plat_resume() due to the changed xhci_resume() call; - added the __maybe_unused attribute to xhci_plat_disable_clocks(). Changes in version 3: - sanitized the clock enabling error paths in xhci_plat_resume() WRT the applicability checks; - factored out the common clock disabling code from the suspend() and resume() driver PM methods; - added to the patch sescriptiun a passage describing the change being done. Changes in version 2: - fixed up the error path for clk_prepare_enable() calls in xhci_plat_resume(). drivers/usb/host/xhci-plat.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)