Re: Fwd: Regression: Line6 Toneport stops working on 4.10-rc5

Re: Fwd: Regression: Line6 Toneport stops working on 4.10-rc5

[Greg KH]

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/usb/line6?id=f6a0dd107ad0c8b59d1c9735eea4b8cb9f460949
> 
> 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
--
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: Fwd: Regression: Line6 Toneport stops working on 4.10-rc5

[Takashi Iwai]

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/usb/line6?id=f6a0dd107ad0c8b59d1c9735eea4b8cb9f460949
> > 
> > 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

Re: Fwd: Regression: Line6 Toneport stops working on 4.10-rc5

[Andrej Kruták]

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/usb/line6?id=f6a0dd107ad0c8b59d1c9735eea4b8cb9f460949
>> >
>> > 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...


-- 
Greetings,

Andrej

Re: Fwd: Regression: Line6 Toneport stops working on 4.10-rc5

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org> 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/usb/line6?id=f6a0dd107ad0c8b59d1c9735eea4b8cb9f460949
> >> >
> >> > 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
--
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

[PATCH] ALSA: line6: Always setup isochronous transfer properties

[Andrej Krutak]

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;
-- 
2.7.4

Re: [PATCH] ALSA: line6: Always setup isochronous transfer properties

[Takashi Iwai]

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");
> +		}
>  		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;
> -- 
> 2.7.4
> 

Re: [PATCH] ALSA: line6: Always setup isochronous transfer properties

[Andrej Kruták]

On Mon, Feb 6, 2017 at 10:19 PM, Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org> 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-/tK0A0Qp8GRaa/9Udqfwiw@public.gmane.org>
>
> 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.... :-)


-- 
Andrej

>
>> ---
>>  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;
>> --
>> 2.7.4
>>
--
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: [PATCH] ALSA: line6: Always setup isochronous transfer properties

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel