On 9/11/23 3:53 PM, Mathias Nyman 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...
>>
>> In order to fix this issue, add the result checks for clk_prepare_enable()
>> calls in xhci_plat_resume(), add conditional clk_disable_unprepare() calls
>> on the error path of xhci_plat_resume(); then factor out the common clock
>> disabling code from the suspend() and resume() driver PM methods into a
>> separate function to avoid code duplication.
> Minor nitpick, but not sure a separate function is helpful here.
> It's two lines of code called twice.
Tried to save on the object code size...
>> 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>
>
> If I understood correctly this issue hasn't been seen in real life,
Not yet?
> and this patch only changes how we fail?
Yes, failing gracefully instead of a kernel oops...
> So I guess this would be more suitable for usb-next than usb-linus.
Maybe. I'll recast and rebase...
[...]
> Thanks
> Mathias
MBR, Sergey
