* [BUG] ALSA: usb-audio: drain may fail with multi-port close race
@ 2025-02-17 11:16 John Keeping
2025-02-17 17:06 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: John Keeping @ 2025-02-17 11:16 UTC (permalink / raw)
To: Takashi Iwai
Cc: John Keeping, Clemens Ladisch, Jaroslav Kysela, linux-sound,
linux-kernel
I'm seeing a bug where data sometimes fails to send on USB MIDI devices
with multiple ports which seems to be a result of a race around closing
ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
pending work at closing a MIDI substream").
The scenario is essentially this program:
snd_rawmidi_t *port0, *port1;
snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
snd_rawmidi_write(port0, data, len);
snd_rawmidi_close(port1);
snd_rawmidi_close(port0);
What happens seems to be the following:
write(port0)
`- snd_usbmidi_output_trigger
`- queue_work()
close(port1)
`- snd_usbmidi_output_close
`- cancel_work_sync() # Work has not yet started here
close(port0)
`- snd_rawmidi_drain_output
# Times out because nothing is processing outbound data!
The two ports interact like this because they are on the same endpoint,
so should the work only be cancelled when the last endpoint is closed?
Thanks,
John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race
2025-02-17 11:16 [BUG] ALSA: usb-audio: drain may fail with multi-port close race John Keeping
@ 2025-02-17 17:06 ` Takashi Iwai
2025-02-17 18:38 ` John Keeping
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2025-02-17 17:06 UTC (permalink / raw)
To: John Keeping
Cc: Takashi Iwai, Clemens Ladisch, Jaroslav Kysela, linux-sound,
linux-kernel
On Mon, 17 Feb 2025 12:16:46 +0100,
John Keeping wrote:
>
> I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> with multiple ports which seems to be a result of a race around closing
> ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> pending work at closing a MIDI substream").
>
> The scenario is essentially this program:
>
> snd_rawmidi_t *port0, *port1;
> snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
>
> snd_rawmidi_write(port0, data, len);
>
> snd_rawmidi_close(port1);
> snd_rawmidi_close(port0);
>
> What happens seems to be the following:
>
> write(port0)
> `- snd_usbmidi_output_trigger
> `- queue_work()
> close(port1)
> `- snd_usbmidi_output_close
> `- cancel_work_sync() # Work has not yet started here
> close(port0)
> `- snd_rawmidi_drain_output
> # Times out because nothing is processing outbound data!
>
> The two ports interact like this because they are on the same endpoint,
> so should the work only be cancelled when the last endpoint is closed?
How about the following patch work?
It's a band-aid, but should suffice. The callback is already
protected with rawmidi open_mutex.
thanks,
Takashi
-- 8< --
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1144,8 +1144,11 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
{
struct usbmidi_out_port *port = substream->runtime->private_data;
+ struct snd_usb_midi *umidi = substream->rmidi->private_data;
- cancel_work_sync(&port->ep->work);
+ /* cancel at the last close */
+ if (umidi->opened[0] == 1)
+ cancel_work_sync(&port->ep->work);
return substream_open(substream, 0, 0);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race
2025-02-17 17:06 ` Takashi Iwai
@ 2025-02-17 18:38 ` John Keeping
2025-02-18 8:07 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: John Keeping @ 2025-02-17 18:38 UTC (permalink / raw)
To: Takashi Iwai
Cc: Takashi Iwai, Clemens Ladisch, Jaroslav Kysela, linux-sound,
linux-kernel
On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> On Mon, 17 Feb 2025 12:16:46 +0100,
> John Keeping wrote:
> >
> > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > with multiple ports which seems to be a result of a race around closing
> > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > pending work at closing a MIDI substream").
> >
> > The scenario is essentially this program:
> >
> > snd_rawmidi_t *port0, *port1;
> > snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> >
> > snd_rawmidi_write(port0, data, len);
> >
> > snd_rawmidi_close(port1);
> > snd_rawmidi_close(port0);
> >
> > What happens seems to be the following:
> >
> > write(port0)
> > `- snd_usbmidi_output_trigger
> > `- queue_work()
> > close(port1)
> > `- snd_usbmidi_output_close
> > `- cancel_work_sync() # Work has not yet started here
> > close(port0)
> > `- snd_rawmidi_drain_output
> > # Times out because nothing is processing outbound data!
> >
> > The two ports interact like this because they are on the same endpoint,
> > so should the work only be cancelled when the last endpoint is closed?
>
> How about the following patch work?
> It's a band-aid, but should suffice. The callback is already
> protected with rawmidi open_mutex.
Yes, this patch fixes it and is
Tested-by: John Keeping <jkeeping@inmusicbrands.com>
Thanks for the quick response!
John
> -- 8< --
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -1144,8 +1144,11 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
> static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
> {
> struct usbmidi_out_port *port = substream->runtime->private_data;
> + struct snd_usb_midi *umidi = substream->rmidi->private_data;
>
> - cancel_work_sync(&port->ep->work);
> + /* cancel at the last close */
> + if (umidi->opened[0] == 1)
> + cancel_work_sync(&port->ep->work);
> return substream_open(substream, 0, 0);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race
2025-02-17 18:38 ` John Keeping
@ 2025-02-18 8:07 ` Takashi Iwai
2025-02-18 10:56 ` John Keeping
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2025-02-18 8:07 UTC (permalink / raw)
To: John Keeping
Cc: Takashi Iwai, Takashi Iwai, Clemens Ladisch, Jaroslav Kysela,
linux-sound, linux-kernel
On Mon, 17 Feb 2025 19:38:31 +0100,
John Keeping wrote:
>
> On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > On Mon, 17 Feb 2025 12:16:46 +0100,
> > John Keeping wrote:
> > >
> > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > with multiple ports which seems to be a result of a race around closing
> > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > pending work at closing a MIDI substream").
> > >
> > > The scenario is essentially this program:
> > >
> > > snd_rawmidi_t *port0, *port1;
> > > snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > >
> > > snd_rawmidi_write(port0, data, len);
> > >
> > > snd_rawmidi_close(port1);
> > > snd_rawmidi_close(port0);
> > >
> > > What happens seems to be the following:
> > >
> > > write(port0)
> > > `- snd_usbmidi_output_trigger
> > > `- queue_work()
> > > close(port1)
> > > `- snd_usbmidi_output_close
> > > `- cancel_work_sync() # Work has not yet started here
> > > close(port0)
> > > `- snd_rawmidi_drain_output
> > > # Times out because nothing is processing outbound data!
> > >
> > > The two ports interact like this because they are on the same endpoint,
> > > so should the work only be cancelled when the last endpoint is closed?
> >
> > How about the following patch work?
> > It's a band-aid, but should suffice. The callback is already
> > protected with rawmidi open_mutex.
>
> Yes, this patch fixes it and is
>
> Tested-by: John Keeping <jkeeping@inmusicbrands.com>
Thank you for quick testing!
Looking at the code again, I think the suggested fix isn't right.
It still allows some pending work accessing the freed object.
Could you test the following one-liner instead?
Takashi
-- 8< --
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1145,7 +1145,7 @@ static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
{
struct usbmidi_out_port *port = substream->runtime->private_data;
- cancel_work_sync(&port->ep->work);
+ flush_work(&port->ep->work);
return substream_open(substream, 0, 0);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race
2025-02-18 8:07 ` Takashi Iwai
@ 2025-02-18 10:56 ` John Keeping
2025-02-18 11:39 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: John Keeping @ 2025-02-18 10:56 UTC (permalink / raw)
To: Takashi Iwai
Cc: Takashi Iwai, Clemens Ladisch, Jaroslav Kysela, linux-sound,
linux-kernel
On Tue, Feb 18, 2025 at 09:07:26AM +0100, Takashi Iwai wrote:
> On Mon, 17 Feb 2025 19:38:31 +0100,
> John Keeping wrote:
> >
> > On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > > On Mon, 17 Feb 2025 12:16:46 +0100,
> > > John Keeping wrote:
> > > >
> > > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > > with multiple ports which seems to be a result of a race around closing
> > > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > > pending work at closing a MIDI substream").
> > > >
> > > > The scenario is essentially this program:
> > > >
> > > > snd_rawmidi_t *port0, *port1;
> > > > snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > > snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > > >
> > > > snd_rawmidi_write(port0, data, len);
> > > >
> > > > snd_rawmidi_close(port1);
> > > > snd_rawmidi_close(port0);
> > > >
> > > > What happens seems to be the following:
> > > >
> > > > write(port0)
> > > > `- snd_usbmidi_output_trigger
> > > > `- queue_work()
> > > > close(port1)
> > > > `- snd_usbmidi_output_close
> > > > `- cancel_work_sync() # Work has not yet started here
> > > > close(port0)
> > > > `- snd_rawmidi_drain_output
> > > > # Times out because nothing is processing outbound data!
> > > >
> > > > The two ports interact like this because they are on the same endpoint,
> > > > so should the work only be cancelled when the last endpoint is closed?
> > >
> > > How about the following patch work?
> > > It's a band-aid, but should suffice. The callback is already
> > > protected with rawmidi open_mutex.
> >
> > Yes, this patch fixes it and is
> >
> > Tested-by: John Keeping <jkeeping@inmusicbrands.com>
>
> Thank you for quick testing!
>
> Looking at the code again, I think the suggested fix isn't right.
> It still allows some pending work accessing the freed object.
>
> Could you test the following one-liner instead?
Tested-by: John Keeping <jkeeping@inmusicbrands.com>
The patch below also fixes the issue. Thanks!
John
> -- 8< --
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -1145,7 +1145,7 @@ static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
> {
> struct usbmidi_out_port *port = substream->runtime->private_data;
>
> - cancel_work_sync(&port->ep->work);
> + flush_work(&port->ep->work);
> return substream_open(substream, 0, 0);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race
2025-02-18 10:56 ` John Keeping
@ 2025-02-18 11:39 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2025-02-18 11:39 UTC (permalink / raw)
To: John Keeping
Cc: Takashi Iwai, Takashi Iwai, Clemens Ladisch, Jaroslav Kysela,
linux-sound, linux-kernel
On Tue, 18 Feb 2025 11:56:19 +0100,
John Keeping wrote:
>
> On Tue, Feb 18, 2025 at 09:07:26AM +0100, Takashi Iwai wrote:
> > On Mon, 17 Feb 2025 19:38:31 +0100,
> > John Keeping wrote:
> > >
> > > On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > > > On Mon, 17 Feb 2025 12:16:46 +0100,
> > > > John Keeping wrote:
> > > > >
> > > > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > > > with multiple ports which seems to be a result of a race around closing
> > > > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > > > pending work at closing a MIDI substream").
> > > > >
> > > > > The scenario is essentially this program:
> > > > >
> > > > > snd_rawmidi_t *port0, *port1;
> > > > > snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > > > snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > > > >
> > > > > snd_rawmidi_write(port0, data, len);
> > > > >
> > > > > snd_rawmidi_close(port1);
> > > > > snd_rawmidi_close(port0);
> > > > >
> > > > > What happens seems to be the following:
> > > > >
> > > > > write(port0)
> > > > > `- snd_usbmidi_output_trigger
> > > > > `- queue_work()
> > > > > close(port1)
> > > > > `- snd_usbmidi_output_close
> > > > > `- cancel_work_sync() # Work has not yet started here
> > > > > close(port0)
> > > > > `- snd_rawmidi_drain_output
> > > > > # Times out because nothing is processing outbound data!
> > > > >
> > > > > The two ports interact like this because they are on the same endpoint,
> > > > > so should the work only be cancelled when the last endpoint is closed?
> > > >
> > > > How about the following patch work?
> > > > It's a band-aid, but should suffice. The callback is already
> > > > protected with rawmidi open_mutex.
> > >
> > > Yes, this patch fixes it and is
> > >
> > > Tested-by: John Keeping <jkeeping@inmusicbrands.com>
> >
> > Thank you for quick testing!
> >
> > Looking at the code again, I think the suggested fix isn't right.
> > It still allows some pending work accessing the freed object.
> >
> > Could you test the following one-liner instead?
>
> Tested-by: John Keeping <jkeeping@inmusicbrands.com>
>
> The patch below also fixes the issue. Thanks!
OK, I'm going to submit and merge the proper patch.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-18 11:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-17 11:16 [BUG] ALSA: usb-audio: drain may fail with multi-port close race John Keeping
2025-02-17 17:06 ` Takashi Iwai
2025-02-17 18:38 ` John Keeping
2025-02-18 8:07 ` Takashi Iwai
2025-02-18 10:56 ` John Keeping
2025-02-18 11:39 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox