Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Philippe Mathieu-Daudé]

On 04/30/2018 10:49 AM, Edgar E. Iglesias wrote:
> On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
>> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <address@hidden> wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> Reviewed-by: Alistair Francis <address@hidden>
>>
>>> @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, 
>>> uint8_t *response)
>>>  {
>>>      SDState *card = get_card(sdbus);
>>>
>>> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
>>>      if (card) {
>>>          SDCardClass *sc = SD_CARD_GET_CLASS(card);
>>
>> Hi -- as a result of this trace event being added, we now get
>> warnings from Coverity that it might use uninitialized data
>> (CID1386074, CID1390571, CID1386072, CID1386076). This is because not
>> all of our SD
>> controllers bother to initialize req->crc, because up til now
>> nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
>> sdhci.c and ssi-sd.c do this).

I was pretty sure we ran Coverity before the stable release :|
Maybe they added new 'uninitialized data' checks recently.

>>
>> Could you have a look at this, please? I think there are
>> three plausible lines of approach:
>>
>> (1) just don't bother to trace the CRC field
>> (2) make bcm2835_sdhost.c, sdhci.c, ssi-sd.c set crc to 0 like
>> omap and pxa2xx already do
> 
> Hi,
> 
> Philippe, any opinion here?
> 
> Otherwise I suggest we do #2 right away to avoid the warnings
> until someone cares enough to implement #3...

I prefer to start with #3 directly, else we'll keep this forever.

> 
> Cheers,
> Edgar
> 
> 
> 
>> (3) "proper" implementation of CRC, so that an sd controller
>> can either (a) mark the SDRequest as "no CRC" and have
>> sd_req_crc_validate() always pass, or (b) mark the SDRequest
>> as having a CRC and have sd_req_crc_validate() actually
>> do the check which it currently stubs out with "return 0"
>>
>> (3 is because it doesn't seem very sensible to spend time
>> calculating a CRC just to test it against a CRC calculated
>> a little bit later on; but we don't really want to drop the
>> CRC stuff entirely because on some controllers like
>> milkymist-memcard.c the CRC byte comes from the guest and
>> we'd ideally like to check it.)
>>
>> I don't have a strong preference for which of 1/2/3 we do.
>>
>> PS: CID1005332 is the coverity issue for "sd_req_crc_validate
>> stubs out its check code with 'return 0' leaving a line of
>> unreachable code".
>>
>> thanks
>> -- PMM
>