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

[Peter Maydell]

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).

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
(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