Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
On 28.09.2018 21:10, Jack Pham wrote: Hi Mathias, Jack, Peter, do these patches solve the remove issues you are seeing? At my two USB3 platforms, only apply the 1st patch can fix my problem. Maybe my USB3 port change interrupt occurs always before removing USB2 HCD. It's possible yes. Peter Ditto. I think the xhci_irq() is getting triggered by something during usb_remove_hcd() (usb_disconnect on the root hub?) but is able to complete before it returns. That is, the NULL pointer dereference is resolved yet I don't see that "ignore port event for removed USB3 hcd" message at all. Regardless, it's good to have here just in case, so Tested-by: Jack Pham Will you be sending this as separate patches for -rc vs -stable? Thanks, Jack Thanks, adding tested-by tags. I'll send them to -rc with stable tag, and then later send a backported version to older kernel once I have a upstream commit ID I can refer to. Thanks -Mathias
Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
Hi Mathias,
On Fri, Sep 28, 2018 at 03:35:10AM +, Peter Chen wrote:
>
> > > Cc:
> > > Reported-by: Peter Chen
> > > Signed-off-by: Mathias Nyman
> > > ---
> > > drivers/usb/host/xhci-ring.c | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/usb/host/xhci-ring.c
> > > b/drivers/usb/host/xhci-ring.c index f0a99aa..3d314b8 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd
> > > *xhci,
> > > goto cleanup;
> > > }
> > >
> > > + /* We might get interrupts after shared_hcd is removed */
> > > + if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
> > > + xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
> > > + bogus_port_status = true;
> > > + goto cleanup;
> > > + }
> > > +
> > > hcd = port->rhub->hcd;
> > > bus_state = &xhci->bus_state[hcd_index(hcd)];
> > > hcd_portnum = port->hcd_portnum;
> > >
> >
> > This probably only applies from 4.18 onwards, to test on older kernel try
> > something
> > like this instead:
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index
> > 6996235..7925da9 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
> > hcd = xhci_to_hcd(xhci);
> > if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
> > hcd = xhci->shared_hcd;
> > -
> > + if (!hcd) {
For testing on 4.14 I also added the debug print "ignore port event"
here as well. Maybe it should be there in the final -stable patch as
well.
> > + bogus_port_status = true;
> > + goto cleanup;
> > + }
> > if (major_revision == 0) {
> > xhci_warn(xhci, "Event for port %u not in "
> > "Extended Capabilities, ignoring.\n",
> >
> >
> > Jack, Peter, do these patches solve the remove issues you are seeing?
>
> At my two USB3 platforms, only apply the 1st patch can fix my problem. Maybe
> my USB3 port change interrupt occurs always before removing USB2 HCD.
>
> Peter
Ditto. I think the xhci_irq() is getting triggered by something during
usb_remove_hcd() (usb_disconnect on the root hub?) but is able to
complete before it returns. That is, the NULL pointer dereference is
resolved yet I don't see that "ignore port event for removed USB3 hcd"
message at all.
Regardless, it's good to have here just in case, so
Tested-by: Jack Pham
Will you be sending this as separate patches for -rc vs -stable?
Thanks,
Jack
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
RE: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
> > Cc:
> > Reported-by: Peter Chen
> > Signed-off-by: Mathias Nyman
> > ---
> > drivers/usb/host/xhci-ring.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c
> > b/drivers/usb/host/xhci-ring.c index f0a99aa..3d314b8 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
> > goto cleanup;
> > }
> >
> > + /* We might get interrupts after shared_hcd is removed */
> > + if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
> > + xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
> > + bogus_port_status = true;
> > + goto cleanup;
> > + }
> > +
> > hcd = port->rhub->hcd;
> > bus_state = &xhci->bus_state[hcd_index(hcd)];
> > hcd_portnum = port->hcd_portnum;
> >
>
> This probably only applies from 4.18 onwards, to test on older kernel try
> something
> like this instead:
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index
> 6996235..7925da9 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
> hcd = xhci_to_hcd(xhci);
> if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
> hcd = xhci->shared_hcd;
> -
> + if (!hcd) {
> + bogus_port_status = true;
> + goto cleanup;
> + }
> if (major_revision == 0) {
> xhci_warn(xhci, "Event for port %u not in "
> "Extended Capabilities, ignoring.\n",
>
>
> Jack, Peter, do these patches solve the remove issues you are seeing?
At my two USB3 platforms, only apply the 1st patch can fix my problem. Maybe
my USB3 port change interrupt occurs always before removing USB2 HCD.
Peter
Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
On 27.09.2018 19:26, Mathias Nyman wrote:
At xhci removal the USB3 hcd (shared_hcd) is removed before the primary
USB2 hcd. Interrupts for port status changes may still occur for USB3
ports after the shared_hcd is freed, causing NULL pointer dereference.
Check if xhci->shared_hcd is still valid before handing USB3 port events
Cc:
Reported-by: Peter Chen
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-ring.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f0a99aa..3d314b8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
goto cleanup;
}
+ /* We might get interrupts after shared_hcd is removed */
+ if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
+ xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
+ bogus_port_status = true;
+ goto cleanup;
+ }
+
hcd = port->rhub->hcd;
bus_state = &xhci->bus_state[hcd_index(hcd)];
hcd_portnum = port->hcd_portnum;
This probably only applies from 4.18 onwards, to test on older kernel try
something
like this instead:
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6996235..7925da9 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
hcd = xhci_to_hcd(xhci);
if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
hcd = xhci->shared_hcd;
-
+ if (!hcd) {
+ bogus_port_status = true;
+ goto cleanup;
+ }
if (major_revision == 0) {
xhci_warn(xhci, "Event for port %u not in "
"Extended Capabilities, ignoring.\n",
Jack, Peter, do these patches solve the remove issues you are seeing?
Thanks
-Mathias
[RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
At xhci removal the USB3 hcd (shared_hcd) is removed before the primary
USB2 hcd. Interrupts for port status changes may still occur for USB3
ports after the shared_hcd is freed, causing NULL pointer dereference.
Check if xhci->shared_hcd is still valid before handing USB3 port events
Cc:
Reported-by: Peter Chen
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-ring.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f0a99aa..3d314b8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
goto cleanup;
}
+ /* We might get interrupts after shared_hcd is removed */
+ if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
+ xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
+ bogus_port_status = true;
+ goto cleanup;
+ }
+
hcd = port->rhub->hcd;
bus_state = &xhci->bus_state[hcd_index(hcd)];
hcd_portnum = port->hcd_portnum;
--
2.7.4
