| Message ID | 20180208105754.19924-1-danielhb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: > This patch adds a break in the switch() statement of complete(), > value 0x42: > > case 0x42: /* FT2 sets output freq with this, go figure */ > qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" > " should\n"); > break; <------- > case 0x41: It seems this is an intentional fallthrough, I understand cmd 0x42 is expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). > > The issue was found by Coverity (#1385841): > > CID 1385841: Control flow issues (MISSING_BREAK) > The case for value "66" is not terminated by a 'break' statement. > > Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()") > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > CC: John Arbuckle <programmingkidx@gmail.com> > CC: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/audio/sb16.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c > index 31de264ab7..b2fdcd8437 100644 > --- a/hw/audio/sb16.c > +++ b/hw/audio/sb16.c > @@ -744,6 +744,7 @@ static void complete (SB16State *s) > case 0x42: /* FT2 sets output freq with this, go figure */ > qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" > " should\n"); > + break; > case 0x41: > s->freq = dsp_get_hilo (s); > ldebug ("set freq %d\n", s->freq); >
On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Daniel, > > On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: >> This patch adds a break in the switch() statement of complete(), >> value 0x42: >> >> case 0x42: /* FT2 sets output freq with this, go figure */ >> qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" >> " should\n"); >> break; <------- >> case 0x41: > > It seems this is an intentional fallthrough, I understand cmd 0x42 is > expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). Yes, I agree; I wrote a bit about this in this thread: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html (though my guess is that actually 0x42 is supposed to do exactly what 0x41 does, and that the LOG_UNIMP should maybe just be removed). thanks -- PMM
On Thu, Feb 08, 2018 at 09:15:10AM -0300, Philippe Mathieu-Daudé wrote: > Hi Daniel, > > On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: > > This patch adds a break in the switch() statement of complete(), > > value 0x42: > > > > case 0x42: /* FT2 sets output freq with this, go figure */ > > qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" > > " should\n"); > > break; <------- > > case 0x41: > > It seems this is an intentional fallthrough, I understand cmd 0x42 is > expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). It might be nice to turn on -Wimplicit-fallthrough and then annotate valid locations like this in qemu with /* fallthrough */ Although GCC has an __attribute((fallthrough)), the warning flag impl also looks for that magic comment, and the magic comment is portable to clang too. Regards, Daniel
On 02/08/2018 10:01 AM, Peter Maydell wrote: > On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Hi Daniel, >> >> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: >>> This patch adds a break in the switch() statement of complete(), >>> value 0x42: >>> >>> case 0x42: /* FT2 sets output freq with this, go figure */ >>> qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" >>> " should\n"); >>> break; <------- >>> case 0x41: >> >> It seems this is an intentional fallthrough, I understand cmd 0x42 is >> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). > > Yes, I agree; I wrote a bit about this in this thread: > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html Oh, very useful link! > (though my guess is that actually 0x42 is supposed to do exactly > what 0x41 does, and that the LOG_UNIMP should maybe just be removed). I now understand 0x42 sets the dsp input sampling freq, the model seems to be designed with output in mind, then added input support (using same freq as output). So imho the simpler/safer fix would be: case 0x42: if (dsp_get_hilo(s) != s->freq) { qemu_log_mask(LOG_UNIMP, "input sampling freq different than " "output not implemented"); } /* fallthrough */ case 0x41: ... and the correct fix would be split s->freq in {s->freq_in, s->freq_out} but nobody ever required this during at least 14 years.
On 02/08/2018 10:16 AM, Philippe Mathieu-Daudé wrote: > On 02/08/2018 10:01 AM, Peter Maydell wrote: >> On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> Hi Daniel, >>> >>> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: >>>> This patch adds a break in the switch() statement of complete(), >>>> value 0x42: >>>> >>>> case 0x42: /* FT2 sets output freq with this, go figure */ >>>> qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" >>>> " should\n"); >>>> break; <------- >>>> case 0x41: >>> >>> It seems this is an intentional fallthrough, I understand cmd 0x42 is >>> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). >> >> Yes, I agree; I wrote a bit about this in this thread: >> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html > > Oh, very useful link! > >> (though my guess is that actually 0x42 is supposed to do exactly >> what 0x41 does, and that the LOG_UNIMP should maybe just be removed). > > I now understand 0x42 sets the dsp input sampling freq, the model seems > to be designed with output in mind, then added input support (using same > freq as output). Now I see Fabrice comment "FT2 sets output freq with this, go figure" and agree with him. I like to think this is a bug in Fast Tracker 2, so Peter suggestion about using LOG_GUEST_ERROR here might be clever. > > So imho the simpler/safer fix would be: > > case 0x42: > if (dsp_get_hilo(s) != s->freq) { > qemu_log_mask(LOG_UNIMP, > "input sampling freq different than " > "output not implemented"); > } > /* fallthrough */ > case 0x41: > ... > > and the correct fix would be split s->freq in {s->freq_in, s->freq_out} > but nobody ever required this during at least 14 years. >
On 8 February 2018 at 13:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Now I see Fabrice comment "FT2 sets output freq with this, go figure" > and agree with him. > > I like to think this is a bug in Fast Tracker 2, so Peter suggestion > about using LOG_GUEST_ERROR here might be clever. > >> >> So imho the simpler/safer fix would be: >> >> case 0x42: >> if (dsp_get_hilo(s) != s->freq) { >> qemu_log_mask(LOG_UNIMP, >> "input sampling freq different than " >> "output not implemented"); >> } >> /* fallthrough */ >> case 0x41: >> ... Wouldn't this falsely report a warning for guest code that really is trying to set the input sampling frequency and doesn't care about output? >> and the correct fix would be split s->freq in {s->freq_in, s->freq_out} ...but that would differ from the hardware implementation, which (apparently) uses a single frequency for both. thanks -- PMM
Hey, On 02/08/2018 10:15 AM, Philippe Mathieu-Daudé wrote: > Hi Daniel, > > On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote: >> This patch adds a break in the switch() statement of complete(), >> value 0x42: >> >> case 0x42: /* FT2 sets output freq with this, go figure */ >> qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" >> " should\n"); >> break; <------- >> case 0x41: > It seems this is an intentional fallthrough, I understand cmd 0x42 is > expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415). Perhaps it should be more explicit then? Something like case 0x40: s->time_const = dsp_get_data (s); ldebug ("set time const %d\n", s->time_const); break; case 0x41: case 0x42: if (s->cmd == 0x42) { /* FT2 sets output freq with this, go figure */ qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" " should\n"); } s->freq = dsp_get_hilo (s); ldebug ("set freq %d\n", s->freq); break; case 0x48: > >> The issue was found by Coverity (#1385841): >> >> CID 1385841: Control flow issues (MISSING_BREAK) >> The case for value "66" is not terminated by a 'break' statement. >> >> Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()") >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> CC: John Arbuckle <programmingkidx@gmail.com> >> CC: Gerd Hoffmann <kraxel@redhat.com> >> --- >> hw/audio/sb16.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c >> index 31de264ab7..b2fdcd8437 100644 >> --- a/hw/audio/sb16.c >> +++ b/hw/audio/sb16.c >> @@ -744,6 +744,7 @@ static void complete (SB16State *s) >> case 0x42: /* FT2 sets output freq with this, go figure */ >> qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" >> " should\n"); >> + break; >> case 0x41: >> s->freq = dsp_get_hilo (s); >> ldebug ("set freq %d\n", s->freq); >>
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c index 31de264ab7..b2fdcd8437 100644 --- a/hw/audio/sb16.c +++ b/hw/audio/sb16.c @@ -744,6 +744,7 @@ static void complete (SB16State *s) case 0x42: /* FT2 sets output freq with this, go figure */ qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" " should\n"); + break; case 0x41: s->freq = dsp_get_hilo (s); ldebug ("set freq %d\n", s->freq);
This patch adds a break in the switch() statement of complete(), value 0x42: case 0x42: /* FT2 sets output freq with this, go figure */ qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it" " should\n"); break; <------- case 0x41: The issue was found by Coverity (#1385841): CID 1385841: Control flow issues (MISSING_BREAK) The case for value "66" is not terminated by a 'break' statement. Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()") Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> CC: John Arbuckle <programmingkidx@gmail.com> CC: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/sb16.c | 1 + 1 file changed, 1 insertion(+)