[PATCH 0/2] USB audio fixes

[PATCH 0/2] USB audio fixes

[Pierre-Louis Bossart]

While trying to understand how asynchronous endpoints are supported, I
identified two major issues. On my M-Audio transit, the following
script tries to play using the 3 different endpoints supported and
fails (error and no audio) after the first playback without the
patches. With the patches on top of 4.1.5 or Takashi's for-next
(4.2.0-rc5), I stopped the audio after an hour.

while true
do
    aplay -d5 -Dhw:2,0 ~/16_48.wav # play 48kHz, 16 bit on async ep
    aplay -d5 -Dhw:2,0 ~/24_48.wav # play 48kHz, 24 bit on 'none'/async ep
    aplay -d5 -Dhw:2,0 ~/24_96.wav # play 96 kHz, 24 bit on adaptive ep
    aplay -d5 -Dhw:2,0 ~/24_48.wav 
    aplay -d5 -Dhw:2,0 ~/16_48.wav
    aplay -d5 -Dhw:2,0 ~/24_96.wav
done

The first issue is pretty bad with corrupted pointers. When
transitioning from a alternate setting with a sync_endpoint to another
which doesn't have one by construction (synchronous, adaptive, async
on capture), the pointers are not reset and the code tries to set the
parameters of a non-existent endpoint.

The second issue is that the M-audio Transit descriptors are illegal
but the logic can be changed without harm.

I sometimes hear noise on playback start, probably the code needs to
be changed to add enough silent samples to let the PLL lock. I haven't
had time to work on this and this is more of an enhancement.

Comments welcome.

Pierre-Louis Bossart (2):
  ALSA: usb: fix corrupted pointers due to interface setting change
  ALSA: usb: handle descriptor with SYNC_NONE illegal value

 sound/usb/pcm.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Pierre-Louis Bossart]

When a transition occurs between alternate settings that do not use the
same synchronization method, the substream pointers were not reset.
This prevents audio from being played during the second transition.

Identified and tested with M-Audio Transit device
(0763:2006 Midiman M-Audio Transit)

Details of the issue:

First playback to adaptive endpoint:
$ aplay -Dhw:1,0 ~/24_96.wav
Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
Rate 96000 Hz, Stereo

[ 3169.297556] usb 1-2: setting usb interface 1:1
[ 3169.297568] usb 1-2: Creating new playback data endpoint #3
[ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0
[ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000

first playback to asynchronous endpoint:
$ aplay -Dhw:1,0 ~/16_48.wav
Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian,
Rate 48000 Hz, Stereo

[ 3204.520251] usb 1-2: setting usb interface 1:3
[ 3204.520264] usb 1-2: Creating new playback data endpoint #3
[ 3204.520272] usb 1-2: Creating new capture sync endpoint #83
[ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
[ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
[ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000
[ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000

second playback to adaptive endpoint: no audio and error on terminal:
$ aplay -Dhw:1,0 ~/24_96.wav
Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
Rate 96000 Hz, Stereo
aplay: pcm_write:1939: write error: Input/output error

[ 3239.483589] usb 1-2: setting usb interface 1:1
[ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000
[ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
[ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0

This last line shows that a sync endpoint is used when it shouldn't.
The sync endpoint is no longer valid and the pointers are corrupted

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/usb/pcm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b4ef410..7cd7c03 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	if (err < 0)
 		return err;
 
+	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
+		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
+
+		/*
+		 * Clean-up subs pointers to make sure sync_endpoint is never
+		 * configured. This is needed in case of a transition between
+		 * alternate settings using different synchronization modes
+		 * where the previous sync_endpoint may no longer be valid.
+		 */
+		subs->sync_endpoint = NULL;
+		subs->data_endpoint->sync_master = NULL;
+	}
+
 	if (altsd->bNumEndpoints < 2)
 		return 0;
 
-- 
1.9.1

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Takashi Iwai]

On Fri, 14 Aug 2015 00:42:32 +0200,
Pierre-Louis Bossart wrote:
> 
> When a transition occurs between alternate settings that do not use the
> same synchronization method, the substream pointers were not reset.
> This prevents audio from being played during the second transition.
> 
> Identified and tested with M-Audio Transit device
> (0763:2006 Midiman M-Audio Transit)

Hmm, I have this old device, too, but couldn't reproduce the problem.
Is there any special setup with it?


Takashi

> 
> Details of the issue:
> 
> First playback to adaptive endpoint:
> $ aplay -Dhw:1,0 ~/24_96.wav
> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> Rate 96000 Hz, Stereo
> 
> [ 3169.297556] usb 1-2: setting usb interface 1:1
> [ 3169.297568] usb 1-2: Creating new playback data endpoint #3
> [ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0
> [ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000
> 
> first playback to asynchronous endpoint:
> $ aplay -Dhw:1,0 ~/16_48.wav
> Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian,
> Rate 48000 Hz, Stereo
> 
> [ 3204.520251] usb 1-2: setting usb interface 1:3
> [ 3204.520264] usb 1-2: Creating new playback data endpoint #3
> [ 3204.520272] usb 1-2: Creating new capture sync endpoint #83
> [ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> [ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> [ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000
> [ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000
> 
> second playback to adaptive endpoint: no audio and error on terminal:
> $ aplay -Dhw:1,0 ~/24_96.wav
> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> Rate 96000 Hz, Stereo
> aplay: pcm_write:1939: write error: Input/output error
> 
> [ 3239.483589] usb 1-2: setting usb interface 1:1
> [ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000
> [ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> [ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> 
> This last line shows that a sync endpoint is used when it shouldn't.
> The sync endpoint is no longer valid and the pointers are corrupted
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/usb/pcm.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index b4ef410..7cd7c03 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  	if (err < 0)
>  		return err;
>  
> +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
> +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
> +
> +		/*
> +		 * Clean-up subs pointers to make sure sync_endpoint is never
> +		 * configured. This is needed in case of a transition between
> +		 * alternate settings using different synchronization modes
> +		 * where the previous sync_endpoint may no longer be valid.
> +		 */
> +		subs->sync_endpoint = NULL;
> +		subs->data_endpoint->sync_master = NULL;
> +	}
> +
>  	if (altsd->bNumEndpoints < 2)
>  		return 0;
>  
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Takashi Iwai]

On Fri, 14 Aug 2015 17:03:10 +0200,
Takashi Iwai wrote:
> 
> On Fri, 14 Aug 2015 00:42:32 +0200,
> Pierre-Louis Bossart wrote:
> > 
> > When a transition occurs between alternate settings that do not use the
> > same synchronization method, the substream pointers were not reset.
> > This prevents audio from being played during the second transition.
> > 
> > Identified and tested with M-Audio Transit device
> > (0763:2006 Midiman M-Audio Transit)
> 
> Hmm, I have this old device, too, but couldn't reproduce the problem.
> Is there any special setup with it?

OK, I could see it after switching a few times.  But, also your patch
didn't cure, either...


Takashi

> 
> 
> Takashi
> 
> > 
> > Details of the issue:
> > 
> > First playback to adaptive endpoint:
> > $ aplay -Dhw:1,0 ~/24_96.wav
> > Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> > Rate 96000 Hz, Stereo
> > 
> > [ 3169.297556] usb 1-2: setting usb interface 1:1
> > [ 3169.297568] usb 1-2: Creating new playback data endpoint #3
> > [ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0
> > [ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000
> > 
> > first playback to asynchronous endpoint:
> > $ aplay -Dhw:1,0 ~/16_48.wav
> > Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian,
> > Rate 48000 Hz, Stereo
> > 
> > [ 3204.520251] usb 1-2: setting usb interface 1:3
> > [ 3204.520264] usb 1-2: Creating new playback data endpoint #3
> > [ 3204.520272] usb 1-2: Creating new capture sync endpoint #83
> > [ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> > [ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> > [ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000
> > [ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000
> > 
> > second playback to adaptive endpoint: no audio and error on terminal:
> > $ aplay -Dhw:1,0 ~/24_96.wav
> > Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> > Rate 96000 Hz, Stereo
> > aplay: pcm_write:1939: write error: Input/output error
> > 
> > [ 3239.483589] usb 1-2: setting usb interface 1:1
> > [ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000
> > [ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> > [ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> > 
> > This last line shows that a sync endpoint is used when it shouldn't.
> > The sync endpoint is no longer valid and the pointers are corrupted
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >  sound/usb/pcm.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> > index b4ef410..7cd7c03 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
> > +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
> > +
> > +		/*
> > +		 * Clean-up subs pointers to make sure sync_endpoint is never
> > +		 * configured. This is needed in case of a transition between
> > +		 * alternate settings using different synchronization modes
> > +		 * where the previous sync_endpoint may no longer be valid.
> > +		 */
> > +		subs->sync_endpoint = NULL;
> > +		subs->data_endpoint->sync_master = NULL;
> > +	}
> > +
> >  	if (altsd->bNumEndpoints < 2)
> >  		return 0;
> >  
> > -- 
> > 1.9.1
> > 

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Pierre-Louis Bossart]

On 8/14/15 10:28 AM, Takashi Iwai wrote:
> On Fri, 14 Aug 2015 17:03:10 +0200,
> Takashi Iwai wrote:
>>
>> On Fri, 14 Aug 2015 00:42:32 +0200,
>> Pierre-Louis Bossart wrote:
>>>
>>> When a transition occurs between alternate settings that do not use the
>>> same synchronization method, the substream pointers were not reset.
>>> This prevents audio from being played during the second transition.
>>>
>>> Identified and tested with M-Audio Transit device
>>> (0763:2006 Midiman M-Audio Transit)
>>
>> Hmm, I have this old device, too, but couldn't reproduce the problem.
>> Is there any special setup with it?
>
> OK, I could see it after switching a few times.  But, also your patch
> didn't cure, either...

I tested this with Ubuntu 14.04 and the default madfuload package that 
comes with it for the firmware.

I think you need both patches to get the device to work. I split the 
patches in two since I think the second problem is device specific while 
the first one is a generic one that can happen on other devices.

I generated three chirps test files with audacity, one stereo 96kHz 
24bits (3 bytes), one stereo 48kHz 24 bits (3 bytes) and then stereo 
48kHz 16 bits. This matches the capabilities of the three endpoints.
If you try first with the 48kHz 16bits one then typically the others 
won't work.

>
>
> Takashi
>
>>
>>
>> Takashi
>>
>>>
>>> Details of the issue:
>>>
>>> First playback to adaptive endpoint:
>>> $ aplay -Dhw:1,0 ~/24_96.wav
>>> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
>>> Rate 96000 Hz, Stereo
>>>
>>> [ 3169.297556] usb 1-2: setting usb interface 1:1
>>> [ 3169.297568] usb 1-2: Creating new playback data endpoint #3
>>> [ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0
>>> [ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000
>>>
>>> first playback to asynchronous endpoint:
>>> $ aplay -Dhw:1,0 ~/16_48.wav
>>> Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian,
>>> Rate 48000 Hz, Stereo
>>>
>>> [ 3204.520251] usb 1-2: setting usb interface 1:3
>>> [ 3204.520264] usb 1-2: Creating new playback data endpoint #3
>>> [ 3204.520272] usb 1-2: Creating new capture sync endpoint #83
>>> [ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
>>> [ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
>>> [ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000
>>> [ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000
>>>
>>> second playback to adaptive endpoint: no audio and error on terminal:
>>> $ aplay -Dhw:1,0 ~/24_96.wav
>>> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
>>> Rate 96000 Hz, Stereo
>>> aplay: pcm_write:1939: write error: Input/output error
>>>
>>> [ 3239.483589] usb 1-2: setting usb interface 1:1
>>> [ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000
>>> [ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
>>> [ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
>>>
>>> This last line shows that a sync endpoint is used when it shouldn't.
>>> The sync endpoint is no longer valid and the pointers are corrupted
>>>
>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>> ---
>>>   sound/usb/pcm.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>>> index b4ef410..7cd7c03 100644
>>> --- a/sound/usb/pcm.c
>>> +++ b/sound/usb/pcm.c
>>> @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>>>   	if (err < 0)
>>>   		return err;
>>>
>>> +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
>>> +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
>>> +
>>> +		/*
>>> +		 * Clean-up subs pointers to make sure sync_endpoint is never
>>> +		 * configured. This is needed in case of a transition between
>>> +		 * alternate settings using different synchronization modes
>>> +		 * where the previous sync_endpoint may no longer be valid.
>>> +		 */
>>> +		subs->sync_endpoint = NULL;
>>> +		subs->data_endpoint->sync_master = NULL;
>>> +	}
>>> +
>>>   	if (altsd->bNumEndpoints < 2)
>>>   		return 0;
>>>
>>> --
>>> 1.9.1
>>>

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Takashi Iwai]

On Fri, 14 Aug 2015 17:39:32 +0200,
Pierre-Louis Bossart wrote:
> 
> On 8/14/15 10:28 AM, Takashi Iwai wrote:
> > On Fri, 14 Aug 2015 17:03:10 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Fri, 14 Aug 2015 00:42:32 +0200,
> >> Pierre-Louis Bossart wrote:
> >>>
> >>> When a transition occurs between alternate settings that do not use the
> >>> same synchronization method, the substream pointers were not reset.
> >>> This prevents audio from being played during the second transition.
> >>>
> >>> Identified and tested with M-Audio Transit device
> >>> (0763:2006 Midiman M-Audio Transit)
> >>
> >> Hmm, I have this old device, too, but couldn't reproduce the problem.
> >> Is there any special setup with it?
> >
> > OK, I could see it after switching a few times.  But, also your patch
> > didn't cure, either...
> 
> I tested this with Ubuntu 14.04 and the default madfuload package that 
> comes with it for the firmware.
> 
> I think you need both patches to get the device to work. I split the 
> patches in two since I think the second problem is device specific while 
> the first one is a generic one that can happen on other devices.

I tested both at once, of course :)
With my device, 24/96 -> 16/48 works, then again switching to 24/96
stalls.  After that, it doesn't work no matter which mode is until I
replug the device.


Takashi

> I generated three chirps test files with audacity, one stereo 96kHz 
> 24bits (3 bytes), one stereo 48kHz 24 bits (3 bytes) and then stereo 
> 48kHz 16 bits. This matches the capabilities of the three endpoints.
> If you try first with the 48kHz 16bits one then typically the others 
> won't work.
> 
> >
> >
> > Takashi
> >
> >>
> >>
> >> Takashi
> >>
> >>>
> >>> Details of the issue:
> >>>
> >>> First playback to adaptive endpoint:
> >>> $ aplay -Dhw:1,0 ~/24_96.wav
> >>> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> >>> Rate 96000 Hz, Stereo
> >>>
> >>> [ 3169.297556] usb 1-2: setting usb interface 1:1
> >>> [ 3169.297568] usb 1-2: Creating new playback data endpoint #3
> >>> [ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0
> >>> [ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000
> >>>
> >>> first playback to asynchronous endpoint:
> >>> $ aplay -Dhw:1,0 ~/16_48.wav
> >>> Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian,
> >>> Rate 48000 Hz, Stereo
> >>>
> >>> [ 3204.520251] usb 1-2: setting usb interface 1:3
> >>> [ 3204.520264] usb 1-2: Creating new playback data endpoint #3
> >>> [ 3204.520272] usb 1-2: Creating new capture sync endpoint #83
> >>> [ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> >>> [ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> >>> [ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000
> >>> [ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000
> >>>
> >>> second playback to adaptive endpoint: no audio and error on terminal:
> >>> $ aplay -Dhw:1,0 ~/24_96.wav
> >>> Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes,
> >>> Rate 96000 Hz, Stereo
> >>> aplay: pcm_write:1939: write error: Input/output error
> >>>
> >>> [ 3239.483589] usb 1-2: setting usb interface 1:1
> >>> [ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000
> >>> [ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0
> >>> [ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0
> >>>
> >>> This last line shows that a sync endpoint is used when it shouldn't.
> >>> The sync endpoint is no longer valid and the pointers are corrupted
> >>>
> >>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>> ---
> >>>   sound/usb/pcm.c | 13 +++++++++++++
> >>>   1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> >>> index b4ef410..7cd7c03 100644
> >>> --- a/sound/usb/pcm.c
> >>> +++ b/sound/usb/pcm.c
> >>> @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
> >>>   	if (err < 0)
> >>>   		return err;
> >>>
> >>> +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
> >>> +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
> >>> +
> >>> +		/*
> >>> +		 * Clean-up subs pointers to make sure sync_endpoint is never
> >>> +		 * configured. This is needed in case of a transition between
> >>> +		 * alternate settings using different synchronization modes
> >>> +		 * where the previous sync_endpoint may no longer be valid.
> >>> +		 */
> >>> +		subs->sync_endpoint = NULL;
> >>> +		subs->data_endpoint->sync_master = NULL;
> >>> +	}
> >>> +
> >>>   	if (altsd->bNumEndpoints < 2)
> >>>   		return 0;
> >>>
> >>> --
> >>> 1.9.1
> >>>
> 

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Takashi Iwai]

On Fri, 14 Aug 2015 17:47:59 +0200,
Takashi Iwai wrote:
> 
> On Fri, 14 Aug 2015 17:39:32 +0200,
> Pierre-Louis Bossart wrote:
> > 
> > On 8/14/15 10:28 AM, Takashi Iwai wrote:
> > > On Fri, 14 Aug 2015 17:03:10 +0200,
> > > Takashi Iwai wrote:
> > >>
> > >> On Fri, 14 Aug 2015 00:42:32 +0200,
> > >> Pierre-Louis Bossart wrote:
> > >>>
> > >>> When a transition occurs between alternate settings that do not use the
> > >>> same synchronization method, the substream pointers were not reset.
> > >>> This prevents audio from being played during the second transition.
> > >>>
> > >>> Identified and tested with M-Audio Transit device
> > >>> (0763:2006 Midiman M-Audio Transit)
> > >>
> > >> Hmm, I have this old device, too, but couldn't reproduce the problem.
> > >> Is there any special setup with it?
> > >
> > > OK, I could see it after switching a few times.  But, also your patch
> > > didn't cure, either...
> > 
> > I tested this with Ubuntu 14.04 and the default madfuload package that 
> > comes with it for the firmware.
> > 
> > I think you need both patches to get the device to work. I split the 
> > patches in two since I think the second problem is device specific while 
> > the first one is a generic one that can happen on other devices.
> 
> I tested both at once, of course :)
> With my device, 24/96 -> 16/48 works, then again switching to 24/96
> stalls.  After that, it doesn't work no matter which mode is until I
> replug the device.

Hmm, now after a few retrials, it starts working.
Will review both patches now.


Takashi

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Takashi Iwai]

On Fri, 14 Aug 2015 00:42:32 +0200,
Pierre-Louis Bossart wrote:
> 
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  	if (err < 0)
>  		return err;
>  
> +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
> +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
> +
> +		/*
> +		 * Clean-up subs pointers to make sure sync_endpoint is never
> +		 * configured. This is needed in case of a transition between
> +		 * alternate settings using different synchronization modes
> +		 * where the previous sync_endpoint may no longer be valid.
> +		 */
> +		subs->sync_endpoint = NULL;
> +		subs->data_endpoint->sync_master = NULL;
> +	}

I think this initialization can be put unconditionally on top, not in
a separate like below, as this is just overlooked leaks.
The comment can be better in more details, of course.


Takashi

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b4ef410e5a98..0d935369d641 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -391,6 +391,10 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	 */
 	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
 
+	/* Clean-up subs sync and master pointers at first */
+	subs->sync_endpoint = NULL;
+	subs->data_endpoint->sync_master = NULL;
+
 	err = set_sync_ep_implicit_fb_quirk(subs, dev, altsd, attr);
 	if (err < 0)
 		return err;

Re: [PATCH 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

[Pierre-Louis Bossart]

On 8/14/15 10:57 AM, Takashi Iwai wrote:
> On Fri, 14 Aug 2015 00:42:32 +0200,
> Pierre-Louis Bossart wrote:
>>
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
>> @@ -395,6 +395,19 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>>   	if (err < 0)
>>   		return err;
>>
>> +	if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) ||
>> +		(!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) {
>> +
>> +		/*
>> +		 * Clean-up subs pointers to make sure sync_endpoint is never
>> +		 * configured. This is needed in case of a transition between
>> +		 * alternate settings using different synchronization modes
>> +		 * where the previous sync_endpoint may no longer be valid.
>> +		 */
>> +		subs->sync_endpoint = NULL;
>> +		subs->data_endpoint->sync_master = NULL;
>> +	}
>
> I think this initialization can be put unconditionally on top, not in
> a separate like below, as this is just overlooked leaks.
> The comment can be better in more details, of course.

I wasn't sure about side effects. I don't know what exactly the 
set_sync_ep_implicit_fb_quick() does and I was worried about changing 
the behavior on devices I didn't test. But if this is fine then I can 
change it, no issue.
Yes the comment isn't clear. I should be something like
"in these modes there is no sync_endpoint and the pointers need to be 
reset to avoid using stale information from previous settings"

Agree on the other changes, will provide an update. Thanks for the quick 
review!

>
>
> Takashi
>
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index b4ef410e5a98..0d935369d641 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -391,6 +391,10 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>   	 */
>   	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
>
> +	/* Clean-up subs sync and master pointers at first */
> +	subs->sync_endpoint = NULL;
> +	subs->data_endpoint->sync_master = NULL;
> +
>   	err = set_sync_ep_implicit_fb_quirk(subs, dev, altsd, attr);
>   	if (err < 0)
>   		return err;
>

[PATCH 2/2] ALSA: usb: handle descriptor with SYNC_NONE illegal value

[Pierre-Louis Bossart]

The M-Audio Transit exposes an interface with a SYNC_NONE attribute.
This is not a valid value according to the USB audio classspec. However
there is a sync endpoint associated to this record. Changing the logic to
try to use this sync endpoint allows for seamless transitions between
altset 2 and altset 3. If any errors happen, the behavior remains the same.

$ more /proc/asound/card1/stream0
M-Audio Transit USB at usb-0000:00:14.0-2, full speed : USB Audio

Playback:
  Status: Stop
  Interface 1
    Altset 1
    Format: S24_3LE
    Channels: 2
    Endpoint: 3 OUT (ADAPTIVE)
    Rates: 48001 - 96000 (continuous)
  Interface 1
    Altset 2
    Format: S24_3LE
    Channels: 2
    Endpoint: 3 OUT (NONE)
    Rates: 8000 - 48000 (continuous)
  Interface 1
    Altset 3
    Format: S16_LE
    Channels: 2
    Endpoint: 3 OUT (ASYNC)
    Rates: 8000 - 48000 (continuous)

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/usb/pcm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 7cd7c03..9f29017 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -411,10 +411,17 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	if (altsd->bNumEndpoints < 2)
 		return 0;
 
-	if ((is_playback && attr != USB_ENDPOINT_SYNC_ASYNC) ||
+	if ((is_playback && attr == USB_ENDPOINT_SYNC_SYNC) ||
+	    (is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE) ||
 	    (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE))
 		return 0;
 
+	/*
+	 * In case of illegal SYNC_NONE for OUT endpoint, we keep going to see
+	 * if we don't find a sync endpoint, as on M-Audio Transit. In case of
+	 * error fall back to SYNC mode and don't create sync endpoint
+	 */
+
 	/* check sync-pipe endpoint */
 	/* ... and check descriptor size before accessing bSynchAddress
 	   because there is a version of the SB Audigy 2 NX firmware lacking
@@ -428,6 +435,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 			   get_endpoint(alts, 1)->bmAttributes,
 			   get_endpoint(alts, 1)->bLength,
 			   get_endpoint(alts, 1)->bSynchAddress);
+		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
+			goto no_valid_sync_ep_found;
 		return -EINVAL;
 	}
 	ep = get_endpoint(alts, 1)->bEndpointAddress;
@@ -438,6 +447,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 			"%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
 			   fmt->iface, fmt->altsetting,
 			   is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
+		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
+			goto no_valid_sync_ep_found;
 		return -EINVAL;
 	}
 
@@ -449,11 +460,15 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 						   implicit_fb ?
 							SND_USB_ENDPOINT_TYPE_DATA :
 							SND_USB_ENDPOINT_TYPE_SYNC);
-	if (!subs->sync_endpoint)
+	if (!subs->sync_endpoint) {
+		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
+			goto no_valid_sync_ep_found;
 		return -EINVAL;
+	}
 
 	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
+no_valid_sync_ep_found: /* fall back to synchronous mode */
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 2/2] ALSA: usb: handle descriptor with SYNC_NONE illegal value

[Takashi Iwai]

On Fri, 14 Aug 2015 00:42:33 +0200,
Pierre-Louis Bossart wrote:
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -411,10 +411,17 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  	if (altsd->bNumEndpoints < 2)
>  		return 0;
>  
> -	if ((is_playback && attr != USB_ENDPOINT_SYNC_ASYNC) ||
> +	if ((is_playback && attr == USB_ENDPOINT_SYNC_SYNC) ||
> +	    (is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE) ||

Better to be:
	    (is_playback && (attr == USB_ENDPOINT_SYNC_SYNC ||
			     attr == USB_ENDPOINT_SYNC_ADAPTIVE)) ||


>  	    (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE))
>  		return 0;
>  
> +	/*
> +	 * In case of illegal SYNC_NONE for OUT endpoint, we keep going to see
> +	 * if we don't find a sync endpoint, as on M-Audio Transit. In case of
> +	 * error fall back to SYNC mode and don't create sync endpoint
> +	 */
> +
>  	/* check sync-pipe endpoint */
>  	/* ... and check descriptor size before accessing bSynchAddress
>  	   because there is a version of the SB Audigy 2 NX firmware lacking
> @@ -428,6 +435,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  			   get_endpoint(alts, 1)->bmAttributes,
>  			   get_endpoint(alts, 1)->bLength,
>  			   get_endpoint(alts, 1)->bSynchAddress);
> +		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
> +			goto no_valid_sync_ep_found;

We handle this as a success case, so the error message should be
avoided for SYNC_NONE.  Also, it's fine just returning 0 here.

>  		return -EINVAL;
>  	}
>  	ep = get_endpoint(alts, 1)->bEndpointAddress;
> @@ -438,6 +447,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  			"%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
>  			   fmt->iface, fmt->altsetting,
>  			   is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
> +		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
> +			goto no_valid_sync_ep_found;

Ditto.


thanks,

Takashi