[PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Jonathan Nieder]

Some USB MIDI keyboards fail to operate after a USB autosuspend.  The
device is recognized by ALSA, but no events are received and the
device goes quiet.

	$ amidi -l
	Dir Device    Name
	IO  hw:1,0,0  Oxygen 49 MIDI 1
	$ amidi -p hw:1,0,0 -d
	<... play some notes ...>
	^C
	0 bytes read

A workaround is to disable USB autosuspend for these devices by
putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
/etc/laptop-mode/conf.d/usb-autosuspend.conf.  In the spirit of commit
166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
reset the device on resume so this workaround is not needed any more.

Noticed on a kernel close to 3.2.10.  Tested against 3.6.5.  Debian's
2.6.32.54-based kernel did not seem to be affected, though it's not
clear whether that is due to code or configuration.

Addresses http://bugs.debian.org/664068

Reported-and-tested-by: David Banks <amoebae@gmail.com> # Oxygen 49
Reported-and-tested-by: Olivier MATZ <zer0@droids-corp.org> # KeyRig 49
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cc: stable@vger.kernel.org
---
Thoughts?

 drivers/usb/core/quirks.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index fdefd9c7f7af..998c6a8290f6 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -105,6 +105,12 @@ static const struct usb_device_id usb_quirk_list[] = {
 	/* Midiman M-Audio Keystation 88es */
 	{ USB_DEVICE(0x0763, 0x0192), .driver_info = USB_QUIRK_RESET_RESUME },
 
+	/* Midiman M-Audio KeyRig 49 */
+	{ USB_DEVICE(0x0763, 0x019b), .driver_info = USB_QUIRK_RESET_RESUME },
+
+	/* Midiman M-Audio Oxygen 49 */
+	{ USB_DEVICE(0x0763, 0x2027), .driver_info = USB_QUIRK_RESET_RESUME },
+
 	/* M-Systems Flash Disk Pioneers */
 	{ USB_DEVICE(0x08ec, 0x1000), .driver_info = USB_QUIRK_RESET_RESUME },
 
-- 
1.8.0

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Clemens Ladisch]

Jonathan Nieder wrote:
> Some USB MIDI keyboards fail to operate after a USB autosuspend.

Make that *all* USB MIDI devices with input ports.

This is not a bug in the device, but one of the many bugs introduced
with the autosuspend code in <http://git.kernel.org/linus/88a8516a2128>.

That patch does not handle input at all, i.e., when the driver wants to
read from the device, it just doesn't take it out of suspend mode.

> A workaround is to disable USB autosuspend for these devices by
> putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
> /etc/laptop-mode/conf.d/usb-autosuspend.conf.  In the spirit of commit
> 166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
> reset the device on resume so this workaround is not needed any more.

It is not feasible to add the IDs of all USB MIDI devices.

I'm working on a fix that adds proper power management for input ports,
but this requires the driver to be reorganized a little ...


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Takashi Iwai]

At Sun, 25 Nov 2012 23:01:27 +0100,
Clemens Ladisch wrote:
> 
> Jonathan Nieder wrote:
> > Some USB MIDI keyboards fail to operate after a USB autosuspend.
> 
> Make that *all* USB MIDI devices with input ports.
> 
> This is not a bug in the device, but one of the many bugs introduced
> with the autosuspend code in <http://git.kernel.org/linus/88a8516a2128>.
> 
> That patch does not handle input at all, i.e., when the driver wants to
> read from the device, it just doesn't take it out of suspend mode.
> 
> > A workaround is to disable USB autosuspend for these devices by
> > putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
> > /etc/laptop-mode/conf.d/usb-autosuspend.conf.  In the spirit of commit
> > 166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
> > reset the device on resume so this workaround is not needed any more.
> 
> It is not feasible to add the IDs of all USB MIDI devices.
> 
> I'm working on a fix that adds proper power management for input ports,
> but this requires the driver to be reorganized a little ...

Doesn't a simple patch like below work?
(It even reduces more lines! :)


Takashi

---
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index eeefbce..2e0fabc 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -148,7 +148,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
@@ -1033,29 +1032,35 @@ 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 open)
 {
 	struct snd_usb_midi* umidi = substream->rmidi->private_data;
 	struct snd_kcontrol *ctl;
+	int err = 0;
 
 	mutex_lock(&umidi->mutex);
-	if (open) {
-		if (umidi->opened++ == 0 && umidi->roland_load_ctl) {
+	if (open && umidi->opened++ == 0) {
+		err = usb_autopm_get_interface(umidi->iface);
+		if (err == -EACCES)
+			err = 0;
+		if (!err && 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);
 		}
-	} else {
-		if (--umidi->opened == 0 && umidi->roland_load_ctl) {
+	} else if (!open && --umidi->opened == 0) {
+		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);
 		}
+		usb_autopm_put_interface(umidi->iface);
 	}
 	mutex_unlock(&umidi->mutex);
+	return err;
 }
 
 static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
@@ -1076,25 +1081,17 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
 		snd_BUG();
 		return -ENXIO;
 	}
-	err = usb_autopm_get_interface(umidi->iface);
-	port->autopm_reference = err >= 0;
-	if (err < 0 && err != -EACCES)
-		return -EIO;
+	err = substream_open(substream, 1);
+	if (err < 0)
+		return err;
 	substream->runtime->private_data = port;
 	port->state = STATE_UNKNOWN;
-	substream_open(substream, 1);
 	return 0;
 }
 
 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);
-	if (port->autopm_reference)
-		usb_autopm_put_interface(umidi->iface);
-	return 0;
+	return substream_open(substream, 0);
 }
 
 static void snd_usbmidi_output_trigger(struct snd_rawmidi_substream *substream, int up)
@@ -1147,14 +1144,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);
 }
 
 static int snd_usbmidi_input_close(struct snd_rawmidi_substream *substream)
 {
-	substream_open(substream, 0);
-	return 0;
+	return substream_open(substream, 0);
 }
 
 static void snd_usbmidi_input_trigger(struct snd_rawmidi_substream *substream, int up)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Clemens Ladisch]

Takashi Iwai wrote:> At Sun, 25 Nov 2012 23:01:27 +0100,
> Clemens Ladisch wrote:
>>
>> Jonathan Nieder wrote:
>>> Some USB MIDI keyboards fail to operate after a USB autosuspend.
>>
>> Make that *all* USB MIDI devices with input ports.
>>
>> This is not a bug in the device, but one of the many bugs introduced
>> with the autosuspend code in <http://git.kernel.org/linus/88a8516a2128>.
>>
>> That patch does not handle input at all, i.e., when the driver wants to
>> read from the device, it just doesn't take it out of suspend mode.
>>
>>> A workaround is to disable USB autosuspend for these devices by
>>> putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
>>> /etc/laptop-mode/conf.d/usb-autosuspend.conf.  In the spirit of commit
>>> 166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
>>> reset the device on resume so this workaround is not needed any more.
>>
>> It is not feasible to add the IDs of all USB MIDI devices.
>>
>> I'm working on a fix that adds proper power management for input ports,
>> but this requires the driver to be reorganized a little ...
>
> Doesn't a simple patch like below work?

> +static int substream_open(struct snd_rawmidi_substream *substream, int open)
>  {
> +	if (open && umidi->opened++ == 0) {
> +		err = usb_autopm_get_interface(umidi->iface);
>
>  static int snd_usbmidi_input_open(struct snd_rawmidi_substream *substream)
>  {
> +	return substream_open(substream, 1);

No, because the input URBs are submitted before the userspace device is
opened.  (And usb_autopm_get_interface() cannot be called from the USB
probe callback.)


Regards,
Clemens

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Takashi Iwai]

At Mon, 26 Nov 2012 13:35:31 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:> At Sun, 25 Nov 2012 23:01:27 +0100,
> > Clemens Ladisch wrote:
> >>
> >> Jonathan Nieder wrote:
> >>> Some USB MIDI keyboards fail to operate after a USB autosuspend.
> >>
> >> Make that *all* USB MIDI devices with input ports.
> >>
> >> This is not a bug in the device, but one of the many bugs introduced
> >> with the autosuspend code in <http://git.kernel.org/linus/88a8516a2128>.
> >>
> >> That patch does not handle input at all, i.e., when the driver wants to
> >> read from the device, it just doesn't take it out of suspend mode.
> >>
> >>> A workaround is to disable USB autosuspend for these devices by
> >>> putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
> >>> /etc/laptop-mode/conf.d/usb-autosuspend.conf.  In the spirit of commit
> >>> 166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
> >>> reset the device on resume so this workaround is not needed any more.
> >>
> >> It is not feasible to add the IDs of all USB MIDI devices.
> >>
> >> I'm working on a fix that adds proper power management for input ports,
> >> but this requires the driver to be reorganized a little ...
> >
> > Doesn't a simple patch like below work?
> 
> > +static int substream_open(struct snd_rawmidi_substream *substream, int open)
> >  {
> > +	if (open && umidi->opened++ == 0) {
> > +		err = usb_autopm_get_interface(umidi->iface);
> >
> >  static int snd_usbmidi_input_open(struct snd_rawmidi_substream *substream)
> >  {
> > +	return substream_open(substream, 1);
> 
> No, because the input URBs are submitted before the userspace device is
> opened.  (And usb_autopm_get_interface() cannot be called from the USB
> probe callback.)

Ah, right.  What's the reason of submitting input urbs for the all
time from the beginning?  For loopback?

If it has to be running, the easiest fix would be the patch like
below.  This will turn off the autopm essentially, but better than
breakage.


Takashi

---
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index eeefbce..66acccb 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -175,6 +175,7 @@ struct snd_usb_midi_in_endpoint {
 	u8 seen_f5;
 	u8 error_resubmit;
 	int current_port;
+	bool autopm_reference;
 };
 
 static void snd_usbmidi_do_output(struct snd_usb_midi_out_endpoint* ep);
@@ -2065,6 +2066,8 @@ void snd_usbmidi_input_stop(struct list_head* p)
 		if (ep->in)
 			for (j = 0; j < INPUT_URBS; ++j)
 				usb_kill_urb(ep->in->urbs[j]);
+		if (ep->autopm_reference)
+			usb_autopm_put_interface(umidi->iface);
 	}
 }
 
@@ -2074,6 +2077,8 @@ static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
 
 	if (!ep)
 		return;
+	ep->autopm_reference =
+		usb_autopm_get_interface(ep->umidi->iface) >= 0;
 	for (i = 0; i < INPUT_URBS; ++i) {
 		struct urb* urb = ep->urbs[i];
 		urb->dev = ep->umidi->dev;

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Clemens Ladisch]

Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Takashi Iwai wrote:
>>> Clemens Ladisch wrote:
>>>> I'm working on a fix that adds proper power management for input ports,
>>>> but this requires the driver to be reorganized a little ...
>>>
>>> Doesn't a simple patch like below work?
>>
>>> +static int substream_open(struct snd_rawmidi_substream *substream, int open)
>>>  {
>>> +	if (open && umidi->opened++ == 0) {
>>> +		err = usb_autopm_get_interface(umidi->iface);
>>>
>>>  static int snd_usbmidi_input_open(struct snd_rawmidi_substream *substream)
>>>  {
>>> +	return substream_open(substream, 1);
>>
>> No, because the input URBs are submitted before the userspace device is
>> opened.
>
> Ah, right.  What's the reason of submitting input urbs for the all
> time from the beginning?  For loopback?

For not needing to count open input ports.

> If it has to be running, the easiest fix would be the patch like
> below.  This will turn off the autopm essentially, but better than
> breakage.
>
> @@ -2074,6 +2077,8 @@ static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
>
> +	ep->autopm_reference =
> +		usb_autopm_get_interface(ep->umidi->iface) >= 0;

usb_autopm_get_interface() cannot be called from the USB probe callback.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Oliver Neukum]

On Monday 26 November 2012 14:43:13 Clemens Ladisch wrote:

> > If it has to be running, the easiest fix would be the patch like
> > below.  This will turn off the autopm essentially, but better than
> > breakage.
> >
> > @@ -2074,6 +2077,8 @@ static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
> >
> > +	ep->autopm_reference =
> > +		usb_autopm_get_interface(ep->umidi->iface) >= 0;
> 
> usb_autopm_get_interface() cannot be called from the USB probe callback.

You can use usb_autopm_get_interface_no_resume()
During probe() the device is known to not be suspended.

	Regards
		Oliver

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Alan Stern]

On Mon, 26 Nov 2012, Oliver Neukum wrote:

> On Monday 26 November 2012 14:43:13 Clemens Ladisch wrote:
> 
> > > If it has to be running, the easiest fix would be the patch like
> > > below.  This will turn off the autopm essentially, but better than
> > > breakage.
> > >
> > > @@ -2074,6 +2077,8 @@ static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
> > >
> > > +	ep->autopm_reference =
> > > +		usb_autopm_get_interface(ep->umidi->iface) >= 0;
> > 
> > usb_autopm_get_interface() cannot be called from the USB probe callback.
> 
> You can use usb_autopm_get_interface_no_resume()
> During probe() the device is known to not be suspended.

In fact, as far as I know you _can_ use usb_autopm_get_interface() from
within a probe routine.  Is there some problem with it I'm not aware of?

Alan Stern

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Clemens Ladisch]

Alan Stern wrote:
> On Mon, 26 Nov 2012, Oliver Neukum wrote:
>> On Monday 26 November 2012 14:43:13 Clemens Ladisch wrote:
>>> usb_autopm_get_interface() cannot be called from the USB probe callback.
>>
>> You can use usb_autopm_get_interface_no_resume()
>> During probe() the device is known to not be suspended.
>
> In fact, as far as I know you _can_ use usb_autopm_get_interface() from
> within a probe routine.  Is there some problem with it I'm not aware of?

It returns -EACCES.

But _no_resume() appears to work just fine.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Alan Stern]

On Mon, 26 Nov 2012, Clemens Ladisch wrote:

> Alan Stern wrote:
> > On Mon, 26 Nov 2012, Oliver Neukum wrote:
> >> On Monday 26 November 2012 14:43:13 Clemens Ladisch wrote:
> >>> usb_autopm_get_interface() cannot be called from the USB probe callback.
> >>
> >> You can use usb_autopm_get_interface_no_resume()
> >> During probe() the device is known to not be suspended.
> >
> > In fact, as far as I know you _can_ use usb_autopm_get_interface() from
> > within a probe routine.  Is there some problem with it I'm not aware of?
> 
> It returns -EACCES.

That's odd.  -EACCES occurs only when the interface or the device it
belongs to is disabled for runtime PM.  But the USB core enables
interfaces before probing, provided the driver's supports_autosuspend
flag is set.

Furthermore, -EACCES can occur in usb_autopm_get_interface() only when 
the interface or its device is not currently resumed -- but USB devices 
and interfaces are always resumed during a probe.

Do you have any idea about what's going wrong here?  Or do I need to 
investigate more closely?

Alan Stern

Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49

[Takashi Iwai]

At Mon, 26 Nov 2012 14:43:13 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Clemens Ladisch wrote:
> >> Takashi Iwai wrote:
> >>> Clemens Ladisch wrote:
> >>>> I'm working on a fix that adds proper power management for input ports,
> >>>> but this requires the driver to be reorganized a little ...
> >>>
> >>> Doesn't a simple patch like below work?
> >>
> >>> +static int substream_open(struct snd_rawmidi_substream *substream, int open)
> >>>  {
> >>> +	if (open && umidi->opened++ == 0) {
> >>> +		err = usb_autopm_get_interface(umidi->iface);
> >>>
> >>>  static int snd_usbmidi_input_open(struct snd_rawmidi_substream *substream)
> >>>  {
> >>> +	return substream_open(substream, 1);
> >>
> >> No, because the input URBs are submitted before the userspace device is
> >> opened.
> >
> > Ah, right.  What's the reason of submitting input urbs for the all
> > time from the beginning?  For loopback?
> 
> For not needing to count open input ports.

So we've spun urbs for all the time just for reducing refcount?
That's bad.

> > If it has to be running, the easiest fix would be the patch like
> > below.  This will turn off the autopm essentially, but better than
> > breakage.
> >
> > @@ -2074,6 +2077,8 @@ static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
> >
> > +	ep->autopm_reference =
> > +		usb_autopm_get_interface(ep->umidi->iface) >= 0;
> 
> usb_autopm_get_interface() cannot be called from the USB probe callback.

OK, it can call no_resume version by passing an argument.

But judging from your comment above, we should fix the free wheel MIDI
input urbs at first indeed, not only for autopm but in general.


Takashi