On 6/30/23 11:48 PM, 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@xxxxxx>
>
> ---
> This patch is against the 'usb-linus' branch of Greg KH's 'usb.git' repo...
>
> Changes in version 2:
> - fixed up the error path for clk_prepare_enable() calls in xhci_plat_resume().
Scratch that, still not correct enough...
> drivers/usb/host/xhci-plat.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 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,23 +470,36 @@ 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)
> + goto disable_clk;
> }
>
> ret = xhci_priv_resume_quirk(hcd);
> if (ret)
> - return ret;
> + goto disable_reg_clk;
>
> ret = xhci_resume(xhci, 0);
> if (ret)
> - return ret;
> + goto disable_reg_clk;
>
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> return 0;
> +
> +disable_reg_clk:
> + clk_disable_unprepare(xhci->reg_clk);
> +
> +disable_clk:
> + clk_disable_unprepare(xhci->clk);
> +
> + return ret;
> }
[...]
Grr, the error path is still incorrect here -- it should've checked
xhci->quirks/etc. before calling the clock disable/unprepare... :-(
MBR, Sergey
