On Tue, Jun 09, 2020 at 10:53:24AM +0200, Martin Pieuchot wrote:
>
> The audio(4) drivers has an unaccounted reference to uaudio(4)'s softc.
> So when the USB thread responsible for detaching device kicks in to
> clean up the software state of an uaudio(4), it first spins on the
> KERNEL_LOCK(). If any of the threads playing/recording audio sleeps
> while holding an unaccounted reference to uaudio(4)'s softc, the above
> issue can happen.
>
> A way to fix this is to use usbd_ref_incr(9) and its counterpart
> usbd_ref_wait(9) in uaudio_detach().
>
> I'm not sure if it's possible for audio(4) to increment the reference
> only once. Is there a place where such increment/decrement can be put?
>
> Otherwise every operation should do the dance.
Thanks for the explanation.
- uaudio_{open,close}() could increment/decrement the counter for the
audio code-paths
- uaudio_{query_devinfo,set_port,get_port} for the controls
manipulation code-paths. These functions are called directly by the
kernel, and are not wrapped in uaudio_{open,close} calls (yet).
Resulting diff below
Index: uaudio.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.159
diff -u -p -u -p -r1.159 uaudio.c
--- uaudio.c 4 May 2020 19:19:26 -0000 1.159
+++ uaudio.c 9 Jun 2020 12:07:20 -0000
@@ -3854,6 +3854,8 @@ uaudio_detach(struct device *self, int f
rv = config_detach_children(self, flags);
+ usbd_ref_wait(sc->udev);
+
while ((alt = sc->alts) != NULL) {
sc->alts = alt->next;
free(alt, M_DEVBUF, sizeof(struct uaudio_alt));
@@ -3892,6 +3894,8 @@ uaudio_open(void *self, int flags)
if (usbd_is_dying(sc->udev))
return EIO;
+ usbd_ref_incr(sc->udev);
+
flags &= (FREAD | FWRITE);
for (p = sc->params_list; p != NULL; p = p->next) {
@@ -3914,6 +3918,7 @@ uaudio_open(void *self, int flags)
}
}
+ usbd_ref_decr(sc->udev);
return ENXIO;
}
@@ -3923,6 +3928,7 @@ uaudio_close(void *self)
struct uaudio_softc *sc = self;
sc->mode = 0;
+ usbd_ref_decr(sc->udev);
}
int
@@ -4183,9 +4189,8 @@ uaudio_get_props(void *self)
}
int
-uaudio_get_port(void *arg, struct mixer_ctrl *ctl)
+uaudio_get_port_do(struct uaudio_softc *sc, struct mixer_ctrl *ctl)
{
- struct uaudio_softc *sc = arg;
struct uaudio_unit *u;
struct uaudio_mixent *m;
unsigned char req_buf[4];
@@ -4253,9 +4258,8 @@ uaudio_get_port(void *arg, struct mixer_
}
int
-uaudio_set_port(void *arg, struct mixer_ctrl *ctl)
+uaudio_set_port_do(struct uaudio_softc *sc, struct mixer_ctrl *ctl)
{
- struct uaudio_softc *sc = arg;
struct uaudio_unit *u;
struct uaudio_mixent *m;
unsigned char req_buf[4];
@@ -4312,9 +4316,8 @@ uaudio_set_port(void *arg, struct mixer_
}
int
-uaudio_query_devinfo(void *arg, struct mixer_devinfo *devinfo)
+uaudio_query_devinfo_do(struct uaudio_softc *sc, struct mixer_devinfo *devinfo)
{
- struct uaudio_softc *sc = arg;
struct uaudio_unit *u;
struct uaudio_mixent *m;
@@ -4379,4 +4382,40 @@ uaudio_query_devinfo(void *arg, struct m
break;
}
return 0;
+}
+
+int
+uaudio_get_port(void *arg, struct mixer_ctrl *ctl)
+{
+ struct uaudio_softc *sc = arg;
+ int rc;
+
+ usbd_ref_incr(sc->udev);
+ rc = uaudio_get_port_do(sc, ctl);
+ usbd_ref_decr(sc->udev);
+ return rc;
+}
+
+int
+uaudio_set_port(void *arg, struct mixer_ctrl *ctl)
+{
+ struct uaudio_softc *sc = arg;
+ int rc;
+
+ usbd_ref_incr(sc->udev);
+ rc = uaudio_set_port_do(sc, ctl);
+ usbd_ref_decr(sc->udev);
+ return rc;
+}
+
+int
+uaudio_query_devinfo(void *arg, struct mixer_devinfo *devinfo)
+{
+ struct uaudio_softc *sc = arg;
+ int rc;
+
+ usbd_ref_incr(sc->udev);
+ rc = uaudio_query_devinfo_do(sc, devinfo);
+ usbd_ref_decr(sc->udev);
+ return rc;
}
