[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement
Date: Thu, 8 Feb 2018 10:34:33 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]