[1/2] ALSA: firewire-tascam: Fix infinite loop in snd_tscm_stream_get_rate()
diff mbox series

Message ID 1494836350-28790-1-git-send-email-geert@linux-m68k.org
State New, archived
Headers show
Series
  • [1/2] ALSA: firewire-tascam: Fix infinite loop in snd_tscm_stream_get_rate()
Related show

Commit Message

Geert Uytterhoeven May 15, 2017, 8:19 a.m. UTC
Obviously the intention was to put a limit on the maximum number of
operations.  However, for this to work, the check should be
"&& trials++ < 5", not "|| trials++ < 5".

Fixes: 35efa5c489de63a9 ("ALSA: firewire-tascam: add streaming functionality")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Compile-tested only.

Triggered by a false-positive warning from gcc-4.1.2:

    warning: ‘err’ may be used uninitialized in this function
---
 sound/firewire/tascam/tascam-stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Takashi Sakamoto May 15, 2017, 2:07 p.m. UTC | #1
Hi,

On May 15 2017 17:19, Geert Uytterhoeven wrote:
> Obviously the intention was to put a limit on the maximum number of
> operations.  However, for this to work, the check should be
> "&& trials++ < 5", not "|| trials++ < 5".
>
> Fixes: 35efa5c489de63a9 ("ALSA: firewire-tascam: add streaming functionality")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Compile-tested only.
>
> Triggered by a false-positive warning from gcc-4.1.2:
>
>     warning: ‘err’ may be used uninitialized in this function
> ---
>  sound/firewire/tascam/tascam-stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
> index f1657a4e0621ef49..e433b92ac6904db5 100644
> --- a/sound/firewire/tascam/tascam-stream.c
> +++ b/sound/firewire/tascam/tascam-stream.c
> @@ -84,7 +84,7 @@ int snd_tscm_stream_get_rate(struct snd_tscm *tscm, unsigned int *rate)
>  	unsigned int trials = 0;
>  	int err;
>
> -	while (data == 0x0 || trials++ < 5) {
> +	while (data == 0x0 && trials++ < 5) {
>  		err = get_clock(tscm, &data);
>  		if (err < 0)
>  			return err;

Yep. It looks a bug.

...However, removal of the bug causes issue that the driver fails to 
start a pair of capture/playback PCM substream when application requests 
them mostly the same time, like jackd process.

I think I did apply the bug as a makeshift workaround, then forgot 
itself when developing the driver... I'd like to have a bit time for 
further investigation, then post my fix in this development period.

Please let me keep this patch pending.


Thanks

Takashi Sakamoto
Geert Uytterhoeven May 15, 2017, 2:22 p.m. UTC | #2
Hi Sakamoto-san,

On Mon, May 15, 2017 at 4:07 PM, Takashi Sakamoto
<o-takashi@sakamocchi.jp> wrote:
> On May 15 2017 17:19, Geert Uytterhoeven wrote:
>> Obviously the intention was to put a limit on the maximum number of
>> operations.  However, for this to work, the check should be
>> "&& trials++ < 5", not "|| trials++ < 5".
>>
>> Fixes: 35efa5c489de63a9 ("ALSA: firewire-tascam: add streaming
>> functionality")
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>> Compile-tested only.
>>
>> Triggered by a false-positive warning from gcc-4.1.2:
>>
>>     warning: ‘err’ may be used uninitialized in this function
>> ---
>>  sound/firewire/tascam/tascam-stream.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/firewire/tascam/tascam-stream.c
>> b/sound/firewire/tascam/tascam-stream.c
>> index f1657a4e0621ef49..e433b92ac6904db5 100644
>> --- a/sound/firewire/tascam/tascam-stream.c
>> +++ b/sound/firewire/tascam/tascam-stream.c
>> @@ -84,7 +84,7 @@ int snd_tscm_stream_get_rate(struct snd_tscm *tscm,
>> unsigned int *rate)
>>         unsigned int trials = 0;
>>         int err;
>>
>> -       while (data == 0x0 || trials++ < 5) {
>> +       while (data == 0x0 && trials++ < 5) {
>>                 err = get_clock(tscm, &data);
>>                 if (err < 0)
>>                         return err;
>
> Yep. It looks a bug.
>
> ...However, removal of the bug causes issue that the driver fails to start a
> pair of capture/playback PCM substream when application requests them mostly
> the same time, like jackd process.
>
> I think I did apply the bug as a makeshift workaround, then forgot itself
> when developing the driver... I'd like to have a bit time for further
> investigation, then post my fix in this development period.

Probably you need a small delay in the loop?
Why else do you #include <linux/delay.h>? ;-)

> Please let me keep this patch pending.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Takashi Sakamoto May 15, 2017, 2:40 p.m. UTC | #3
On May 15 2017 23:22, Geert Uytterhoeven wrote:
>>> diff --git a/sound/firewire/tascam/tascam-stream.c
>>> b/sound/firewire/tascam/tascam-stream.c
>>> index f1657a4e0621ef49..e433b92ac6904db5 100644
>>> --- a/sound/firewire/tascam/tascam-stream.c
>>> +++ b/sound/firewire/tascam/tascam-stream.c
>>> @@ -84,7 +84,7 @@ int snd_tscm_stream_get_rate(struct snd_tscm *tscm,
>>> unsigned int *rate)
>>>         unsigned int trials = 0;
>>>         int err;
>>>
>>> -       while (data == 0x0 || trials++ < 5) {
>>> +       while (data == 0x0 && trials++ < 5) {
>>>                 err = get_clock(tscm, &data);
>>>                 if (err < 0)
>>>                         return err;
>>
>> Yep. It looks a bug.
>>
>> ...However, removal of the bug causes issue that the driver fails to start a
>> pair of capture/playback PCM substream when application requests them mostly
>> the same time, like jackd process.
>>
>> I think I did apply the bug as a makeshift workaround, then forgot itself
>> when developing the driver... I'd like to have a bit time for further
>> investigation, then post my fix in this development period.
>
> Probably you need a small delay in the loop?
> Why else do you #include <linux/delay.h>? ;-)

I know PDI1394L40 (link layer) has quirks inconvenient to us. 
Furthermore, the behaviour of on-board FPGAs with unique firmwares are 
in black boxes to me.

I prefer deliberate approach for this kind of work, reverse engineering.


Regards

Takashi Sakamoto

Patch
diff mbox series

diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
index f1657a4e0621ef49..e433b92ac6904db5 100644
--- a/sound/firewire/tascam/tascam-stream.c
+++ b/sound/firewire/tascam/tascam-stream.c
@@ -84,7 +84,7 @@  int snd_tscm_stream_get_rate(struct snd_tscm *tscm, unsigned int *rate)
 	unsigned int trials = 0;
 	int err;
 
-	while (data == 0x0 || trials++ < 5) {
+	while (data == 0x0 && trials++ < 5) {
 		err = get_clock(tscm, &data);
 		if (err < 0)
 			return err;