All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jack Pham <jackp@codeaurora.org>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Peter Chen <peter.chen@nxp.com>, dl-linux-imx <linux-imx@nxp.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Jun Li <jun.li@nxp.com>, "joel@jms.id.au" <joel@jms.id.au>,
	"mrana@codeaurora.org" <mrana@codeaurora.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: [RFT,2/2] xhci: handle port status events for removed USB3 hcd
Date: Fri, 28 Sep 2018 11:10:33 -0700	[thread overview]
Message-ID: <20180928181033.GC17520@jackp-linux.qualcomm.com> (raw)

Hi Mathias,

On Fri, Sep 28, 2018 at 03:35:10AM +0000, Peter Chen wrote:
>  
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Peter Chen <peter.chen@nxp.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   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 <jackp@codeaurora.org>

Will you be sending this as separate patches for -rc vs -stable?

Thanks,
Jack

WARNING: multiple messages have this Message-ID (diff)
From: Jack Pham <jackp@codeaurora.org>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Peter Chen <peter.chen@nxp.com>, dl-linux-imx <linux-imx@nxp.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Jun Li <jun.li@nxp.com>, "joel@jms.id.au" <joel@jms.id.au>,
	"mrana@codeaurora.org" <mrana@codeaurora.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd
Date: Fri, 28 Sep 2018 11:10:33 -0700	[thread overview]
Message-ID: <20180928181033.GC17520@jackp-linux.qualcomm.com> (raw)
In-Reply-To: <VI1PR04MB5327EA29B883F2E7CE80E3008BEC0@VI1PR04MB5327.eurprd04.prod.outlook.com>

Hi Mathias,

On Fri, Sep 28, 2018 at 03:35:10AM +0000, Peter Chen wrote:
>  
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Peter Chen <peter.chen@nxp.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   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 <jackp@codeaurora.org>

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

             reply	other threads:[~2018-09-28 18:10 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-28 18:10 Jack Pham [this message]
2018-09-28 18:10 ` [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd Jack Pham
  -- strict thread matches above, loose matches on Subject: below --
2018-10-01 15:52 [RFT,2/2] " Mathias Nyman
2018-10-01 15:52 ` [RFT PATCH 2/2] " Mathias Nyman
2018-09-28 18:12 [RFT,1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal Jack Pham
2018-09-28 18:12 ` [RFT PATCH 1/2] " Jack Pham
2018-09-28  3:35 [RFT,1/2] " Peter Chen
2018-09-28  3:35 ` [RFT PATCH 1/2] " Peter Chen
2018-09-28  3:35 [RFT,2/2] xhci: handle port status events for removed USB3 hcd Peter Chen
2018-09-28  3:35 ` [RFT PATCH 2/2] " Peter Chen
2018-09-28  2:16 [RFT,1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal Chunfeng Yun
2018-09-28  2:16 ` [RFT PATCH 1/2] " Chunfeng Yun
2018-09-27 16:34 [RFT,2/2] xhci: handle port status events for removed USB3 hcd Mathias Nyman
2018-09-27 16:34 ` [RFT PATCH 2/2] " Mathias Nyman
2018-09-27 16:26 [RFT,2/2] " Mathias Nyman
2018-09-27 16:26 ` [RFT PATCH 2/2] " Mathias Nyman
2018-09-27 16:26 [RFT,1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal Mathias Nyman
2018-09-27 16:26 ` [RFT PATCH 1/2] " Mathias Nyman
2018-09-27  1:39 [1/3] usb: host: xhci: fix oops when removing hcd Jack Pham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180928181033.GC17520@jackp-linux.qualcomm.com \
    --to=jackp@codeaurora.org \
    --cc=joel@jms.id.au \
    --cc=jun.li@nxp.com \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@linux.intel.com \
    --cc=mrana@codeaurora.org \
    --cc=peter.chen@nxp.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.