Hi Felipe, > >Pawel Laszczak <pawell@xxxxxxxxxxx> writes: >> From: Pawel Laszczak <pawell@xxxxxxxxxxx> >> >> Patch adds disabling endpoint before enabling it during changing >> alternate setting. Lack of this functionality causes that in some >> cases uac2 queue the same request multiple time. >> Such situation can occur when host send set interface with >> alternate setting 1 twice. > >Which host is doing that? I've found out this issue on different scenario, but we can imaging the case with double alternate setting. My case looks like: - host send Set Alternate Interface (1) - device controller doesn't send ACK for SETUP packet - device delegates request to class and class starts processing it (enable endpoints, etc.) - in the meantime host re-send the same SETUP packet - device controller driver detects this packet and try to finish previous one (according to USB spec), but it cannot disable enabled endpoints. - device delegate the new SETUP packet to uac2 class - uac2 again enables endpoint and try to queues again queued requests - system crash The similar solution exist in f_uvc.c: https://elixir.bootlin.com/linux/v5.12-rc8/source/drivers/usb/gadget/function/f_uvc.c#L290 I didn't check the other drivers. Maybe such fix should be added somewhere else. Such issue can be very rare and very hard to debug. > >> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx> >> >> --- >> Changelog: >> v2: >> - moved disabling endpoint into u_audio_start_playback >> >> drivers/usb/gadget/function/u_audio.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c >> index 265c4d805f81..c4bbc9decaba 100644 >> --- a/drivers/usb/gadget/function/u_audio.c >> +++ b/drivers/usb/gadget/function/u_audio.c >> @@ -401,6 +401,10 @@ int u_audio_start_playback(struct g_audio *audio_dev) >> >> ep = audio_dev->in_ep; >> prm = &uac->p_prm; >> + >> + if (prm->ep_enabled) >> + u_audio_stop_capture(audio_dev); > >this looks to be a bug in f_uac2.c::afunc_set_alt(), actually. Note how >e.g. f_obex.c::obex_set_alt() is implemented: > >> } else if (intf == obex->data_id) { >> if (alt > 1) >> goto fail; >> >> if (obex->port.in->enabled) { > >if interface is already enabled... > >> dev_dbg(&cdev->gadget->dev, >> "reset obex ttyGS%d\n", obex->port_num); >> gserial_disconnect(&obex->port); > >...disable it first... > >> } >> >> if (!obex->port.in->desc || !obex->port.out->desc) { >> dev_dbg(&cdev->gadget->dev, >> "init obex ttyGS%d\n", obex->port_num); >> if (config_ep_by_speed(cdev->gadget, f, >> obex->port.in) || >> config_ep_by_speed(cdev->gadget, f, >> obex->port.out)) { > >...before configuring endpoints again > -- Regards Pawel Laszczak
