| Message ID | 7eff75da-bdd5-8ccd-1ad1-676f29041545@omp.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: host: xhci-plat: fix possible kernel oops while resuming | expand |
On 6/30/23 12:10 AM, 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-linus' branch of Greg KH's 'usb.git' repo... > > drivers/usb/host/xhci-plat.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Index: usb/drivers/usb/host/xhci-plat.c > =================================================================== > --- usb.orig/drivers/usb/host/xhci-plat.c > +++ usb/drivers/usb/host/xhci-plat.c > @@ -470,8 +470,14 @@ 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; > + } Scratch that, please! I didn't realize in time I have to disable/unprepare the clocks on the errors occuring below as well, not just return as before... :-/ > } > > ret = xhci_priv_resume_quirk(hcd); > MBR, Sergey
Index: usb/drivers/usb/host/xhci-plat.c =================================================================== --- usb.orig/drivers/usb/host/xhci-plat.c +++ usb/drivers/usb/host/xhci-plat.c @@ -470,8 +470,14 @@ 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 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-linus' branch of Greg KH's 'usb.git' repo... drivers/usb/host/xhci-plat.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)