Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756475AbbLAOcS (ORCPT ); Tue, 1 Dec 2015 09:32:18 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:44914 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755288AbbLAOcQ (ORCPT ); Tue, 1 Dec 2015 09:32:16 -0500 From: Felipe Balbi To: Mathias Nyman , CC: , , , Mathias Nyman Subject: Re: [TESTPATCH v2] xhci: fix usb2 resume timing and races. In-Reply-To: <1448958418-6114-1-git-send-email-mathias.nyman@linux.intel.com> References: <1448958418-6114-1-git-send-email-mathias.nyman@linux.intel.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 1 Dec 2015 08:32:08 -0600 Message-ID: <87d1uq1bpz.fsf@saruman.tx.rr.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7521 Lines: 198 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Mathias Nyman writes: > usb2 ports need to signal resume for 20ms before moving to U0 state. at least 20ms ;-) Recently, we decided to drive resume for 40ms to support devices with broken FW. > Both device and host can initiate resume. > > On host initated resume port is set to resume state, sleep 20ms, sleep 40ms: include/linux/usb.h:232:#define USB_RESUME_TIMEOUT 40 /* ms */ > and finally set port to U0 state. > > On device initated resume a port status interrupt with a port in resume > state in issued. The interrupt handler tags a resume_done[port] > timestamp with current time + 20ms, and kick roothub timer. + 40ms ? > Root hub timer requests for port status, finds the port in resume state, > checks if resume_done[port] timestamp passed, and set port to U0 state. > > There are a few issues with this approach, > 1. A host initated resume will also generate a resume event, the event > handler will find the port in resume state, believe it's a device > initated and act accordingly. > > 2. A port status request might cut the 20ms resume signalling short if a > get_port_status request is handled during the 20ms host resume. > The port will be found in resume state. The timestamp is not set leadi= ng > to time_after_eq(jiffoes, timestamp) returning true, as timestamp =3D = 0. > get_port_status will proceed with moving the port to U0. > > 3. If an error, or anything else happends to the port during device > initated 20ms resume signalling it will leave all device resume > parameters hanging uncleared preventing further resume. > > Fix this by using the existing resuming_ports bitfield to indicate if > resume signalling timing is taken care of. > Also check if the resume_done[port] is set before using it in time > comparison. Also clear out any resume signalling related variables if port > is not in U0 or Resume state. > > v2. fix parentheses when checking for uncleared resume variables. > we want: if ((unclear1 OR unclear2 ) AND !in_resume AND !in_U3) { .. } this 'v2' note doesn't have to go into commit log, IMO. > Signed-off-by: Mathias Nyman > --- > drivers/usb/host/xhci-hub.c | 38 ++++++++++++++++++++++++++++++++++++-- > drivers/usb/host/xhci-ring.c | 3 ++- > 2 files changed, 38 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index 78241b5..a750298 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -616,8 +616,30 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd, > if ((raw_port_status & PORT_RESET) || > !(raw_port_status & PORT_PE)) > return 0xffffffff; > - if (time_after_eq(jiffies, > - bus_state->resume_done[wIndex])) { > + /* did port event handler already start 20ms resume timing? */ > + if (!bus_state->resume_done[wIndex]) { > + /* If not, maybe we are in a host initated resume? */ > + if (test_bit(wIndex, &bus_state->resuming_ports)) { > + /* Host initated resume doesn't time the resume > + * signalling using resume_done[]. > + * It manually sets RESUME state, sleeps 20ms > + * and sets U0 state. This should probably be > + * changed, but not right now, do nothing > + */ > + } else { > + /* port resume was discovered now and here, > + * start resume timing > + */ > + unsigned long timeout =3D jiffies + > + msecs_to_jiffies(USB_RESUME_TIMEOUT); > + > + set_bit(wIndex, &bus_state->resuming_ports); > + bus_state->resume_done[wIndex] =3D timeout; > + mod_timer(&hcd->rh_timer, timeout); > + } > + /* Has resume been signalled for 20ms? yet? */ How about: "Has resume been signalled for at least 20ms?" Or even better: Has resume been signalled for at least USB_RESUME_TIMEOUT ms yet ? > + } else if (time_after_eq(jiffies, > + bus_state->resume_done[wIndex])) { > int time_left; >=20=20 > xhci_dbg(xhci, "Resume USB2 port %d\n", > @@ -665,6 +687,16 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd, > status |=3D USB_PORT_STAT_SUSPEND; > } > } > + /* Clear stale usb2 resume signalling variables in case port changed > + * state during 20ms resume signalling. For example on error > + */ > + if ((bus_state->resume_done[wIndex] || > + test_bit(wIndex, &bus_state->resuming_ports)) && > + (raw_port_status & PORT_PLS_MASK) !=3D XDEV_U3 && > + (raw_port_status & PORT_PLS_MASK) !=3D XDEV_RESUME) { > + bus_state->resume_done[wIndex] =3D 0; > + clear_bit(wIndex, &bus_state->resuming_ports); > + } > if ((raw_port_status & PORT_PLS_MASK) =3D=3D XDEV_U0 > && (raw_port_status & PORT_POWER) > && (bus_state->suspended_ports & (1 << wIndex))) { > @@ -995,6 +1027,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeRe= q, u16 wValue, > if ((temp & PORT_PE) =3D=3D 0) > goto error; >=20=20 > + set_bit(wIndex, &bus_state->resuming_ports); > xhci_set_link_state(xhci, port_array, wIndex, > XDEV_RESUME); > spin_unlock_irqrestore(&xhci->lock, flags); > @@ -1002,6 +1035,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeR= eq, u16 wValue, > spin_lock_irqsave(&xhci->lock, flags); > xhci_set_link_state(xhci, port_array, wIndex, > XDEV_U0); > + clear_bit(wIndex, &bus_state->resuming_ports); > } > bus_state->port_c_suspend |=3D 1 << wIndex; >=20=20 > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 97ffe39..3743bb2 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1583,7 +1583,8 @@ static void handle_port_status(struct xhci_hcd *xhc= i, > */ > bogus_port_status =3D true; > goto cleanup; > - } else { > + } else if (!test_bit(faked_port_index, > + &bus_state->resuming_ports)) { > xhci_dbg(xhci, "resume HS port %d\n", port_id); > bus_state->resume_done[faked_port_index] =3D jiffies + > msecs_to_jiffies(USB_RESUME_TIMEOUT); > --=20 > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWXa9pAAoJEIaOsuA1yqREbjMQAK5zVsZjVYB3HATLIxTM5uGT ZGYjDbP7bwGhLqHA6MtmD9YmOFmeH3QyL/7Gm+O+gqnghEQkxLsb352fv16sH9Qz sYHrwULvPnDA5fp9oqlymbJTUGRE9ds34djKROT5/jSVPdaT2rXm7bYdsty3wu2H Y4nAG9wY3s3ogwpjCuATtxIgO1wiss49rPzPOXu8VtuP5U2HInFeDiKVeIz1kX0O /nDx5vpQd+Fk5vtj7x8dpuMRA3FuhF3HjaISnuRnpXtvxwR3Cz+Qti3H1WFT24Wf Tch7wVTe+vNkVystf1RtmUvz4a9Kwbpxd/OfeDVCJc7tcXrqQv4jDyXzFDa+b0RC 3fvRFKymMIW/bVw5R7WmT3peDaX4c6Bkf09T5WB9kYvUBMwPcTU/0Wv44d+W6NjE swZeWzSMPsPysBTL4BuzgTbcCf2+NLkaUbm3CSeH2bRaIFybhw+w22W7mPTR80cj ZtF8bZY4keL+MFsbcWl/AqH14naL5f1B7Bfr0tdYfoWHoEB/uMhQjA7kCxFQQRbE dskFqIpGyFNvfjcpezi7divk5A9LjAyjLETvARFd26ZIhyj3rEkywqKaH0jZfef7 d7fr6UItgPqdg5Qx8uNzTISvaTVaGvDSmV0ohdQGyhjBnm4pxKKB9pzeo0KLJixC FNXY1OcuoNrtXG6qDkdk =tyrq -----END PGP SIGNATURE----- --=-=-=-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/