| Message ID | 1463652776-30456-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 19 May 2016, Srinivas Kandagatla wrote: > This patch protects system from crashing at shutdown in > cases where usb host is not added yet from OTG controller driver. > As ehci_setup() not done yet, so stop accessing registers or > variables initialized as part of ehci_setup(). > > The use case is simple, for boards like DB410c where the usb host > or device functionality is decided based on the micro-usb cable > presence. If the board boots up with micro-usb connected, the > OTG driver like echi-msm would not add the usb host by default. > However a system shutdown would go and access registers and > uninitialized variables, resulting in below crash. ... > Fixes 4bb3cad7125b ("usb: host: ehci-msm: Register usb shutdown function") > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/usb/host/ehci-hcd.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index ae1b6e6..a962b89 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) > { > struct ehci_hcd *ehci = hcd_to_ehci(hcd); > > + /** > + * Protect the system from crashing at system shutdown in cases where > + * usb host is not added yet from OTG controller driver. > + * As ehci_setup() not done yet, so stop accessing registers or > + * variables initialized in ehci_setup() > + */ > + if (!ehci->sbrn) > + return; > + > spin_lock_irq(&ehci->lock); > ehci->shutdown = true; > ehci->rh_state = EHCI_RH_STOPPING; Acked-by: Alan Stern <stern@rowland.harvard.edu>
On 19 May 2016 at 05:12, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > This patch protects system from crashing at shutdown in > cases where usb host is not added yet from OTG controller driver. > As ehci_setup() not done yet, so stop accessing registers or > variables initialized as part of ehci_setup(). > > The use case is simple, for boards like DB410c where the usb host > or device functionality is decided based on the micro-usb cable > presence. If the board boots up with micro-usb connected, the > OTG driver like echi-msm would not add the usb host by default. > However a system shutdown would go and access registers and > uninitialized variables, resulting in below crash. Works great. Tested-by: Andy Gross <andy.gross@linaro.org>
On 19 May 2016 at 05:12, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: <snip> > +++ b/drivers/usb/host/ehci-hcd.c > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) > { > struct ehci_hcd *ehci = hcd_to_ehci(hcd); > > + /** > + * Protect the system from crashing at system shutdown in cases where > + * usb host is not added yet from OTG controller driver. > + * As ehci_setup() not done yet, so stop accessing registers or > + * variables initialized in ehci_setup() > + */ > + if (!ehci->sbrn) > + return; > + > spin_lock_irq(&ehci->lock); > ehci->shutdown = true; > ehci->rh_state = EHCI_RH_STOPPING; Should we also not check this in ehci_suspend and ehci_resume? If I do suspend/resume, it crashes in a similar manner. I know this goes beyond the problem scope of the patch. Perhaps I should send in an additional patch with the code? Are there any other places in the ehci-hcd where this needs to be checked? Regards, Andy
On Thu, 19 May 2016, Andy Gross wrote: > On 19 May 2016 at 05:12, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: > > <snip> > > > +++ b/drivers/usb/host/ehci-hcd.c > > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) > > { > > struct ehci_hcd *ehci = hcd_to_ehci(hcd); > > > > + /** > > + * Protect the system from crashing at system shutdown in cases where > > + * usb host is not added yet from OTG controller driver. > > + * As ehci_setup() not done yet, so stop accessing registers or > > + * variables initialized in ehci_setup() > > + */ > > + if (!ehci->sbrn) > > + return; > > + > > spin_lock_irq(&ehci->lock); > > ehci->shutdown = true; > > ehci->rh_state = EHCI_RH_STOPPING; > > > Should we also not check this in ehci_suspend and ehci_resume? If I > do suspend/resume, it crashes in a similar manner. I know this goes > beyond the problem scope of the patch. Perhaps I should send in an > additional patch with the code? > > Are there any other places in the ehci-hcd where this needs to be checked? Really, this is an indication that the problem lies not in ehci-hcd but rather in the platform glue code. The platform code allows itself to be registered as the driver of the controller device but does not initialize the ehci-hcd parts. Neither ehci_suspend nor ehci_resume is called directly by the driver core; the calls all have to pass through the glue code. So maybe the glue layer needs to be careful about invoking these routines in ehci-hcd before ehci-hcd has been initialized. Alan Stern
On 20 May 2016 at 09:31, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 19 May 2016, Andy Gross wrote: > >> On 19 May 2016 at 05:12, Srinivas Kandagatla >> <srinivas.kandagatla@linaro.org> wrote: >> >> <snip> >> >> > +++ b/drivers/usb/host/ehci-hcd.c >> > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) >> > { >> > struct ehci_hcd *ehci = hcd_to_ehci(hcd); >> > >> > + /** >> > + * Protect the system from crashing at system shutdown in cases where >> > + * usb host is not added yet from OTG controller driver. >> > + * As ehci_setup() not done yet, so stop accessing registers or >> > + * variables initialized in ehci_setup() >> > + */ >> > + if (!ehci->sbrn) >> > + return; >> > + >> > spin_lock_irq(&ehci->lock); >> > ehci->shutdown = true; >> > ehci->rh_state = EHCI_RH_STOPPING; >> >> >> Should we also not check this in ehci_suspend and ehci_resume? If I >> do suspend/resume, it crashes in a similar manner. I know this goes >> beyond the problem scope of the patch. Perhaps I should send in an >> additional patch with the code? >> >> Are there any other places in the ehci-hcd where this needs to be checked? > > Really, this is an indication that the problem lies not in ehci-hcd but > rather in the platform glue code. The platform code allows itself to > be registered as the driver of the controller device but does not > initialize the ehci-hcd parts. > > Neither ehci_suspend nor ehci_resume is called directly by the driver > core; the calls all have to pass through the glue code. So maybe the > glue layer needs to be careful about invoking these routines in > ehci-hcd before ehci-hcd has been initialized. That's a good point. Perhaps all of this should be in the glue, including the shutdown case. Regards, Andy
On Fri, 20 May 2016, Andy Gross wrote: > On 20 May 2016 at 09:31, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 19 May 2016, Andy Gross wrote: > > > >> On 19 May 2016 at 05:12, Srinivas Kandagatla > >> <srinivas.kandagatla@linaro.org> wrote: > >> > >> <snip> > >> > >> > +++ b/drivers/usb/host/ehci-hcd.c > >> > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) > >> > { > >> > struct ehci_hcd *ehci = hcd_to_ehci(hcd); > >> > > >> > + /** > >> > + * Protect the system from crashing at system shutdown in cases where > >> > + * usb host is not added yet from OTG controller driver. > >> > + * As ehci_setup() not done yet, so stop accessing registers or > >> > + * variables initialized in ehci_setup() > >> > + */ > >> > + if (!ehci->sbrn) > >> > + return; > >> > + > >> > spin_lock_irq(&ehci->lock); > >> > ehci->shutdown = true; > >> > ehci->rh_state = EHCI_RH_STOPPING; > >> > >> > >> Should we also not check this in ehci_suspend and ehci_resume? If I > >> do suspend/resume, it crashes in a similar manner. I know this goes > >> beyond the problem scope of the patch. Perhaps I should send in an > >> additional patch with the code? > >> > >> Are there any other places in the ehci-hcd where this needs to be checked? > > > > Really, this is an indication that the problem lies not in ehci-hcd but > > rather in the platform glue code. The platform code allows itself to > > be registered as the driver of the controller device but does not > > initialize the ehci-hcd parts. > > > > Neither ehci_suspend nor ehci_resume is called directly by the driver > > core; the calls all have to pass through the glue code. So maybe the > > glue layer needs to be careful about invoking these routines in > > ehci-hcd before ehci-hcd has been initialized. > > That's a good point. Perhaps all of this should be in the glue, > including the shutdown case. Many of the platform glue files set usb_hcd_platform_shutdown() as their shutdown callback, which makes it impossible for them to include a controller-specific test. Alan Stern
On 20 May 2016 at 10:57, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 20 May 2016, Andy Gross wrote: > >> On 20 May 2016 at 09:31, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Thu, 19 May 2016, Andy Gross wrote: >> > >> >> On 19 May 2016 at 05:12, Srinivas Kandagatla >> >> <srinivas.kandagatla@linaro.org> wrote: >> >> >> >> <snip> >> >> >> >> > +++ b/drivers/usb/host/ehci-hcd.c >> >> > @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) >> >> > { >> >> > struct ehci_hcd *ehci = hcd_to_ehci(hcd); >> >> > >> >> > + /** >> >> > + * Protect the system from crashing at system shutdown in cases where >> >> > + * usb host is not added yet from OTG controller driver. >> >> > + * As ehci_setup() not done yet, so stop accessing registers or >> >> > + * variables initialized in ehci_setup() >> >> > + */ >> >> > + if (!ehci->sbrn) >> >> > + return; >> >> > + >> >> > spin_lock_irq(&ehci->lock); >> >> > ehci->shutdown = true; >> >> > ehci->rh_state = EHCI_RH_STOPPING; >> >> >> >> >> >> Should we also not check this in ehci_suspend and ehci_resume? If I >> >> do suspend/resume, it crashes in a similar manner. I know this goes >> >> beyond the problem scope of the patch. Perhaps I should send in an >> >> additional patch with the code? >> >> >> >> Are there any other places in the ehci-hcd where this needs to be checked? >> > >> > Really, this is an indication that the problem lies not in ehci-hcd but >> > rather in the platform glue code. The platform code allows itself to >> > be registered as the driver of the controller device but does not >> > initialize the ehci-hcd parts. >> > >> > Neither ehci_suspend nor ehci_resume is called directly by the driver >> > core; the calls all have to pass through the glue code. So maybe the >> > glue layer needs to be careful about invoking these routines in >> > ehci-hcd before ehci-hcd has been initialized. >> >> That's a good point. Perhaps all of this should be in the glue, >> including the shutdown case. > > Many of the platform glue files set usb_hcd_platform_shutdown() as > their shutdown callback, which makes it impossible for them to include > a controller-specific test. Yes, I just found this out for myself. So in that specific case, it makes sense to have the check within the ehci-hcd. Whereas in the glue driver, the suspend/resume use direct calls to ehci_suspend/resume. I'll send along a ehci-msm patch to address the issues i see with suspend/resume. Thanks! Andy
On 19 May 2016 at 15:42, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: <snip> > Fixes 4bb3cad7125b ("usb: host: ehci-msm: Register usb shutdown function") > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Was seeing this crash while doing a reboot on db410c which is fixed with this patch: Tested-by: Pramod Gurav <pramod.gurav@linaro.org>
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index ae1b6e6..a962b89 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -368,6 +368,15 @@ static void ehci_shutdown(struct usb_hcd *hcd) { struct ehci_hcd *ehci = hcd_to_ehci(hcd); + /** + * Protect the system from crashing at system shutdown in cases where + * usb host is not added yet from OTG controller driver. + * As ehci_setup() not done yet, so stop accessing registers or + * variables initialized in ehci_setup() + */ + if (!ehci->sbrn) + return; + spin_lock_irq(&ehci->lock); ehci->shutdown = true; ehci->rh_state = EHCI_RH_STOPPING;
This patch protects system from crashing at shutdown in cases where usb host is not added yet from OTG controller driver. As ehci_setup() not done yet, so stop accessing registers or variables initialized as part of ehci_setup(). The use case is simple, for boards like DB410c where the usb host or device functionality is decided based on the micro-usb cable presence. If the board boots up with micro-usb connected, the OTG driver like echi-msm would not add the usb host by default. However a system shutdown would go and access registers and uninitialized variables, resulting in below crash. Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = ffffffc034581000 [00000008] *pgd=0000000000000000, *pud=0000000000000000 CPU: 2 PID: 1957 Comm: reboot Not tainted 4.6.0+ #99 task: ffffffc034bc0000 ti: ffffffc0345cc000 task.ti: ffffffc0345cc000 PC is at ehci_halt+0x54/0x108 LR is at ehci_halt+0x38/0x108 pc : [<ffffff800869837c>] lr : [<ffffff8008698360>] pstate: a00001c5 sp : ffffffc0345cfc60 x29: ffffffc0345cfc60 x28: ffffffc0345cc000 x27: ffffff8008a4d000 x26: 000000000000008e x25: ffffff8008d86cb0 x24: ffffff800908b040 x23: ffffffc036068870 x22: ffffff8009d0a000 x21: ffffffc03512a410 x20: ffffffc03512a410 x19: ffffffc03512a338 x18: 00000000000065ba x17: ffffff8009b16b80 x16: 0000000000000003 x15: 00000000000065b9 x14: 00000000000065b6 x13: 0000000000000000 x12: 0000000000000000 x11: 000000000000003d x10: ffffffc0345cf9e0 x9 : 0000000000000001 x8 : ffffffc0345cc000 x7 : ffffff8008698360 x6 : 0000000000000000 x5 : 0000000000000080 x4 : 0000000000000001 x3 : 0000000000000000 x2 : 0000000000000000 x1 : 0000000000000008 x0 : ffffffc034bc0000 Process reboot (pid: 1957, stack limit = 0xffffffc0345cc020) Stack: (0xffffffc0345cfc60 to 0xffffffc0345d0000) fc60: ffffffc0345cfc90 ffffff8008698448 ffffffc03512a338 ffffffc03512a338 fc80: ffffffc03512a410 ffffff8008a3bbfc ffffffc0345cfcc0 ffffff8008698548 fca0: ffffffc03512a338 ffffffc03512a000 ffffffc03512a410 ffffff8009d0a000 fcc0: ffffffc0345cfcf0 ffffff800865d2bc ffffffc036068828 ffffffc036068810 fce0: ffffffc036003810 ffffff800853f43c ffffffc0345cfd00 ffffff800854338c fd00: ffffffc0345cfd10 ffffff800853f45c ffffffc0345cfd60 ffffff80080e0f48 fd20: 0000000000000000 0000000001234567 ffffff8008f8c000 ffffff8008f8c060 fd40: 0000000000000000 0000000000000015 0000000000000120 ffffff80080e0f30 fd60: ffffffc0345cfd70 ffffff80080e1020 ffffffc0345cfd90 ffffff80080e12fc fd80: 0000000000000000 0000000001234567 0000000000000000 ffffff8008085e70 fda0: 0000000000000000 0000005592905000 ffffffffffffffff 0000007f79daf1cc fdc0: 0000000000000000 0000000000000000 0000007ffcbb1198 000000000000000a fde0: 00000055928d3f58 0000000000000001 ffffffc034900000 00000000fffffffe fe00: ffffffc034900000 0000007f79da902c ffffffc0345cfe40 ffffff800820af38 fe20: 0000000000000000 0000007ffcbb1078 ffffffffffffffff ffffff80081e9b38 fe40: ffffffc0345cfe60 ffffff80081eb410 ffffffc0345cfe60 ffffff80081eb444 fe60: ffffffc0345cfec0 ffffff80081ec4f4 0000000000000000 0000007ffcbb1078 fe80: ffffffffffffffff 0000000000000015 ffffffc0345cfec0 0000007ffcbb1078 fea0: 0000000000000002 000000000000000a ffffffffffffffff 0000000000000000 fec0: 0000000000000000 ffffff8008085e70 fffffffffee1dead 0000000028121969 fee0: 0000000001234567 0000000000000000 ffffffffffffffff 8080800000800000 ff00: 0000800000808080 0000007ffcbb10f0 000000000000008e fefeff54918cb8c7 ff20: 7f7f7f7fffffffff 0101010101010101 0000000000000010 0000000000000000 ff40: 0000000000000000 0000007f79e33588 0000005592905eb8 0000007f79daf1b0 ff60: 0000007ffcbb1340 0000005592906000 0000005592905000 0000005592906000 ff80: 0000005592907000 0000000000000002 0000007ffcbb1d98 0000005592906000 ffa0: 00000055928d2000 0000000000000000 0000000000000000 0000007ffcbb1aa0 ffc0: 00000055928b819c 0000007ffcbb1aa0 0000007f79daf1cc 0000000000000000 ffe0: fffffffffee1dead 000000000000008e 05ef555057155555 d555544d55d775d3 Call trace: Exception stack(0xffffffc0345cfaa0 to 0xffffffc0345cfbc0) Set corner to 6 faa0: ffffffc03512a338 ffffffc03512a410 ffffffc0345cfc60 ffffff800869837c fac0: ffffff8008114210 0000000100000001 ffffff8009ce1b20 ffffff8009ce5f20 fae0: ffffffc0345cfb80 ffffff80081145a8 ffffffc0345cfc10 ffffff800810b924 fb00: ffffffc0345cc000 00000000000001c0 ffffffc03512a410 ffffff8009d0a000 fb20: ffffffc036068870 ffffff800908b040 ffffff8008d86cb0 000000000000008e fb40: ffffffc034bc0000 0000000000000008 0000000000000000 0000000000000000 fb60: 0000000000000001 0000000000000080 0000000000000000 ffffff8008698360 fb80: ffffffc0345cc000 0000000000000001 ffffffc0345cf9e0 000000000000003d fba0: 0000000000000000 0000000000000000 00000000000065b6 00000000000065b9 [<ffffff800869837c>] ehci_halt+0x54/0x108 [<ffffff8008698448>] ehci_silence_controller+0x18/0xcc [<ffffff8008698548>] ehci_shutdown+0x4c/0x64 [<ffffff800865d2bc>] usb_hcd_platform_shutdown+0x1c/0x24 [<ffffff800854338c>] platform_drv_shutdown+0x20/0x28 [<ffffff800853f45c>] device_shutdown+0xf4/0x1b0 [<ffffff80080e0f48>] kernel_restart_prepare+0x34/0x3c [<ffffff80080e1020>] kernel_restart+0x14/0x74 [<ffffff80080e12fc>] SyS_reboot+0x110/0x21c [<ffffff8008085e70>] el0_svc_naked+0x24/0x28 Code: 53001c42 350000a2 d5033e9f 91002021 (b9000022) Fixes 4bb3cad7125b ("usb: host: ehci-msm: Register usb shutdown function") Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/usb/host/ehci-hcd.c | 9 +++++++++ 1 file changed, 9 insertions(+)