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

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

[Geert Uytterhoeven]

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

[PATCH 2/2] ALSA: firewire-tascam: Use do { } while for loops executed at least once

[Geert Uytterhoeven]

If a loop is intended to be executed at least once, it is better to use
"do { ... } while (...)" instead of "while (...) { ... }".
The former is easier to understand for the casual reviewer, and doesn't
require preinitializing the canary variable.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Compile-tested only.
---
 sound/firewire/tascam/tascam-stream.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
index e433b92ac6904db5..4ea6845aae87fdef 100644
--- a/sound/firewire/tascam/tascam-stream.c
+++ b/sound/firewire/tascam/tascam-stream.c
@@ -80,17 +80,17 @@ static int set_clock(struct snd_tscm *tscm, unsigned int rate,
 
 int snd_tscm_stream_get_rate(struct snd_tscm *tscm, unsigned int *rate)
 {
-	u32 data = 0x0;
 	unsigned int trials = 0;
+	u32 data;
 	int err;
 
-	while (data == 0x0 && trials++ < 5) {
+	do {
 		err = get_clock(tscm, &data);
 		if (err < 0)
 			return err;
 
 		data = (data & 0xff000000) >> 24;
-	}
+	} while (data == 0x0 && ++trials < 5);
 
 	/* Check base rate. */
 	if ((data & 0x0f) == 0x01)
-- 
2.7.4

Re: [PATCH 1/2] ALSA: firewire-tascam: Fix infinite loop in snd_tscm_stream_get_rate()

[Takashi Sakamoto]

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

Re: [PATCH 1/2] ALSA: firewire-tascam: Fix infinite loop in snd_tscm_stream_get_rate()

[Geert Uytterhoeven]

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

Re: [PATCH 1/2] ALSA: firewire-tascam: Fix infinite loop in snd_tscm_stream_get_rate()

[Takashi Sakamoto]

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