| Message ID | 1494836350-28790-1-git-send-email-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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;
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(-)