On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> > Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
>
> > @@ -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
Hi,
Philippe, any opinion here?
Otherwise I suggest we do #2 right away to avoid the warnings
until someone cares enough to implement #3...
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
