[PATCH 0/2] USB MIDI fixes

Alsa-Devel Archive on lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/2] USB MIDI fixes
@ 2012-12-03 10:48 Takashi Iwai
  2012-12-03 10:48 ` [PATCH 1/2] ALSA: usb-audio: Avoid autopm calls after disconnection Takashi Iwai
  2012-12-03 10:48 ` [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input Takashi Iwai
  0 siblings, 2 replies; 5+ messages in thread
From: Takashi Iwai @ 2012-12-03 10:48 UTC (permalink / raw)
  To: alsa-devel; +Cc: Jonathan Nieder, David Banks, Olivier MATZ, Clemens Ladisch

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] ALSA: usb-audio: Avoid autopm calls after disconnection
  2012-12-03 10:48 [PATCH 0/2] USB MIDI fixes Takashi Iwai
@ 2012-12-03 10:48 ` Takashi Iwai
  2012-12-03 10:48 ` [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input Takashi Iwai
  1 sibling, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2012-12-03 10:48 UTC (permalink / raw)
  To: alsa-devel; +Cc: Jonathan Nieder, David Banks, Olivier MATZ, Clemens Ladisch

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input
  2012-12-03 10:48 [PATCH 0/2] USB MIDI fixes Takashi Iwai
  2012-12-03 10:48 ` [PATCH 1/2] ALSA: usb-audio: Avoid autopm calls after disconnection Takashi Iwai
@ 2012-12-03 10:48 ` Takashi Iwai
  2012-12-03 20:47   ` Clemens Ladisch
  1 sibling, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2012-12-03 10:48 UTC (permalink / raw)
  To: alsa-devel; +Cc: Jonathan Nieder, David Banks, Olivier MATZ, Clemens Ladisch

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input
  2012-12-03 10:48 ` [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input Takashi Iwai
@ 2012-12-03 20:47   ` Clemens Ladisch
  2012-12-04  7:01     ` Takashi Iwai
  0 siblings, 1 reply; 5+ messages in thread
From: Clemens Ladisch @ 2012-12-03 20:47 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Jonathan Nieder, David Banks, alsa-devel, Olivier MATZ

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input
  2012-12-03 20:47   ` Clemens Ladisch
@ 2012-12-04  7:01     ` Takashi Iwai
  0 siblings, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2012-12-04  7:01 UTC (permalink / raw)
  To: Clemens Ladisch; +Cc: Jonathan Nieder, David Banks, alsa-devel, Olivier MATZ

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-12-04  7:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-03 10:48 [PATCH 0/2] USB MIDI fixes Takashi Iwai
2012-12-03 10:48 ` [PATCH 1/2] ALSA: usb-audio: Avoid autopm calls after disconnection Takashi Iwai
2012-12-03 10:48 ` [PATCH 2/2] ALSA: usb-audio: Fix missing autopm for MIDI input Takashi Iwai
2012-12-03 20:47   ` Clemens Ladisch
2012-12-04  7:01     ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox