From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753513AbcERPd0 (ORCPT ); Wed, 18 May 2016 11:33:26 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:38098 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753120AbcERPdY (ORCPT ); Wed, 18 May 2016 11:33:24 -0400 Subject: Re: [PATCH] usb: echi-hcd: Add register access check in shutdown To: Alan Stern References: Cc: Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, andy.gross@linaro.org, linux-arm-kernel@lists.infradead.org From: Srinivas Kandagatla Message-ID: <573C8B40.1040503@linaro.org> Date: Wed, 18 May 2016 16:33:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/05/16 15:56, Alan Stern wrote: > On Wed, 18 May 2016, Srinivas Kandagatla wrote: > >> This patch adds a check in ehci_shutdown(), to make sure >> that the register access is available before accessing registers. >> >> 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 and the >> host driver is probed, but the ehci_setup() has not been done yet, >> then a system shutdown would trigger below NULL pointer exception >> without this patch. > > How can that happen? While the host driver is probed, the probing > thread holds the device lock. But the system shutdown routine acquires > the device lock before invoking the ->shutdown callback. Therefore the > two things cannot happen concurrently. No, I did not mean them happening concurrently, I mean that the host driver is up, however ehci_setup() is not done yet. Will change the comments if its misleading the reader. > >> Unable to handle kernel NULL pointer dereference at virtual address >> 00000008 > > ... > >> --- a/drivers/usb/host/ehci-hcd.c >> +++ b/drivers/usb/host/ehci-hcd.c >> @@ -368,6 +368,9 @@ static void ehci_shutdown(struct usb_hcd *hcd) >> { >> struct ehci_hcd *ehci = hcd_to_ehci(hcd); >> >> + if (!HCD_HW_ACCESSIBLE(hcd)) >> + return; >> + >> spin_lock_irq(&ehci->lock); >> ehci->shutdown = true; >> ehci->rh_state = EHCI_RH_STOPPING; > > This doesn't seem like the right place. What you really should do is > skip calling ehci_silence_controller() if the hardware isn't > accessible. That's where the hardware gets touched, not in > ehci_shutdown(). Yep , that should work as well. Will send a v2 patch. thanks, srini > > Alan Stern >