[PATCH 0/2] USB MIDI fixes

[PATCH 0/2] USB MIDI fixes

[Takashi Iwai]

Hi,

this is a series of patches to fix pending issues wrt USB MIDI in the
recent kernels.

The first patch is to close the disconnection race for autopm and other
codes in sound/usb/midi.c.
The second patch is a fix for missing autopm handling for USB MIDI input,
which has been discussed shortly ago.

It's based on the latest 3.7-rc, but should be applicable to the latest
stable-queue for 3.6 and older, too.  The previous autopm fix in stable
tree isn't released yet, so you can't apply it to the released 3.6.y.
You'll need pick up the patch from stable-queue tree.


Takashi

[PATCH 1/2] ALSA: usb-audio: Avoid autopm calls after disconnection

[Takashi Iwai]

Add a similar protection against the disconnection race and the
invalid use of usb instance after disconnection, as well as we've done
for the USB audio PCM.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=51201
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/midi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index eeefbce..c0054ee 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -116,6 +116,7 @@ struct snd_usb_midi {
 	struct list_head list;
 	struct timer_list error_timer;
 	spinlock_t disc_lock;
+	struct rw_semaphore disc_rwsem;
 	struct mutex mutex;
 	u32 usb_id;
 	int next_midi_device;
@@ -1038,6 +1039,12 @@ static void substream_open(struct snd_rawmidi_substream *substream, int open)
 	struct snd_usb_midi* umidi = substream->rmidi->private_data;
 	struct snd_kcontrol *ctl;
 
+	down_read(&umidi->disc_rwsem);
+	if (umidi->disconnected) {
+		up_read(&umidi->disc_rwsem);
+		return;
+	}
+
 	mutex_lock(&umidi->mutex);
 	if (open) {
 		if (umidi->opened++ == 0 && umidi->roland_load_ctl) {
@@ -1056,6 +1063,7 @@ static void substream_open(struct snd_rawmidi_substream *substream, int open)
 		}
 	}
 	mutex_unlock(&umidi->mutex);
+	up_read(&umidi->disc_rwsem);
 }
 
 static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
@@ -1076,8 +1084,15 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
 		snd_BUG();
 		return -ENXIO;
 	}
+
+	down_read(&umidi->disc_rwsem);
+	if (umidi->disconnected) {
+		up_read(&umidi->disc_rwsem);
+		return -ENODEV;
+	}
 	err = usb_autopm_get_interface(umidi->iface);
 	port->autopm_reference = err >= 0;
+	up_read(&umidi->disc_rwsem);
 	if (err < 0 && err != -EACCES)
 		return -EIO;
 	substream->runtime->private_data = port;
@@ -1092,8 +1107,10 @@ static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
 	struct usbmidi_out_port *port = substream->runtime->private_data;
 
 	substream_open(substream, 0);
-	if (port->autopm_reference)
+	down_read(&umidi->disc_rwsem);
+	if (!umidi->disconnected && port->autopm_reference)
 		usb_autopm_put_interface(umidi->iface);
+	up_read(&umidi->disc_rwsem);
 	return 0;
 }
 
@@ -1403,9 +1420,12 @@ void snd_usbmidi_disconnect(struct list_head* p)
 	 * a timer may submit an URB. To reliably break the cycle
 	 * a flag under lock must be used
 	 */
+	down_write(&umidi->disc_rwsem);
 	spin_lock_irq(&umidi->disc_lock);
 	umidi->disconnected = 1;
 	spin_unlock_irq(&umidi->disc_lock);
+	up_write(&umidi->disc_rwsem);
+
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i) {
 		struct snd_usb_midi_endpoint* ep = &umidi->endpoints[i];
 		if (ep->out)
@@ -2117,6 +2137,7 @@ int snd_usbmidi_create(struct snd_card *card,
 	umidi->usb_protocol_ops = &snd_usbmidi_standard_ops;
 	init_timer(&umidi->error_timer);
 	spin_lock_init(&umidi->disc_lock);
+	init_rwsem(&umidi->disc_rwsem);
 	mutex_init(&umidi->mutex);
 	umidi->usb_id = USB_ID(le16_to_cpu(umidi->dev->descriptor.idVendor),
 			       le16_to_cpu(umidi->dev->descriptor.idProduct));
-- 
1.8.0.1

[PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input

[Takashi Iwai]

The commit [88a8516a: ALSA: usbaudio: implement USB autosuspend] added
the support of autopm for USB MIDI output, but it didn't take the MIDI
input into account.

This patch adds the following for fixing the autopm:
- Manage the URB start at the first MIDI input stream open, instead of
  the time of instance creation
- Move autopm code to the common substream_open()
- Make snd_usbmidi_input_start/_stop() more robust and add the running
  state check

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/midi.c | 87 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 45 insertions(+), 42 deletions(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index c0054ee..6da1cf1 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -126,8 +126,10 @@ struct snd_usb_midi {
 		struct snd_usb_midi_in_endpoint *in;
 	} endpoints[MIDI_MAX_ENDPOINTS];
 	unsigned long input_triggered;
-	unsigned int opened;
+	bool autopm_reference;
+	unsigned int opened[2];
 	unsigned char disconnected;
+	unsigned char input_running;
 
 	struct snd_kcontrol *roland_load_ctl;
 };
@@ -149,7 +151,6 @@ struct snd_usb_midi_out_endpoint {
 		struct snd_usb_midi_out_endpoint* ep;
 		struct snd_rawmidi_substream *substream;
 		int active;
-		bool autopm_reference;
 		uint8_t cable;		/* cable number << 4 */
 		uint8_t state;
 #define STATE_UNKNOWN	0
@@ -1034,36 +1035,57 @@ static void update_roland_altsetting(struct snd_usb_midi* umidi)
 	snd_usbmidi_input_start(&umidi->list);
 }
 
-static void substream_open(struct snd_rawmidi_substream *substream, int open)
+static int substream_open(struct snd_rawmidi_substream *substream, int dir,
+			  int open)
 {
 	struct snd_usb_midi* umidi = substream->rmidi->private_data;
 	struct snd_kcontrol *ctl;
+	int err;
 
 	down_read(&umidi->disc_rwsem);
 	if (umidi->disconnected) {
 		up_read(&umidi->disc_rwsem);
-		return;
+		return open ? -ENODEV : 0;
 	}
 
 	mutex_lock(&umidi->mutex);
 	if (open) {
-		if (umidi->opened++ == 0 && umidi->roland_load_ctl) {
-			ctl = umidi->roland_load_ctl;
-			ctl->vd[0].access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE;
-			snd_ctl_notify(umidi->card,
+		if (!umidi->opened[0] && !umidi->opened[1]) {
+			err = usb_autopm_get_interface(umidi->iface);
+			umidi->autopm_reference = err >= 0;
+			if (err < 0 && err != -EACCES) {
+				up_read(&umidi->disc_rwsem);
+				return -EIO;
+			}
+			if (umidi->roland_load_ctl) {
+				ctl = umidi->roland_load_ctl;
+				ctl->vd[0].access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE;
+				snd_ctl_notify(umidi->card,
 				       SNDRV_CTL_EVENT_MASK_INFO, &ctl->id);
-			update_roland_altsetting(umidi);
+				update_roland_altsetting(umidi);
+			}
 		}
+		umidi->opened[dir]++;
+		if (umidi->opened[1])
+			snd_usbmidi_input_start(&umidi->list);
 	} else {
-		if (--umidi->opened == 0 && umidi->roland_load_ctl) {
-			ctl = umidi->roland_load_ctl;
-			ctl->vd[0].access &= ~SNDRV_CTL_ELEM_ACCESS_INACTIVE;
-			snd_ctl_notify(umidi->card,
+		umidi->opened[dir]--;
+		if (!umidi->opened[1])
+			snd_usbmidi_input_stop(&umidi->list);
+		if (!umidi->opened[0] && !umidi->opened[1]) {
+			if (umidi->roland_load_ctl) {
+				ctl = umidi->roland_load_ctl;
+				ctl->vd[0].access &= ~SNDRV_CTL_ELEM_ACCESS_INACTIVE;
+				snd_ctl_notify(umidi->card,
 				       SNDRV_CTL_EVENT_MASK_INFO, &ctl->id);
+			}
+			if (umidi->autopm_reference)
+				usb_autopm_put_interface(umidi->iface);
 		}
 	}
 	mutex_unlock(&umidi->mutex);
 	up_read(&umidi->disc_rwsem);
+	return 0;
 }
 
 static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
@@ -1071,7 +1093,6 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
 	struct snd_usb_midi* umidi = substream->rmidi->private_data;
 	struct usbmidi_out_port* port = NULL;
 	int i, j;
-	int err;
 
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i)
 		if (umidi->endpoints[i].out)
@@ -1085,33 +1106,14 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
 		return -ENXIO;
 	}
 
-	down_read(&umidi->disc_rwsem);
-	if (umidi->disconnected) {
-		up_read(&umidi->disc_rwsem);
-		return -ENODEV;
-	}
-	err = usb_autopm_get_interface(umidi->iface);
-	port->autopm_reference = err >= 0;
-	up_read(&umidi->disc_rwsem);
-	if (err < 0 && err != -EACCES)
-		return -EIO;
 	substream->runtime->private_data = port;
 	port->state = STATE_UNKNOWN;
-	substream_open(substream, 1);
-	return 0;
+	return substream_open(substream, 0, 1);
 }
 
 static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
 {
-	struct snd_usb_midi* umidi = substream->rmidi->private_data;
-	struct usbmidi_out_port *port = substream->runtime->private_data;
-
-	substream_open(substream, 0);
-	down_read(&umidi->disc_rwsem);
-	if (!umidi->disconnected && port->autopm_reference)
-		usb_autopm_put_interface(umidi->iface);
-	up_read(&umidi->disc_rwsem);
-	return 0;
+	return substream_open(substream, 0, 0);
 }
 
 static void snd_usbmidi_output_trigger(struct snd_rawmidi_substream *substream, int up)
@@ -1164,14 +1166,12 @@ static void snd_usbmidi_output_drain(struct snd_rawmidi_substream *substream)
 
 static int snd_usbmidi_input_open(struct snd_rawmidi_substream *substream)
 {
-	substream_open(substream, 1);
-	return 0;
+	return substream_open(substream, 1, 1);
 }
 
 static int snd_usbmidi_input_close(struct snd_rawmidi_substream *substream)
 {
-	substream_open(substream, 0);
-	return 0;
+	return substream_open(substream, 1, 0);
 }
 
 static void snd_usbmidi_input_trigger(struct snd_rawmidi_substream *substream, int up)
@@ -2080,12 +2080,15 @@ void snd_usbmidi_input_stop(struct list_head* p)
 	unsigned int i, j;
 
 	umidi = list_entry(p, struct snd_usb_midi, list);
+	if (!umidi->input_running)
+		return;
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i) {
 		struct snd_usb_midi_endpoint* ep = &umidi->endpoints[i];
 		if (ep->in)
 			for (j = 0; j < INPUT_URBS; ++j)
 				usb_kill_urb(ep->in->urbs[j]);
 	}
+	umidi->input_running = 0;
 }
 
 static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
@@ -2110,8 +2113,11 @@ void snd_usbmidi_input_start(struct list_head* p)
 	int i;
 
 	umidi = list_entry(p, struct snd_usb_midi, list);
+	if (umidi->input_running || !umidi->opened[1])
+		return;
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i)
 		snd_usbmidi_input_start_ep(umidi->endpoints[i].in);
+	umidi->input_running = 1;
 }
 
 /*
@@ -2250,9 +2256,6 @@ int snd_usbmidi_create(struct snd_card *card,
 	}
 
 	list_add_tail(&umidi->list, midi_list);
-
-	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i)
-		snd_usbmidi_input_start_ep(umidi->endpoints[i].in);
 	return 0;
 }
 
-- 
1.8.0.1

Re: [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input

[Clemens Ladisch]

Takashi Iwai wrote:
> The commit [88a8516a: ALSA: usbaudio: implement USB autosuspend] added
> the support of autopm for USB MIDI output, but it didn't take the MIDI
> input into account.
>
> This patch adds the following for fixing the autopm:
> - Manage the URB start at the first MIDI input stream open, instead of
>   the time of instance creation
> - Move autopm code to the common substream_open()
> - Make snd_usbmidi_input_start/_stop() more robust and add the running
>   state check
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ...
> +static int substream_open(struct snd_rawmidi_substream *substream, int dir,
> +			  int open)
>  {
>  	struct snd_usb_midi* umidi = substream->rmidi->private_data;
>  	struct snd_kcontrol *ctl;
> +	int err;
>
>  	down_read(&umidi->disc_rwsem);
>  	if (umidi->disconnected) {
>  		up_read(&umidi->disc_rwsem);
> +		return open ? -ENODEV : 0;
>  	}
>
>  	mutex_lock(&umidi->mutex);
>  	if (open) {
> +		if (!umidi->opened[0] && !umidi->opened[1]) {
> +			err = usb_autopm_get_interface(umidi->iface);
> +			umidi->autopm_reference = err >= 0;
> +			if (err < 0 && err != -EACCES) {
> +				up_read(&umidi->disc_rwsem);
> +				return -EIO;

umidi->mutex is still held here.

Otherwise, for both patches:
Reviewd-by: Clemens Ladisch <clemens@ladisch.de>
Tested-by: Clemens Ladisch <clemens@ladisch.de>


Regards,
Clemens

Re: [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input

[Takashi Iwai]

At Mon, 03 Dec 2012 21:47:48 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > The commit [88a8516a: ALSA: usbaudio: implement USB autosuspend] added
> > the support of autopm for USB MIDI output, but it didn't take the MIDI
> > input into account.
> >
> > This patch adds the following for fixing the autopm:
> > - Manage the URB start at the first MIDI input stream open, instead of
> >   the time of instance creation
> > - Move autopm code to the common substream_open()
> > - Make snd_usbmidi_input_start/_stop() more robust and add the running
> >   state check
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ...
> > +static int substream_open(struct snd_rawmidi_substream *substream, int dir,
> > +			  int open)
> >  {
> >  	struct snd_usb_midi* umidi = substream->rmidi->private_data;
> >  	struct snd_kcontrol *ctl;
> > +	int err;
> >
> >  	down_read(&umidi->disc_rwsem);
> >  	if (umidi->disconnected) {
> >  		up_read(&umidi->disc_rwsem);
> > +		return open ? -ENODEV : 0;
> >  	}
> >
> >  	mutex_lock(&umidi->mutex);
> >  	if (open) {
> > +		if (!umidi->opened[0] && !umidi->opened[1]) {
> > +			err = usb_autopm_get_interface(umidi->iface);
> > +			umidi->autopm_reference = err >= 0;
> > +			if (err < 0 && err != -EACCES) {
> > +				up_read(&umidi->disc_rwsem);
> > +				return -EIO;
> 
> umidi->mutex is still held here.

Right, fixed.

> Otherwise, for both patches:
> Reviewd-by: Clemens Ladisch <clemens@ladisch.de>
> Tested-by: Clemens Ladisch <clemens@ladisch.de>

OK, I merged the fixed patches and pushed out.

Thanks!


Takashi