Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Philippe Mathieu-Daudé]

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é <address@hidden> 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.
>