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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Reviewed-by: Alistair Francis <alistair.francis@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