Re: [PATCH] ALSA: usb-audio: reset a MIDI device whose input is stuck

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usb-audio: stale failing state in snd_usbmidi_in_error() resets device
- [Medium] ALSA: usb-audio: duplicate device resets from per-endpoint reset_queued state
--

commit c2e2b9a4ef7034463197fae2778c946cbde9e7fb
Author: Stjepan Poljak <stjepan.poljak@gmail.com>

ALSA: usb-audio: reset a MIDI device whose input is stuck

This commit delays resubmission of stalled (-EPIPE) USB MIDI input URBs via
the error timer and queues a USB device reset when an input endpoint stalls or
fails without a successful URB for 2 seconds.

> diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> index e3d1575540613..7f93df66e55b7 100644
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c

[ ... ]

> @@ -162,6 +168,10 @@ struct snd_usb_midi_in_endpoint {
>  	bool in_sysex;
>  	u8 last_cin;
>  	u8 error_resubmit;
> +	u8 failing;		/* no successful URB since the last error */
> +	u8 reset_queued;	/* at reset_at */
> +	unsigned long failing_since;
> +	unsigned long reset_at;
>  	int current_port;
>  };

[Severity: Medium]
Should reset_queued and reset_at be stored on struct snd_usb_midi rather than
struct snd_usb_midi_in_endpoint?

In __snd_usbmidi_create(), a single struct snd_usb_midi bound to umidi->iface
can have multiple input endpoints in umidi->endpoints[i].in (for example, via
snd_usbmidi_get_ms_info() or snd_usbmidi_detect_per_port_endpoints()).

When multiple input endpoints on the same interface fail,
snd_usbmidi_in_urb_complete() -> snd_usbmidi_in_error() checks
ep->reset_queued on each endpoint independently:

    ep->reset_queued = 1;
    ep->reset_at = jiffies;
    dev_warn(&ep->umidi->dev->dev,
             "MIDI input not recovering (urb status %d), resetting device\n",
             status);
    usb_queue_reset_device(ep->umidi->iface);

Once system_wq starts running __usb_queue_reset_device() for the first
endpoint, WORK_STRUCT_PENDING_BIT on iface->reset_ws is cleared while
usb_reset_device() is in progress. Before usb_reset_device() finishes
unbinding interfaces and killing URBs in snd_usbmidi_disconnect(), a failing
URB on a second endpoint (where ep->reset_queued is still 0) will log a
duplicate warning and call usb_queue_reset_device(ep->umidi->iface) again.

Because neither usb_reset_device() nor usb_unbind_interface() cancels
iface->reset_ws, can that re-queued work run after the first reset finishes
and immediately reset the newly re-probed device a second time?

> @@ -234,6 +244,36 @@ static void dump_urb(const char *type, const u8 *data, int length)
>  #define dump_urb(type, data, length) /* nothing */
>  #endif
>  
> +/*
> + * A device that was power cycled without the hub noticing (e.g. a bus
> + * powered keyboard switched off and on again) has lost its configuration:
> + * it stalls the input endpoint or stops answering, and resubmitting the
> + * URBs never recovers it. Reset it instead; snd-usb-audio has no
> + * pre_reset/post_reset, so this unbinds and re-probes the card.
> + * A queued reset is skipped if usb_lock_device_for_reset() fails, so if
> + * we're still here a while later, ask again.
> + */
> +static void snd_usbmidi_in_error(struct snd_usb_midi_in_endpoint *ep,
> +				 int status)
> +{
> +	if (!ep->failing) {
> +		ep->failing = 1;
> +		ep->failing_since = jiffies;
> +	}
> +	if (ep->reset_queued &&
> +	    time_before(jiffies, ep->reset_at + RESET_DELAY_JIFFIES))
> +		return;
> +	if (status != -EPIPE &&
> +	    time_before(jiffies, ep->failing_since + RESET_DELAY_JIFFIES))
> +		return;
> +	ep->reset_queued = 1;
> +	ep->reset_at = jiffies;
> +	dev_warn(&ep->umidi->dev->dev,
> +		 "MIDI input not recovering (urb status %d), resetting device\n",
> +		 status);
> +	usb_queue_reset_device(ep->umidi->iface);
> +}
> +
>  /*
>   * Processes the data read from the device.
>   */
> @@ -242,13 +282,16 @@ static void snd_usbmidi_in_urb_complete(struct urb *urb)
>  	struct snd_usb_midi_in_endpoint *ep = urb->context;
>  
>  	if (urb->status == 0) {
> +		ep->failing = 0;

[Severity: High]
Can ep->failing and ep->failing_since stay latched across idle periods and
trigger a false-positive device reset on the next isolated error?

Unlike isochronous audio endpoints, USB MIDI input endpoints created in
snd_usbmidi_in_endpoint_create() are bulk or interrupt IN endpoints. When a
healthy MIDI device is idle and not transmitting MIDI data, it NAKs IN tokens
and submitted IN URBs remain pending without completing.

If a single transient bus error (-EPROTO, -ETIME, or -EILSEQ) occurs while the
device is idle:

1. snd_usbmidi_in_urb_complete() -> snd_usbmidi_in_error() sets
   ep->failing = 1 and latches ep->failing_since = jiffies.
2. snd_usbmidi_error_timer() resubmits the URB after ERROR_DELAY_JIFFIES
   (100 ms).
3. Because the device is healthy and idle, the resubmitted URB stays pending
   on the bus without completing. Since ep->failing is only cleared in
   snd_usbmidi_in_urb_complete() when an URB completes with urb->status == 0
   (and is also not cleared in snd_usbmidi_input_stop() or
   snd_usbmidi_input_start() when usb_kill_urb() completes URBs with
   -ENOENT), ep->failing remains 1 and ep->failing_since stays latched at the
   old timestamp indefinitely.

If another single transient error (or the first -EPROTO of a normal cable
unplug) occurs more than 2 seconds later without any MIDI notes having been
received in between, time_before(jiffies, ep->failing_since +
RESET_DELAY_JIFFIES) in snd_usbmidi_in_error() evaluates to false on the very
first failing URB.

Won't this immediately call usb_queue_reset_device(ep->umidi->iface) and
unbind the entire ALSA card (including any active PCM audio or MIDI output
streams on a composite device) without input having continuously failed for 2
seconds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026125751.1700779-1-stjepan.poljak@gmail.com?part=1