Re: [alsa-devel] Fwd: Regression: Line6 Toneport stops working on 4.10-rc5
On Mon, Feb 06, 2017 at 01:01:56PM +0300, Igor Zinovev wrote:
Hello again!
Thanks for the advice, but bisecting the whole tree took me quite a while and I ended up stuck because booting on some bisect states has led my system to run without some devices, like wireless and sound. So I tried to bisect the sound/usb/line6 path. It turns out that this commit is the point where it breaks for me: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound...
In fact, when I apply the reverse patch on top of 4.10-rc7, it fixes the problem for me.
Wonderful, thanks so much for tracking this down!
Should I bring some of the people that are mentioned in the commit into this conversation?
I've done so now. Andrej and Takashi, the above patch causes a regression for Igor, any thoughts? Should we just revert it?
thanks,
greg k-h
On Mon, 06 Feb 2017 11:18:44 +0100, Greg KH wrote:
On Mon, Feb 06, 2017 at 01:01:56PM +0300, Igor Zinovev wrote:
Hello again!
Thanks for the advice, but bisecting the whole tree took me quite a while and I ended up stuck because booting on some bisect states has led my system to run without some devices, like wireless and sound. So I tried to bisect the sound/usb/line6 path. It turns out that this commit is the point where it breaks for me: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound...
In fact, when I apply the reverse patch on top of 4.10-rc7, it fixes the problem for me.
Wonderful, thanks so much for tracking this down!
Should I bring some of the people that are mentioned in the commit into this conversation?
I've done so now. Andrej and Takashi, the above patch causes a regression for Igor, any thoughts? Should we just revert it?
Yeah, the patch looks wrong, the function line6_get_interval() should be called unconditionally.
I'm going to queue a revert for 4.10-final.
thanks,
Takashi
On Mon, Feb 6, 2017 at 12:01 PM, Takashi Iwai tiwai@suse.de wrote:
On Mon, 06 Feb 2017 11:18:44 +0100, Greg KH wrote:
On Mon, Feb 06, 2017 at 01:01:56PM +0300, Igor Zinovev wrote:
Hello again!
Thanks for the advice, but bisecting the whole tree took me quite a while and I ended up stuck because booting on some bisect states has led my system to run without some devices, like wireless and sound. So I tried to bisect the sound/usb/line6 path. It turns out that this commit is the point where it breaks for me: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound...
In fact, when I apply the reverse patch on top of 4.10-rc7, it fixes the problem for me.
Wonderful, thanks so much for tracking this down!
Should I bring some of the people that are mentioned in the commit into this conversation?
I've done so now. Andrej and Takashi, the above patch causes a regression for Igor, any thoughts? Should we just revert it?
Yeah, the patch looks wrong, the function line6_get_interval() should be called unconditionally.
I'm going to queue a revert for 4.10-final.
Yep, on the first look it's suspicious. I'll have a look on it in the evening - not sure simple revert is the ideal solution... I'll get back to you.
PS: Thanks for the bisecting, Igor! PPS: Maybe this should go also to 4.9 stable, once resolved...
On Mon, 06 Feb 2017 12:04:29 +0100, Andrej Kruták wrote:
On Mon, Feb 6, 2017 at 12:01 PM, Takashi Iwai tiwai@suse.de wrote:
On Mon, 06 Feb 2017 11:18:44 +0100, Greg KH wrote:
On Mon, Feb 06, 2017 at 01:01:56PM +0300, Igor Zinovev wrote:
Hello again!
Thanks for the advice, but bisecting the whole tree took me quite a while and I ended up stuck because booting on some bisect states has led my system to run without some devices, like wireless and sound. So I tried to bisect the sound/usb/line6 path. It turns out that this commit is the point where it breaks for me: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound...
In fact, when I apply the reverse patch on top of 4.10-rc7, it fixes the problem for me.
Wonderful, thanks so much for tracking this down!
Should I bring some of the people that are mentioned in the commit into this conversation?
I've done so now. Andrej and Takashi, the above patch causes a regression for Igor, any thoughts? Should we just revert it?
Yeah, the patch looks wrong, the function line6_get_interval() should be called unconditionally.
I'm going to queue a revert for 4.10-final.
Yep, on the first look it's suspicious. I'll have a look on it in the evening - not sure simple revert is the ideal solution... I'll get back to you.
I guess in this case the revert is the best option. But I'll postpone it until tomorrow. If you find a better solution, please let me know.
PS: Thanks for the bisecting, Igor! PPS: Maybe this should go also to 4.9 stable, once resolved...
Yes, definitely.
thanks,
Takashi
While not all line6 devices currently support PCM, it causes no harm to 'have it prepared'.
This also fixes toneport, which only has PCM - in which case we previously skipped the USB transfer properties detection completely.
Signed-off-by: Andrej Krutak dev@andree.sk --- sound/usb/line6/driver.c | 49 ++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 90009c0..0ff5a7d 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -492,42 +492,46 @@ static void line6_destruct(struct snd_card *card) usb_put_dev(usbdev); }
-/* get data from endpoint descriptor (see usb_maxpacket): */ -static void line6_get_interval(struct usb_line6 *line6) +static void line6_get_usb_properties(struct usb_line6 *line6) { struct usb_device *usbdev = line6->usbdev; const struct line6_properties *properties = line6->properties; int pipe; - struct usb_host_endpoint *ep; + struct usb_host_endpoint *ep = NULL;
- if (properties->capabilities & LINE6_CAP_CONTROL_MIDI) { - pipe = - usb_rcvintpipe(line6->usbdev, line6->properties->ep_ctrl_r); - } else { - pipe = - usb_rcvbulkpipe(line6->usbdev, line6->properties->ep_ctrl_r); + if (properties->capabilities & LINE6_CAP_CONTROL) { + if (properties->capabilities & LINE6_CAP_CONTROL_MIDI) { + pipe = usb_rcvintpipe(line6->usbdev, + line6->properties->ep_ctrl_r); + } else { + pipe = usb_rcvbulkpipe(line6->usbdev, + line6->properties->ep_ctrl_r); + } + ep = usbdev->ep_in[usb_pipeendpoint(pipe)]; } - ep = usbdev->ep_in[usb_pipeendpoint(pipe)];
+ /* Control data transfer properties */ if (ep) { line6->interval = ep->desc.bInterval; - if (usbdev->speed == USB_SPEED_LOW) { - line6->intervals_per_second = USB_LOW_INTERVALS_PER_SECOND; - line6->iso_buffers = USB_LOW_ISO_BUFFERS; - } else { - line6->intervals_per_second = USB_HIGH_INTERVALS_PER_SECOND; - line6->iso_buffers = USB_HIGH_ISO_BUFFERS; - } - line6->max_packet_size = le16_to_cpu(ep->desc.wMaxPacketSize); } else { - dev_err(line6->ifcdev, - "endpoint not available, using fallback values"); + if (properties->capabilities & LINE6_CAP_CONTROL) { + dev_err(line6->ifcdev, + "endpoint not available, using fallback values"); + } line6->interval = LINE6_FALLBACK_INTERVAL; line6->max_packet_size = LINE6_FALLBACK_MAXPACKETSIZE; } -}
+ /* Isochronous transfer properties */ + if (usbdev->speed == USB_SPEED_LOW) { + line6->intervals_per_second = USB_LOW_INTERVALS_PER_SECOND; + line6->iso_buffers = USB_LOW_ISO_BUFFERS; + } else { + line6->intervals_per_second = USB_HIGH_INTERVALS_PER_SECOND; + line6->iso_buffers = USB_HIGH_ISO_BUFFERS; + } +}
/* Enable buffering of incoming messages, flush the buffer */ static int line6_hwdep_open(struct snd_hwdep *hw, struct file *file) @@ -754,8 +758,9 @@ int line6_probe(struct usb_interface *interface, goto error; }
+ line6_get_usb_properties(line6); + if (properties->capabilities & LINE6_CAP_CONTROL) { - line6_get_interval(line6); ret = line6_init_cap_control(line6); if (ret < 0) goto error;
On Mon, 06 Feb 2017 20:34:58 +0100, Andrej Krutak wrote:
While not all line6 devices currently support PCM, it causes no harm to 'have it prepared'.
This also fixes toneport, which only has PCM - in which case we previously skipped the USB transfer properties detection completely.
Signed-off-by: Andrej Krutak dev@andree.sk
Well, this looks too complex and vague as a regression fix. If it were 4.10-rc1, I could take it. But now it's already rc7 and the next might be final, so I prefer a shorter and easier solution.
That said, we can revert the commit as a clear fix at first for 4.10 (with Cc to stable), then take this on top as an enhancement for 4.11.
Still it's important to know whether this works on Igor's system, so Igor, please give this one try.
thanks,
Takashi
sound/usb/line6/driver.c | 49 ++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 90009c0..0ff5a7d 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -492,42 +492,46 @@ static void line6_destruct(struct snd_card *card) usb_put_dev(usbdev); }
-/* get data from endpoint descriptor (see usb_maxpacket): */ -static void line6_get_interval(struct usb_line6 *line6) +static void line6_get_usb_properties(struct usb_line6 *line6) { struct usb_device *usbdev = line6->usbdev; const struct line6_properties *properties = line6->properties; int pipe;
- struct usb_host_endpoint *ep;
- struct usb_host_endpoint *ep = NULL;
- if (properties->capabilities & LINE6_CAP_CONTROL_MIDI) {
pipe =usb_rcvintpipe(line6->usbdev, line6->properties->ep_ctrl_r);- } else {
pipe =usb_rcvbulkpipe(line6->usbdev, line6->properties->ep_ctrl_r);
- if (properties->capabilities & LINE6_CAP_CONTROL) {
if (properties->capabilities & LINE6_CAP_CONTROL_MIDI) {pipe = usb_rcvintpipe(line6->usbdev,line6->properties->ep_ctrl_r);} else {pipe = usb_rcvbulkpipe(line6->usbdev,line6->properties->ep_ctrl_r);}ep = usbdev->ep_in[usb_pipeendpoint(pipe)];
- ep = usbdev->ep_in[usb_pipeendpoint(pipe)];
- /* Control data transfer properties */ if (ep) { line6->interval = ep->desc.bInterval;
if (usbdev->speed == USB_SPEED_LOW) {line6->intervals_per_second = USB_LOW_INTERVALS_PER_SECOND;line6->iso_buffers = USB_LOW_ISO_BUFFERS;} else {line6->intervals_per_second = USB_HIGH_INTERVALS_PER_SECOND;line6->iso_buffers = USB_HIGH_ISO_BUFFERS;}- line6->max_packet_size = le16_to_cpu(ep->desc.wMaxPacketSize); } else {
dev_err(line6->ifcdev,"endpoint not available, using fallback values");
if (properties->capabilities & LINE6_CAP_CONTROL) {dev_err(line6->ifcdev,"endpoint not available, using fallback values");}-}
- /* Isochronous transfer properties */
- if (usbdev->speed == USB_SPEED_LOW) {
line6->intervals_per_second = USB_LOW_INTERVALS_PER_SECOND;line6->iso_buffers = USB_LOW_ISO_BUFFERS;- } else {
line6->intervals_per_second = USB_HIGH_INTERVALS_PER_SECOND;line6->iso_buffers = USB_HIGH_ISO_BUFFERS;- }
+}
/* Enable buffering of incoming messages, flush the buffer */ static int line6_hwdep_open(struct snd_hwdep *hw, struct file *file) @@ -754,8 +758,9 @@ int line6_probe(struct usb_interface *interface, goto error; }
- line6_get_usb_properties(line6);
- if (properties->capabilities & LINE6_CAP_CONTROL) {
line6_get_interval(line6);-- 2.7.4
On Mon, Feb 6, 2017 at 10:19 PM, Takashi Iwai tiwai@suse.de wrote:
On Mon, 06 Feb 2017 20:34:58 +0100, Andrej Krutak wrote:
While not all line6 devices currently support PCM, it causes no harm to 'have it prepared'.
This also fixes toneport, which only has PCM - in which case we previously skipped the USB transfer properties detection completely.
Signed-off-by: Andrej Krutak dev@andree.sk
Well, this looks too complex and vague as a regression fix. If it were 4.10-rc1, I could take it. But now it's already rc7 and the next might be final, so I prefer a shorter and easier solution.
In the end, the patch is just "careful", so that the endpoint structures are not read if the devices is not "marked" as having them.
That said, we can revert the commit as a clear fix at first for 4.10 (with Cc to stable), then take this on top as an enhancement for 4.11.
Still it's important to know whether this works on Igor's system, so Igor, please give this one try.
Thanks to zero-init of toneport_properties_table, the proposed simple revert should be enough for toneport, and shouldn't break other devices (perhaps HD300-HD500 may start showing the dev_err from line6_get_interval()).
So do as you like - e.g. simple revert for 4.10 and apply this patch for 4.11. But I don't mind if you won't.... :-)
On Mon, 06 Feb 2017 22:54:59 +0100, Andrej Kruták wrote:
On Mon, Feb 6, 2017 at 10:19 PM, Takashi Iwai tiwai@suse.de wrote:
On Mon, 06 Feb 2017 20:34:58 +0100, Andrej Krutak wrote:
While not all line6 devices currently support PCM, it causes no harm to 'have it prepared'.
This also fixes toneport, which only has PCM - in which case we previously skipped the USB transfer properties detection completely.
Signed-off-by: Andrej Krutak dev@andree.sk
Well, this looks too complex and vague as a regression fix. If it were 4.10-rc1, I could take it. But now it's already rc7 and the next might be final, so I prefer a shorter and easier solution.
In the end, the patch is just "careful", so that the endpoint structures are not read if the devices is not "marked" as having them.
Yes, but it's still not very clear what actually "fixes".
That said, we can revert the commit as a clear fix at first for 4.10 (with Cc to stable), then take this on top as an enhancement for 4.11.
Still it's important to know whether this works on Igor's system, so Igor, please give this one try.
Thanks to zero-init of toneport_properties_table, the proposed simple revert should be enough for toneport, and shouldn't break other devices (perhaps HD300-HD500 may start showing the dev_err from line6_get_interval()).
So do as you like - e.g. simple revert for 4.10 and apply this patch for 4.11. But I don't mind if you won't.... :-)
Yes, I now queued the revert patch to for-linus branch, and yours to for-next branch on top of it.
thanks,
Takashi
participants (3)
-
Andrej Kruták -
Greg KH -
Takashi Iwai