Re: kern/56739: Hang during boot since recent USB changes

To: kern-bug-people%netbsd.org@localhost,gnats-admin%netbsd.org@localhost,netbsd-bugs%netbsd.org@localhost,gson%gson.org@localhost (Andreas Gustafsson)
Subject: Re: kern/56739: Hang during boot since recent USB changes
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 9 Mar 2022 12:20:01 +0000 (UTC)

The following reply was made to PR kern/56739; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Andreas Gustafsson <gson%gson.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/56739: Hang during boot since recent USB changes
Date: Wed, 9 Mar 2022 12:19:07 +0000

 This is a multi-part message in MIME format.
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
 
 Can you try the attached patch?
 
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
 Content-Type: text/plain; charset="ISO-8859-1"; name="usbroothub"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="usbroothub.patch"
 
 From 9aa011ffacd2e0f57490342446173233d6231ae9 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Wed, 9 Mar 2022 11:37:27 +0000
 Subject: [PATCH] usb: Provisionally release bus lock around ubm_rhctrl.
 
 This isn't quite correct, but it avoids a deadlock:
 
 - *_roothub_ctrl holds bus lock, waits in usb_delay_ms for kpause
 - softint waits for bus lock, holds up kpause wakeup
 
 The deadlock is new since recent changes to hold the bus lock over
 upm_start/upm_transfer.  Making this change regresses to other
 problems:
 
 - *_suspend/resume and *_roothub_ctrl often touch the same portsc
    registers
 
 - roothub_ctrl_abort needs to wait for ubm_rhctrl to complete.
 
   When the bus lock was held across both, a noop served here, but we
   can't hold the bus lock across both, so that doesn't work.
 
 However, these problems -- which we've had for a long time -- seem to
 be less bad than the deadlock.  So let's avoid the deadlock for now
 and then work out another way to serialize suspend/resume/rhctrl and
 aborts.
 
 Candidate fix for PR kern/56739.
 ---
  sys/arch/mips/adm5120/dev/ahci.c |  2 --
  sys/dev/ic/sl811hs.c             |  2 --
  sys/dev/usb/ehci.c               |  8 +++++++-
  sys/dev/usb/motg.c               |  2 --
  sys/dev/usb/ohci.c               |  2 --
  sys/dev/usb/uhci.c               |  2 --
  sys/dev/usb/usbdivar.h           |  2 +-
  sys/dev/usb/usbroothub.c         | 12 ++++++++++++
  sys/dev/usb/xhci.c               |  2 --
  9 files changed, 20 insertions(+), 14 deletions(-)
 
 diff --git a/sys/arch/mips/adm5120/dev/ahci.c b/sys/arch/mips/adm5120/dev/a=
 hci.c
 index ca53af493e9b..7bc3a9fd0008 100644
 --- a/sys/arch/mips/adm5120/dev/ahci.c
 +++ b/sys/arch/mips/adm5120/dev/ahci.c
 @@ -569,8 +569,6 @@ ahci_roothub_ctrl(struct usbd_bus *bus, usb_device_requ=
 est_t *req,
 =20
  	DPRINTF(D_TRACE, ("SLRCstart "));
 =20
 -	KASSERT(bus->ub_polling || mutex_owned(bus->ub_lock));
 -
  	len =3D UGETW(req->wLength);
  	value =3D UGETW(req->wValue);
  	index =3D UGETW(req->wIndex);
 diff --git a/sys/dev/ic/sl811hs.c b/sys/dev/ic/sl811hs.c
 index 2a10911f3066..76db51573d95 100644
 --- a/sys/dev/ic/sl811hs.c
 +++ b/sys/dev/ic/sl811hs.c
 @@ -3195,8 +3195,6 @@ slhci_roothub_ctrl(struct usbd_bus *bus, usb_device_r=
 equest_t *req,
  	uint8_t type;
  	int actlen =3D 0;
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 -
  	len =3D UGETW(req->wLength);
  	value =3D UGETW(req->wValue);
  	index =3D UGETW(req->wIndex);
 diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
 index 5efa069a37af..4765b744d0e4 100644
 --- a/sys/dev/usb/ehci.c
 +++ b/sys/dev/usb/ehci.c
 @@ -1438,6 +1438,9 @@ ehci_activate(device_t self, enum devact act)
   *
   * Note that this power handler isn't to be registered directly; the
   * bus glue needs to call out to it.
 + *
 + * XXX This should be serialized with ehci_roothub_ctrl's access to the
 + * portsc registers.
   */
  bool
  ehci_suspend(device_t dv, const pmf_qual_t *qual)
 @@ -2369,7 +2372,10 @@ ehci_roothub_ctrl(struct usbd_bus *bus, usb_device_r=
 equest_t *req,
 =20
  	EHCIHIST_FUNC(); EHCIHIST_CALLED();
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 +	/*
 +	 * XXX This should be serialized with ehci_suspend/resume's
 +	 * access to the portsc registers.
 +	 */
 =20
  	if (sc->sc_dying)
  		return -1;
 diff --git a/sys/dev/usb/motg.c b/sys/dev/usb/motg.c
 index 36da0575b148..aba2ab1b4090 100644
 --- a/sys/dev/usb/motg.c
 +++ b/sys/dev/usb/motg.c
 @@ -806,8 +806,6 @@ motg_roothub_ctrl(struct usbd_bus *bus, usb_device_requ=
 est_t *req,
 =20
  	MOTGHIST_FUNC(); MOTGHIST_CALLED();
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 -
  	if (sc->sc_dying)
  		return -1;
 =20
 diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
 index c55078e8217f..c13ec53b5688 100644
 --- a/sys/dev/usb/ohci.c
 +++ b/sys/dev/usb/ohci.c
 @@ -2431,8 +2431,6 @@ ohci_roothub_ctrl(struct usbd_bus *bus, usb_device_re=
 quest_t *req,
 =20
  	OHCIHIST_FUNC(); OHCIHIST_CALLED();
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 -
  	if (sc->sc_dying)
  		return -1;
 =20
 diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c
 index 99409084944f..fa80f49d6e6c 100644
 --- a/sys/dev/usb/uhci.c
 +++ b/sys/dev/usb/uhci.c
 @@ -3589,8 +3589,6 @@ uhci_roothub_ctrl(struct usbd_bus *bus, usb_device_re=
 quest_t *req,
 =20
  	UHCIHIST_FUNC(); UHCIHIST_CALLED();
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 -
  	if (sc->sc_dying)
  		return -1;
 =20
 diff --git a/sys/dev/usb/usbdivar.h b/sys/dev/usb/usbdivar.h
 index 5a5d94f26ccf..1be0236e49d2 100644
 --- a/sys/dev/usb/usbdivar.h
 +++ b/sys/dev/usb/usbdivar.h
 @@ -51,7 +51,7 @@
   *	ubm_abortx		x	must not release/reacquire lock
   *	ubm_getlock 		-	Called at attach time
   *	ubm_newdev		-	Will take lock
 - *	ubm_rhctrl              x
 + *	ubm_rhctrl              -
   *
   *	PIPE METHOD		LOCK	NOTES
   *	----------------------- -------	-------------------------
 diff --git a/sys/dev/usb/usbroothub.c b/sys/dev/usb/usbroothub.c
 index 6425d9cf21c7..74d42266725e 100644
 --- a/sys/dev/usb/usbroothub.c
 +++ b/sys/dev/usb/usbroothub.c
 @@ -549,7 +549,19 @@ roothub_ctrl_start(struct usbd_xfer *xfer)
  		break;
  	}
 =20
 +	/*
 +	 * XXX This needs some mechanism for concurrent
 +	 * roothub_ctrl_abort to wait for ubm_rhctrl to finish.  We
 +	 * can't use the bus lock because many ubm_rhctrl methods do
 +	 * usb_delay_ms and many bus locks are taken in softint
 +	 * context, leading to deadlock in the softclock needed to wake
 +	 * up usb_delay_ms.
 +	 */
 +	if (!bus->ub_usepolling)
 +		mutex_exit(bus->ub_lock);
  	actlen =3D bus->ub_methods->ubm_rhctrl(bus, req, buf, buflen);
 +	if (!bus->ub_usepolling)
 +		mutex_enter(bus->ub_lock);
  	if (actlen < 0)
  		goto fail;
 =20
 diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c
 index 96663025878c..885bbc0af378 100644
 --- a/sys/dev/usb/xhci.c
 +++ b/sys/dev/usb/xhci.c
 @@ -3856,8 +3856,6 @@ xhci_roothub_ctrl(struct usbd_bus *bus, usb_device_re=
 quest_t *req,
 =20
  	XHCIHIST_FUNC();
 =20
 -	KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
 -
  	if (sc->sc_dying)
  		return -1;
 =20
 
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG--

Prev by Date: Re: kern/56739: Hang during boot since recent USB changes
Next by Date: bin/56743: Cannot create more than 1022 files from an AWK program
Previous by Thread: Re: kern/56739: Hang during boot since recent USB changes
Next by Thread: Re: kern/56739: Hang during boot since recent USB changes
Indexes:

Home | Main Index | Thread Index | Old Index