lkml.org 
[lkml]   [2018]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/2] usb: xhci: fix uninitialized completion when USB3 port got wrong status
    From
    Date


    On 10/22/18 9:12 PM, Mathias Nyman wrote:
    > On 21.10.2018 20:08, Aaron Ma wrote:
    >> Realtek USB3.0 Card Reader [0bda:0328] reports wrong port status on
    >> Cannon lake PCH USB3.1 xHCI [8086:a36d] after resume from S3,
    >> after clear port reset it works fine.
    >>
    >> Since this device is registered on USB3 roothub at boot,
    >> when port status reports not superspeed, xhci_get_port_status will call
    >> an uninitialized completion in bus_state[0].
    >> Kernel will hang because of NULL pointer.
    >>
    >> Restrict the USB2 resume status check in USB2 roothub to fix hang issue.
    >> No harm to initialize USB3 bus_state[0] in case it is called.
    >>
    >> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
    >> ---
    >>   drivers/usb/host/xhci-hub.c  | 2 +-
    >>   drivers/usb/host/xhci-mem.c  | 1 +
    >>   drivers/usb/host/xhci-ring.c | 2 +-
    >>   3 files changed, 3 insertions(+), 2 deletions(-)
    >>
    >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
    >> index 7e2a531ba321..d30ca6ceffc9 100644
    >> --- a/drivers/usb/host/xhci-hub.c
    >> +++ b/drivers/usb/host/xhci-hub.c
    >> @@ -876,7 +876,7 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
    >>               status |= USB_PORT_STAT_SUSPEND;
    >>       }
    >>       if ((raw_port_status & PORT_PLS_MASK) == XDEV_RESUME &&
    >> -        !DEV_SUPERSPEED_ANY(raw_port_status)) {
    >> +        !DEV_SUPERSPEED_ANY(raw_port_status) && 1 == hcd_index(hcd)) {
    >>           if ((raw_port_status & PORT_RESET) ||
    >>                   !(raw_port_status & PORT_PE))
    >>               return 0xffffffff;
    >
    > Nice catch.
    >
    > Maybe use "hcd->speed < HCD_USB3" instead of "1 == hcd_index(hcd)"
    > It's easier to understand.
    >
    > Turns out this isn't an issue with your Realtek device, it just happens
    > to trigger
    > a driver issue.
    >
    > The original !DEV_SUPERSPEED_ANY() check was not suitable here.
    > It checks the port-speed field of portsc register (bits 13:10), which
    > are only valid for USB3
    > ports if all link training is done and port reached its "enabled" state.
    > Otherwise it will return 0, and USB3 ports may be mistaken for USB2 ports.

    PORT_ENABLE should be already set to one.
    The same device ID card reader doesn't have issue on Sunrise Point.
    Maybe it is related to Cannon lake PCH USB controller?

    >
    > Just to make sure, Does your device stay as a USB 3 device, it's never
    > enumerated as USB2, right?
    >

    Right, always USB3.

    > I'm in the middle of refactoring the get_port_status(), it should solve
    > this
    > as well, but we need your solution stable releases.
    >

    V2 sent out. Cc-ed stable.

    > Any chance you to check if the refactored code works with the Realtek
    > device?
    > I just created a "get_port_status_refactor" branch for it:
    >
    > git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
    > get_port_status_refactor

    The hang issue is not reproduced on this kernel branch.

    Thanks,
    Aaron

    >
    >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
    >> index b1f27aa38b10..dd2ad50c5289 100644
    >> --- a/drivers/usb/host/xhci-mem.c
    >> +++ b/drivers/usb/host/xhci-mem.c
    >> @@ -2539,6 +2539,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t
    >> flags)
    >>           xhci->bus_state[0].resume_done[i] = 0;
    >>           xhci->bus_state[1].resume_done[i] = 0;
    >>           /* Only the USB 2.0 completions will ever be used. */
    >> +        init_completion(&xhci->bus_state[0].rexit_done[i]);
    >>           init_completion(&xhci->bus_state[1].rexit_done[i]);
    >>       }
    >
    > I don't think we should init the completion unnecessary for USB3 ports.
    >
    >>   diff --git a/drivers/usb/host/xhci-ring.c
    >> b/drivers/usb/host/xhci-ring.c
    >> index f0a99aa0ac58..894d4625b8b9 100644
    >> --- a/drivers/usb/host/xhci-ring.c
    >> +++ b/drivers/usb/host/xhci-ring.c
    >> @@ -1634,7 +1634,7 @@ static void handle_port_status(struct xhci_hcd
    >> *xhci,
    >>        * RExit to a disconnect state).  If so, let the the driver know
    >> it's
    >>        * out of the RExit state.
    >>        */
    >> -    if (!DEV_SUPERSPEED_ANY(portsc) &&
    >> +    if (!DEV_SUPERSPEED_ANY(portsc) && 1 == hcd_index(hcd) &&
    >>               test_and_clear_bit(hcd_portnum,
    >>                   &bus_state->rexit_ports)) {
    >>           complete(&bus_state->rexit_done[hcd_portnum]);
    >>
    >
    > Same here, prefer hcd->speed < HCD_USB3
    >
    > Thanks
    > Mathias

    \
     
     \ /
      Last update: 2018-10-22 19:54    [W:2.946 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site