lkml.org 
[lkml]   [2023]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH -next] usb: xhci: do not free an empty cmd ring
Date
Hi greg,

在 2023/3/23 03:29, Greg KH 写道:
> On Mon, Mar 20, 2023 at 12:22:23PM +0800, Hongyu Xie wrote:
>> It was first found on HUAWEI Kirin 9006C platform with a builtin xhci
>> controller during stress cycle test(stress-ng, glmark2, x11perf, S4...).
>>
>> phase one:
>> [26788.706878] PM: dpm_run_callback(): platform_pm_thaw+0x0/0x68 returns -12
>> [26788.706878] PM: Device xhci-hcd.1.auto failed to thaw async: error -12
>> ...
>> phase two:
>> [28650.583496] [2023:01:19 04:43:29]Unable to handle kernel NULL pointer dereference at virtual address 0000000000000028
>> ...
>> [28650.583526] user pgtable: 4k pages, 39-bit VAs, pgdp=000000027862a000
>> [28650.583557] [0000000000000028] pgd=0000000000000000
>> ...
>> [28650.583587] pc : xhci_suspend+0x154/0x5b0
>> [28650.583618] lr : xhci_suspend+0x148/0x5b0
>> [28650.583618] sp : ffffffc01c7ebbd0
>> [28650.583618] x29: ffffffc01c7ebbd0 x28: ffffffec834d0000
>> [28650.583618] x27: ffffffc0106a3cc8 x26: ffffffb2c540c848
>> [28650.583618] x25: 0000000000000000 x24: ffffffec82ee30b0
>> [28650.583618] x23: ffffffb43b31c2f8 x22: 0000000000000000
>> [28650.583618] x21: 0000000000000000 x20: ffffffb43b31c000
>> [28650.583648] x19: ffffffb43b31c2a8 x18: 0000000000000001
>> [28650.583648] x17: 0000000000000803 x16: 00000000fffffffe
>> [28650.583648] x15: 0000000000001000 x14: ffffffb150b67e00
>> [28650.583648] x13: 00000000f0000000 x12: 0000000000000001
>> [28650.583648] x11: 0000000000000000 x10: 0000000000000a80
>> [28650.583648] x9 : ffffffc01c7eba00 x8 : ffffffb43ad10ae0
>> [28650.583648] x7 : ffffffb84cd98dc0 x6 : 0000000cceb6a101
>> [28650.583679] x5 : 00ffffffffffffff x4 : 0000000000000001
>> [28650.583679] x3 : 0000000000000011 x2 : 0000000000e2cfa8
>> [28650.583679] x1 : 00000000823535e1 x0 : 0000000000000000
>>
>> gdb:
>> (gdb) l *(xhci_suspend+0x154)
>> 0xffffffc010b6cd44 is in xhci_suspend (/.../drivers/usb/host/xhci.c:854).
>> 849 {
>> 850 struct xhci_ring *ring;
>> 851 struct xhci_segment *seg;
>> 852
>> 853 ring = xhci->cmd_ring;
>> 854 seg = ring->deq_seg;
>> (gdb) disassemble 0xffffffc010b6cd44
>> ...
>> 0xffffffc010b6cd40 <+336>: ldr x22, [x19, #160]
>> 0xffffffc010b6cd44 <+340>: ldr x20, [x22, #40]
>> 0xffffffc010b6cd48 <+344>: mov w1, #0x0 // #0
>>
>> During phase one, platform_pm_thaw called xhci_plat_resume which called
>> xhci_resume. The rest possible calling routine might be
>> xhci_resume->xhci_init->xhci_mem_init, and xhci->cmd_ring was cleaned in
>> xhci_mem_cleanup before xhci_mem_init returned -ENOMEM.
>>
>> During phase two, systemd was tring to hibernate again and called
>> xhci_suspend, then xhci_clear_command_ring dereferenced xhci->cmd_ring
>> which was already NULL.
>>
>> So if xhci->cmd_ring is NULL, xhci_clear_command_ring just return.
>>
>> Co-developed-by: sunke <sunke@kylinos.cn>
>> Signed-off-by: sunke <sunke@kylinos.cn>
>> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
>> ---
>> drivers/usb/host/xhci.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 6183ce8574b1..8b79ad2955e5 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -919,6 +919,10 @@ static void xhci_clear_command_ring(struct xhci_hcd *xhci)
>> {
>> struct xhci_ring *ring;
>> struct xhci_segment *seg;
>> + if (!xhci->cmd_ring) {
>> + xhci_err(xhci, "Empty cmd ring");
>
> Why is this being told to userspace? What can it do about it?
Can do nothing about it. But it's always good to know what was wrong.
>
>> + return;
>
> Not returning an error?
Return -EINVAL to skip the rest of xhci_suspend? It might break kabi.
>
>> + }
>>
>
> Please always use checkpatch.pl when sending patches out so that
> maintainers have to ask you to use checkpatch.pl.
I did, no warning and no error.
>
> Also, what commit does this fix? Does this need to go to stable trees?
It would be 898213200cba. Yes. I'll fix it in v2.
>
> But wait, this feels wrong, what can keep this variable from being set
> to NULL right after you check it? Where it the lock involved?
xhci_clear_command_ring only exits in xhci_suspend. cmd_ring only sets
to NULL in xhci_mem_cleanup, which called by xhci_mem_init, xhci_stop,
xhci_resume.
>
> thanks,
>
> greg k-h
thanks,

Hongyu Xie

\
 
 \ /
  Last update: 2023-03-27 01:13    [W:0.062 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site