[PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Daniel Mack]

The frame check in i_usX2Y_urb_complete() and
i_usX2Y_usbpcm_urb_complete() is bogus and produces false positives as
described in this LAU thread:

  http://linuxaudio.org/mailarchive/lau/2013/5/20/200177

This patch removes the check code entirely.

Cc: fzu@wemgehoertderstaat.de
Reported-by: Dr Nicholas J Bailey <nicholas.bailey@glasgow.ac.uk>
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 sound/usb/usx2y/usbusx2yaudio.c | 22 +++-------------------
 sound/usb/usx2y/usx2yhwdeppcm.c |  7 +------
 2 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 63fb521..6234a51 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -299,19 +299,6 @@ static void usX2Y_error_urb_status(struct usX2Ydev *usX2Y,
 	usX2Y_clients_stop(usX2Y);
 }
 
-static void usX2Y_error_sequence(struct usX2Ydev *usX2Y,
-				 struct snd_usX2Y_substream *subs, struct urb *urb)
-{
-	snd_printk(KERN_ERR
-"Sequence Error!(hcd_frame=%i ep=%i%s;wait=%i,frame=%i).\n"
-"Most probably some urb of usb-frame %i is still missing.\n"
-"Cause could be too long delays in usb-hcd interrupt handling.\n",
-		   usb_get_current_frame_number(usX2Y->dev),
-		   subs->endpoint, usb_pipein(urb->pipe) ? "in" : "out",
-		   usX2Y->wait_iso_frame, urb->start_frame, usX2Y->wait_iso_frame);
-	usX2Y_clients_stop(usX2Y);
-}
-
 static void i_usX2Y_urb_complete(struct urb *urb)
 {
 	struct snd_usX2Y_substream *subs = urb->context;
@@ -328,12 +315,9 @@ static void i_usX2Y_urb_complete(struct urb *urb)
 		usX2Y_error_urb_status(usX2Y, subs, urb);
 		return;
 	}
-	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame & 0xFFFF)))
-		subs->completed_urb = urb;
-	else {
-		usX2Y_error_sequence(usX2Y, subs, urb);
-		return;
-	}
+
+	subs->completed_urb = urb;
+
 	{
 		struct snd_usX2Y_substream *capsubs = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE],
 			*playbacksubs = usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c
index f2a1acd..814d0e8 100644
--- a/sound/usb/usx2y/usx2yhwdeppcm.c
+++ b/sound/usb/usx2y/usx2yhwdeppcm.c
@@ -244,13 +244,8 @@ static void i_usX2Y_usbpcm_urb_complete(struct urb *urb)
 		usX2Y_error_urb_status(usX2Y, subs, urb);
 		return;
 	}
-	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame & 0xFFFF)))
-		subs->completed_urb = urb;
-	else {
-		usX2Y_error_sequence(usX2Y, subs, urb);
-		return;
-	}
 
+	subs->completed_urb = urb;
 	capsubs = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE];
 	capsubs2 = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE + 2];
 	playbacksubs = usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
-- 
1.8.3.1

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks -- SUCCESS

[Dr Nicholas J Bailey]

I applied the patch by hand to my current source tree (3.10.11) and compiled 
the kernel from scratch.

I plugged a MIDI cable between midi in and out on the TASCAM US-122. Using 
QJackCtl, I connected the US-122's MIDI out to fluidsynth, and used amidiplay 
to send play a MIDI file to the US-122.

While the MIDI was playing, I used Audacity to record the audio via the 
US-122's mic inputs holding the mic close to the speakers. After about 10 
seconds, I rewound audacity and hit record again so that it was both playing 
and recording through the US-122 while the US-122 was also routing MIDI.

This appears to work fine (at 44100Hz sampling; I've not tried any others), and 
I reckon this is a pretty severe test.

Thank you for pushing this patch which seems to resolve the original issue. 
This will be particularly appreciated by many because the US-122, no longer 
supported for Windows I believe, is currently available on eBay for about £30 
in the UK, and represents an inexpensive way for Linux users to make 
reasonable quality recordings from microphones needing a phantom power source.


On Wednesday 02 October 2013 16:49:50 Daniel Mack wrote:
> The frame check in i_usX2Y_urb_complete() and
> i_usX2Y_usbpcm_urb_complete() is bogus and produces false positives as
> described in this LAU thread:
> 
>   http://linuxaudio.org/mailarchive/lau/2013/5/20/200177
> 
> This patch removes the check code entirely.
> 
> Cc: fzu@wemgehoertderstaat.de
> Reported-by: Dr Nicholas J Bailey <nicholas.bailey@glasgow.ac.uk>
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  sound/usb/usx2y/usbusx2yaudio.c | 22 +++-------------------
>  sound/usb/usx2y/usx2yhwdeppcm.c |  7 +------
>  2 files changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c
> b/sound/usb/usx2y/usbusx2yaudio.c index 63fb521..6234a51 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -299,19 +299,6 @@ static void usX2Y_error_urb_status(struct usX2Ydev
> *usX2Y, usX2Y_clients_stop(usX2Y);
>  }
> 
> -static void usX2Y_error_sequence(struct usX2Ydev *usX2Y,
> -				 struct snd_usX2Y_substream *subs, struct urb *urb)
> -{
> -	snd_printk(KERN_ERR
> -"Sequence Error!(hcd_frame=%i ep=%i%s;wait=%i,frame=%i).\n"
> -"Most probably some urb of usb-frame %i is still missing.\n"
> -"Cause could be too long delays in usb-hcd interrupt handling.\n",
> -		   usb_get_current_frame_number(usX2Y->dev),
> -		   subs->endpoint, usb_pipein(urb->pipe) ? "in" : "out",
> -		   usX2Y->wait_iso_frame, urb->start_frame, usX2Y->wait_iso_frame);
> -	usX2Y_clients_stop(usX2Y);
> -}
> -
>  static void i_usX2Y_urb_complete(struct urb *urb)
>  {
>  	struct snd_usX2Y_substream *subs = urb->context;
> @@ -328,12 +315,9 @@ static void i_usX2Y_urb_complete(struct urb *urb)
>  		usX2Y_error_urb_status(usX2Y, subs, urb);
>  		return;
>  	}
> -	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame &
> 0xFFFF))) -		subs->completed_urb = urb;
> -	else {
> -		usX2Y_error_sequence(usX2Y, subs, urb);
> -		return;
> -	}
> +
> +	subs->completed_urb = urb;
> +
>  	{
>  		struct snd_usX2Y_substream *capsubs =
> usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE], *playbacksubs =
> usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
> diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c
> b/sound/usb/usx2y/usx2yhwdeppcm.c index f2a1acd..814d0e8 100644
> --- a/sound/usb/usx2y/usx2yhwdeppcm.c
> +++ b/sound/usb/usx2y/usx2yhwdeppcm.c
> @@ -244,13 +244,8 @@ static void i_usX2Y_usbpcm_urb_complete(struct urb
> *urb) usX2Y_error_urb_status(usX2Y, subs, urb);
>  		return;
>  	}
> -	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame &
> 0xFFFF))) -		subs->completed_urb = urb;
> -	else {
> -		usX2Y_error_sequence(usX2Y, subs, urb);
> -		return;
> -	}
> 
> +	subs->completed_urb = urb;
>  	capsubs = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE];
>  	capsubs2 = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE + 2];
>  	playbacksubs = usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Guido Aulisi]

> Next time, when detecting such problems, please consider sending a
patch
> to alsa-devel, so fixes make it to the users eventually :)

You are right, but I thought it was a problem with a broken/weird usb
controller, because it worked with my old laptop.
When I saw another user with the same problem, I sent the patch to LAU
(the only mailing list I've subscribed to).

Now I remember why I did that patch: if you see my dmesg output without
that patch, you can see that the driver is comparing 1024 to 0. If you
check the history of the driver, you can see that this check has already
been narrowed with the bit AND operation; I tried to narrow the check a
little more, so I chose 0x3ff because 1024 & 0x3ff = 0.
But I forgot about my Tascam (because I bought a RME Raydat) and never
tested that patch, but I was quite confident that it should work.

[  669.312765] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
here with counts=42
[  669.329821] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
here with counts=42
[  669.345854] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
here with counts=42
[  669.471627] ALSA sound/usb/usx2y/usbusx2yaudio.c:311 Sequence
Error!(hcd_frame=3 ep=10out;wait=1024,frame=0).
Most probably some urb of usb-frame 1024 is still missing.
Cause could be too long delays in usb-hcd interrupt handling.
[ 1203.531392] ALSA sound/usb/usx2y/usbusx2yaudio.c:311 Sequence
Error!(hcd_frame=4 ep=8in;wait=1025,frame=1).
Most probably some urb of usb-frame 1025 is still missing.
Cause could be too long delays in usb-hcd interrupt handling.
[ 1218.908160] ALSA sound/usb/usx2y/usbusx2yaudio.c:311 Sequence
Error!(hcd_frame=5 ep=8in;wait=1026,frame=2).
Most probably some urb of usb-frame 1026 is still missing.

Ciao a tutti
Guido

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks -- SUCCESS

[Takashi Iwai]

At Thu, 3 Oct 2013 11:25:03 +0100,
Dr Nicholas J Bailey wrote:
> 
> I applied the patch by hand to my current source tree (3.10.11) and compiled 
> the kernel from scratch.
> 
> I plugged a MIDI cable between midi in and out on the TASCAM US-122. Using 
> QJackCtl, I connected the US-122's MIDI out to fluidsynth, and used amidiplay 
> to send play a MIDI file to the US-122.
> 
> While the MIDI was playing, I used Audacity to record the audio via the 
> US-122's mic inputs holding the mic close to the speakers. After about 10 
> seconds, I rewound audacity and hit record again so that it was both playing 
> and recording through the US-122 while the US-122 was also routing MIDI.
> 
> This appears to work fine (at 44100Hz sampling; I've not tried any others), and 
> I reckon this is a pretty severe test.
> 
> Thank you for pushing this patch which seems to resolve the original issue. 
> This will be particularly appreciated by many because the US-122, no longer 
> supported for Windows I believe, is currently available on eBay for about £30 
> in the UK, and represents an inexpensive way for Linux users to make 
> reasonable quality recordings from microphones needing a phantom power source.

OK, applied the patch now with Cc to stable.


thanks,

Takashi

> 
> 
> On Wednesday 02 October 2013 16:49:50 Daniel Mack wrote:
> > The frame check in i_usX2Y_urb_complete() and
> > i_usX2Y_usbpcm_urb_complete() is bogus and produces false positives as
> > described in this LAU thread:
> > 
> >   http://linuxaudio.org/mailarchive/lau/2013/5/20/200177
> > 
> > This patch removes the check code entirely.
> > 
> > Cc: fzu@wemgehoertderstaat.de
> > Reported-by: Dr Nicholas J Bailey <nicholas.bailey@glasgow.ac.uk>
> > Suggested-by: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Daniel Mack <zonque@gmail.com>
> > ---
> >  sound/usb/usx2y/usbusx2yaudio.c | 22 +++-------------------
> >  sound/usb/usx2y/usx2yhwdeppcm.c |  7 +------
> >  2 files changed, 4 insertions(+), 25 deletions(-)
> > 
> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c
> > b/sound/usb/usx2y/usbusx2yaudio.c index 63fb521..6234a51 100644
> > --- a/sound/usb/usx2y/usbusx2yaudio.c
> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
> > @@ -299,19 +299,6 @@ static void usX2Y_error_urb_status(struct usX2Ydev
> > *usX2Y, usX2Y_clients_stop(usX2Y);
> >  }
> > 
> > -static void usX2Y_error_sequence(struct usX2Ydev *usX2Y,
> > -				 struct snd_usX2Y_substream *subs, struct urb *urb)
> > -{
> > -	snd_printk(KERN_ERR
> > -"Sequence Error!(hcd_frame=%i ep=%i%s;wait=%i,frame=%i).\n"
> > -"Most probably some urb of usb-frame %i is still missing.\n"
> > -"Cause could be too long delays in usb-hcd interrupt handling.\n",
> > -		   usb_get_current_frame_number(usX2Y->dev),
> > -		   subs->endpoint, usb_pipein(urb->pipe) ? "in" : "out",
> > -		   usX2Y->wait_iso_frame, urb->start_frame, usX2Y->wait_iso_frame);
> > -	usX2Y_clients_stop(usX2Y);
> > -}
> > -
> >  static void i_usX2Y_urb_complete(struct urb *urb)
> >  {
> >  	struct snd_usX2Y_substream *subs = urb->context;
> > @@ -328,12 +315,9 @@ static void i_usX2Y_urb_complete(struct urb *urb)
> >  		usX2Y_error_urb_status(usX2Y, subs, urb);
> >  		return;
> >  	}
> > -	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame &
> > 0xFFFF))) -		subs->completed_urb = urb;
> > -	else {
> > -		usX2Y_error_sequence(usX2Y, subs, urb);
> > -		return;
> > -	}
> > +
> > +	subs->completed_urb = urb;
> > +
> >  	{
> >  		struct snd_usX2Y_substream *capsubs =
> > usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE], *playbacksubs =
> > usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
> > diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c
> > b/sound/usb/usx2y/usx2yhwdeppcm.c index f2a1acd..814d0e8 100644
> > --- a/sound/usb/usx2y/usx2yhwdeppcm.c
> > +++ b/sound/usb/usx2y/usx2yhwdeppcm.c
> > @@ -244,13 +244,8 @@ static void i_usX2Y_usbpcm_urb_complete(struct urb
> > *urb) usX2Y_error_urb_status(usX2Y, subs, urb);
> >  		return;
> >  	}
> > -	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame &
> > 0xFFFF))) -		subs->completed_urb = urb;
> > -	else {
> > -		usX2Y_error_sequence(usX2Y, subs, urb);
> > -		return;
> > -	}
> > 
> > +	subs->completed_urb = urb;
> >  	capsubs = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE];
> >  	capsubs2 = usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE + 2];
> >  	playbacksubs = usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Takashi Iwai]

At Sun, 06 Oct 2013 10:29:23 +0200,
Guido Aulisi wrote:
> 
> > Next time, when detecting such problems, please consider sending a
> patch
> > to alsa-devel, so fixes make it to the users eventually :)
> 
> You are right, but I thought it was a problem with a broken/weird usb
> controller, because it worked with my old laptop.
> When I saw another user with the same problem, I sent the patch to LAU
> (the only mailing list I've subscribed to).
> 
> Now I remember why I did that patch: if you see my dmesg output without
> that patch, you can see that the driver is comparing 1024 to 0. If you
> check the history of the driver, you can see that this check has already
> been narrowed with the bit AND operation; I tried to narrow the check a
> little more, so I chose 0x3ff because 1024 & 0x3ff = 0.
> But I forgot about my Tascam (because I bought a RME Raydat) and never
> tested that patch, but I was quite confident that it should work.
> 
> [  669.312765] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
> here with counts=42
> [  669.329821] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
> here with counts=42
> [  669.345854] ALSA sound/usb/usx2y/usbusx2yaudio.c:141 should not be
> here with counts=42

Maybe this is the actual reason?  This is a place returning -EPIPE,
thus these urbs won't be submitted.

Can anyone test whether the patch below works alone without the
previous patch?  (Meanwhile, the previous patch to remove the frame
check is fine, it's just too strict for controllers like ehci, so it's
still fine to apply.)


thanks,

Takashi

---
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 6234a51..1236ff3 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -137,10 +137,12 @@ static int usX2Y_urb_play_prepare(struct snd_usX2Y_substream *subs,
 		/* calculate the size of a packet */
 		counts = cap_urb->iso_frame_desc[pack].actual_length / usX2Y->stride;
 		count += counts;
+#if 0
 		if (counts < 43 || counts > 50) {
 			snd_printk(KERN_ERR "should not be here with counts=%i\n", counts);
 			return -EPIPE;
 		}
+#endif
 		/* set up descriptor */
 		urb->iso_frame_desc[pack].offset = pack ?
 			urb->iso_frame_desc[pack - 1].offset +

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Dr Nicholas J Bailey]

On Monday 07 October 2013 08:41:06 you wrote:
> Can anyone test whether the patch below works alone without the
> previous patch?  (Meanwhile, the previous patch to remove the frame
> check is fine, it's just too strict for controllers like ehci, so it's
> still fine to apply.)
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c
> b/sound/usb/usx2y/usbusx2yaudio.c index 6234a51..1236ff3 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -137,10 +137,12 @@ static int usX2Y_urb_play_prepare(struct
> snd_usX2Y_substream *subs, /* calculate the size of a packet */
>                 counts = cap_urb->iso_frame_desc[pack].actual_length /
> usX2Y->stride; count += counts;
> +#if 0
>                 if (counts < 43 || counts > 50) {
>                         snd_printk(KERN_ERR "should not be here with
> counts=%i\n", counts); return -EPIPE;
>                 }
> +#endif
>                 /* set up descriptor */
>                 urb->iso_frame_desc[pack].offset = pack ?
>                         urb->iso_frame_desc[pack - 1].offset +

For me, this patch alone fails in much the same way (possibly exactly the same 
way) as before the previous patch.

>From audacity (probably not much use unless you know the source code... I 
don't):

Expression 'PaAlsaStreamComponent_Initialize( &self->capture, alsaApi, 
inParams, StreamDirection_In, NULL != callback )' failed in 
'src/hostapi/alsa/pa_linux_alsa.c', line: 2092
Expression 'PaAlsaStream_Initialize( stream, alsaHostApi, inputParameters, 
outputParameters, sampleRate, framesPerBuffer, callback, streamFlags, userData 
)' failed in 'src/hostapi/alsa/pa_linux_alsa.c', line: 2764
Expression 'stream->playback.pcm' failed in 
'src/hostapi/alsa/pa_linux_alsa.c', line: 4541
Expression 'stream->playback.pcm' failed in 
'src/hostapi/alsa/pa_linux_alsa.c', line: 4541
^C


[I hit ^C because recording stalled]


nick@arial:/usr/src$ dmesg | tail
[  111.667015] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
SerialNumber=0
[  114.707156] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
[  114.707156] Most probably some urb of usb-frame 1024 is still missing.
[  114.707156] Cause could be too long delays in usb-hcd interrupt handling.
[  574.566748] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
[  574.566748] Most probably some urb of usb-frame 1024 is still missing.
[  574.566748] Cause could be too long delays in usb-hcd interrupt handling.
[  575.593918] Sequence Error!(hcd_frame=6 ep=8in;wait=1027,frame=3).
[  575.593918] Most probably some urb of usb-frame 1027 is still missing.
[  575.593918] Cause could be too long delays in usb-hcd interrupt handling.


Reverting to the previous patch, everything works fine:

[  170.252226] usb 2-1.6.4: USB disconnect, device number 5
[  173.012844] usb 2-1.6.4: new full-speed USB device number 7 using ehci-pci
[  173.105451] usb 2-1.6.4: New USB device found, idVendor=1604, 
idProduct=8006
[  173.105456] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
SerialNumber=0
[  175.372992] usb 2-1.6.4: USB disconnect, device number 7
[  177.109626] usb 2-1.6.4: new full-speed USB device number 8 using ehci-pci
[  177.202361] usb 2-1.6.4: New USB device found, idVendor=1604, 
idProduct=8007
[  177.202372] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
SerialNumber=0


[no further messages]

Best stick with the original patch then :)

Nick/.

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Daniel Mack]

On 07.10.2013 16:58, Dr Nicholas J Bailey wrote:
> For me, this patch alone fails in much the same way (possibly exactly the same 
> way) as before the previous patch.
> 
> From audacity (probably not much use unless you know the source code... I 
> don't):

[...]

> nick@arial:/usr/src$ dmesg | tail
> [  111.667015] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
> SerialNumber=0
> [  114.707156] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
> [  114.707156] Most probably some urb of usb-frame 1024 is still missing.

You certainly haven't booted a kernel which contains the patch we're
talking about here. The only occurance of the error message you quote
was removed by this patch, so a patched kernel can't possibly produce
what you see in your logs.

Please check your setup again and see what 'uname -a' says.


Daniel

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Daniel Mack]

On 07.10.2013 17:35, Takashi Iwai wrote:
> At Mon, 07 Oct 2013 17:11:44 +0200,
> Daniel Mack wrote:
>>
>> On 07.10.2013 16:58, Dr Nicholas J Bailey wrote:
>>> For me, this patch alone fails in much the same way (possibly exactly the same 
>>> way) as before the previous patch.
>>>
>>> From audacity (probably not much use unless you know the source code... I 
>>> don't):
>>
>> [...]
>>
>>> nick@arial:/usr/src$ dmesg | tail
>>> [  111.667015] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
>>> SerialNumber=0
>>> [  114.707156] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
>>> [  114.707156] Most probably some urb of usb-frame 1024 is still missing.
>>
>> You certainly haven't booted a kernel which contains the patch we're
>> talking about here. The only occurance of the error message you quote
>> was removed by this patch, so a patched kernel can't possibly produce
>> what you see in your logs.
> 
> Nicholas booted a kernel without your fix patch but only with my patch
> to disable the -EPIPE check, in order to see whether the latter alone
> suffices or not.  So he's testing with a right kernel :)

Oh, so *I* was referring to the wrong patch then.
Apologies for causing confusion :)


Daniel

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Takashi Iwai]

At Mon, 07 Oct 2013 17:11:44 +0200,
Daniel Mack wrote:
> 
> On 07.10.2013 16:58, Dr Nicholas J Bailey wrote:
> > For me, this patch alone fails in much the same way (possibly exactly the same 
> > way) as before the previous patch.
> > 
> > From audacity (probably not much use unless you know the source code... I 
> > don't):
> 
> [...]
> 
> > nick@arial:/usr/src$ dmesg | tail
> > [  111.667015] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0, 
> > SerialNumber=0
> > [  114.707156] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
> > [  114.707156] Most probably some urb of usb-frame 1024 is still missing.
> 
> You certainly haven't booted a kernel which contains the patch we're
> talking about here. The only occurance of the error message you quote
> was removed by this patch, so a patched kernel can't possibly produce
> what you see in your logs.

Nicholas booted a kernel without your fix patch but only with my patch
to disable the -EPIPE check, in order to see whether the latter alone
suffices or not.  So he's testing with a right kernel :)


Takashi

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Dr Nicholas J Bailey]

On Monday 07 October 2013 16:34:37 you wrote:
> On 07.10.2013 17:35, Takashi Iwai wrote:
> > At Mon, 07 Oct 2013 17:11:44 +0200,
> > 
> > Daniel Mack wrote:
> >> On 07.10.2013 16:58, Dr Nicholas J Bailey wrote:
> >>> For me, this patch alone fails in much the same way (possibly exactly
> >>> the same way) as before the previous patch.
> >>> 
> >>> From audacity (probably not much use unless you know the source code...
> >>> I
> >> 
> >>> don't):
> >> [...]
> >> 
> >>> nick@arial:/usr/src$ dmesg | tail
> >>> [  111.667015] usb 2-1.6.4: New USB device strings: Mfr=0, Product=0,
> >>> SerialNumber=0
> >>> [  114.707156] Sequence Error!(hcd_frame=3 ep=8in;wait=1024,frame=0).
> >>> [  114.707156] Most probably some urb of usb-frame 1024 is still
> >>> missing.
> >> 
> >> You certainly haven't booted a kernel which contains the patch we're
> >> talking about here. The only occurance of the error message you quote
> >> was removed by this patch, so a patched kernel can't possibly produce
> >> what you see in your logs.
> > 
> > Nicholas booted a kernel without your fix patch but only with my patch
> > to disable the -EPIPE check, in order to see whether the latter alone
> > suffices or not.  So he's testing with a right kernel :)
> 
> Oh, so *I* was referring to the wrong patch then.
> Apologies for causing confusion :)
> 
> 
> Daniel

That's cool, Daniel. Actually, I've not written anything for the kernel since 
1.x (it was an intravascular ultrasound board my PhD student built - it was a 
hardware project and the sorfware was never distributed). Things have changed 
a *lot* since then, and I never used make-dpkg before this, so please keep on 
checking I do the right thing!

The only thing I *might* have screwed up is that both the kernels are called 
3.10.11 as far as uname -a is concerned, but are in separate debs. So the 
patch which took out

	static void usX2Y_error_sequence(
		struct usX2Ydev *usX2Y,
		struct snd_usX2Y_substream *subs, struct urb *urb)

and the bit about

	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame & 
0xFFFF)))
		subs->completed_urb = urb;
	else {
		usX2Y_error_sequence(usX2Y, subs, urb);
		return;
	}

are in a deb called

	linux-image-3.10.11_3.10.11.TASCAM.2_i386.deb

and the one with Takashi's cpp directive /only/ based on a fresh source tree 
from the debian source package is in

	linux-image-3.10.11_3.10.11.TASCAM.3_i386.deb

I'm installing the package I want to play with, and there's a message to say I 
should reboot ASAP because of potential module conflicts, and that when I do 
the machine will recompute the mod deps for me, so I do, and that's that.

Probably a bit of a hack, but I don't think it's stupidly wrong (unless you 
know different).

Bottom line is:
	TASCAM.2 works with no complaints;
	TASCAM.3 (with just the #if 0...#endif and no other changes to 3.10.11) 
gives the stalls and dmesg messages.

The only other thing I changed was the processor. I selected core2 because I 
thought it would be nice to have a custom kernel for once :)

I hope I've not been wasting your time.

Nick/.

Re: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

[Dr Nicholas J Bailey]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Monday 07 October 2013 16:07:28 you wrote:
> OK, then the sequence error thingy is independent from the bogus
> overrun check.  But I guess you still get the errors about "should not
> be here with..." even with the sequence-check-removal patch?
> 
> 
> thanks,
> 
> Takashi

No, because I did the bigger of the two original patches (this is getting 
difficult!!) as per the other email I just sent, so the whole of the function 
usX2Y_error_sequence is gone. So no messages.

I did also build a package I called

	linux-image-3.10.11_3.10.11.TASCAM.1_i386.deb

which had just the change to the

	if (likely((urb->start_frame & 0xFFFF) == (usX2Y->wait_iso_frame & 
0xFFFF)))

(0xFFFF to 0X03FF or whatever). That worked too. I can't remember if there 
were still sequence errors. I've deleted now I'm afraid, having gone for the 
larger version.

The patch that works well for me modifies sound/usb/usx2y/usx2yhwdeppcm.c as 
well, replacing a whole similar if statement at line 244  with the line

	subs->completed_urb = urb;

I got it from the attached email from Daniel.

Nick/.

[-- Attachment #2: forwarded message --]
[-- Type: text/plain, Size: 4928 bytes --]

Content-Type: message/rfc822
Content-Disposition: inline; filename="forwarded message"
Content-Description: Daniel Mack <zonque@gmail.com>: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks

Received: from palin.cent.gla.ac.uk (130.209.34.34) by smtp.campus.gla.ac.uk (130.209.14.19) with Microsoft SMTP Server id 8.3.327.1; Wed, 2 Oct 2013 16:50:03 +0100
Received: from svenfoo.org ([82.94.215.22] helo=mail.zonque.de)	by palin.cent.gla.ac.uk with esmtp (Exim 4.72)	(envelope-from <zonque@gmail.com>)	id 1VROgc-00033f-QQ	for nicholas.bailey@glasgow.ac.uk; Wed, 02 Oct 2013 16:50:03 +0100
Received: from localhost (localhost [127.0.0.1])	by mail.zonque.de (Postfix) with ESMTP id 817F7C1744;	Wed,  2 Oct 2013 17:50:00 +0200 (CEST)
Received: from mail.zonque.de ([127.0.0.1])	by localhost (rambrand.bugwerft.de [127.0.0.1]) (amavisd-new, port 10024)	with ESMTP id Hk6AJ7ax5yMM; Wed,  2 Oct 2013 17:50:00 +0200 (CEST)
Received: from tamtam.taperay.com (unknown [46.115.92.231])	(using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits))	(No client certificate requested)	by mail.zonque.de (Postfix) with ESMTPSA id B37B4C1732;	Wed,  2 Oct 2013 17:49:58 +0200 (CEST)
From: Daniel Mack <zonque@gmail.com>
To: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Cc: "tiwai@suse.de" <tiwai@suse.de>, Nicholas Bailey <Nicholas.Bailey@glasgow.ac.uk>, "guido.aulisi@gmail.com" <guido.aulisi@gmail.com>, Daniel Mack <zonque@gmail.com>, "fzu@wemgehoertderstaat.de" <fzu@wemgehoertderstaat.de>
Date: Wed, 02 Oct 2013 16:49:50 +0100
Subject: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks
Thread-Topic: [PATCH v2] ALSA: snd-usb-usx2y: remove bogus frame checks
Thread-Index: Ac6/hw9nqvIRMxIZQoqDHPQXxXHLkQ==
Message-ID: <1380728990-8443-1-git-send-email-zonque@gmail.com>
Accept-Language: en-GB
Content-Language: en-GB
X-MS-Exchange-Organization-AuthAs: Anonymous
X-MS-Exchange-Organization-AuthSource: hub01.campus.gla.ac.uk
x-gla-spam-scan: R
x-gla-spam-report: _SUMMARY_
x-gla-spam-score: 0.0 (/)
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

The frame check in i_usX2Y_urb_complete() and
i_usX2Y_usbpcm_urb_complete() is bogus and produces false positives as
described in this LAU thread:

  http://linuxaudio.org/mailarchive/lau/2013/5/20/200177

This patch removes the check code entirely.

Cc: fzu@wemgehoertderstaat.de
Reported-by: Dr Nicholas J Bailey <nicholas.bailey@glasgow.ac.uk>
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 sound/usb/usx2y/usbusx2yaudio.c | 22 +++-------------------
 sound/usb/usx2y/usx2yhwdeppcm.c |  7 +------
 2 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudi=
o.c
index 63fb521..6234a51 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -299,19 +299,6 @@ static void usX2Y_error_urb_status(struct usX2Ydev *us=
X2Y,
 	usX2Y_clients_stop(usX2Y);
 }
=20
-static void usX2Y_error_sequence(struct usX2Ydev *usX2Y,
-				 struct snd_usX2Y_substream *subs, struct urb *urb)
-{
-	snd_printk(KERN_ERR
-"Sequence Error!(hcd_frame=3D%i ep=3D%i%s;wait=3D%i,frame=3D%i).\n"
-"Most probably some urb of usb-frame %i is still missing.\n"
-"Cause could be too long delays in usb-hcd interrupt handling.\n",
-		   usb_get_current_frame_number(usX2Y->dev),
-		   subs->endpoint, usb_pipein(urb->pipe) ? "in" : "out",
-		   usX2Y->wait_iso_frame, urb->start_frame, usX2Y->wait_iso_frame);
-	usX2Y_clients_stop(usX2Y);
-}
-
 static void i_usX2Y_urb_complete(struct urb *urb)
 {
 	struct snd_usX2Y_substream *subs =3D urb->context;
@@ -328,12 +315,9 @@ static void i_usX2Y_urb_complete(struct urb *urb)
 		usX2Y_error_urb_status(usX2Y, subs, urb);
 		return;
 	}
-	if (likely((urb->start_frame & 0xFFFF) =3D=3D (usX2Y->wait_iso_frame & 0x=
FFFF)))
-		subs->completed_urb =3D urb;
-	else {
-		usX2Y_error_sequence(usX2Y, subs, urb);
-		return;
-	}
+
+	subs->completed_urb =3D urb;
+
 	{
 		struct snd_usX2Y_substream *capsubs =3D usX2Y->subs[SNDRV_PCM_STREAM_CAP=
TURE],
 			*playbacksubs =3D usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppc=
m.c
index f2a1acd..814d0e8 100644
--- a/sound/usb/usx2y/usx2yhwdeppcm.c
+++ b/sound/usb/usx2y/usx2yhwdeppcm.c
@@ -244,13 +244,8 @@ static void i_usX2Y_usbpcm_urb_complete(struct urb *ur=
b)
 		usX2Y_error_urb_status(usX2Y, subs, urb);
 		return;
 	}
-	if (likely((urb->start_frame & 0xFFFF) =3D=3D (usX2Y->wait_iso_frame & 0x=
FFFF)))
-		subs->completed_urb =3D urb;
-	else {
-		usX2Y_error_sequence(usX2Y, subs, urb);
-		return;
-	}
=20
+	subs->completed_urb =3D urb;
 	capsubs =3D usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE];
 	capsubs2 =3D usX2Y->subs[SNDRV_PCM_STREAM_CAPTURE + 2];
 	playbacksubs =3D usX2Y->subs[SNDRV_PCM_STREAM_PLAYBACK];
--=20
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]