| Message ID | 20180213040809.26021-21-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | SDHCI: clean v1/2 Specs, UHS-I cards tuning sequence | expand |
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 (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
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
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é <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). 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 >
On 1 May 2018 at 04:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > 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é <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). > > I was pretty sure we ran Coverity before the stable release :| > Maybe they added new 'uninitialized data' checks recently. No, we didn't get a coverity run for a long time (a combination of Scan being down and then requiring a new version of the local build tools, which we didn't realize because the website didn't give a useful error message for that case). So this most recent run has brought up a pile of accumulated issues since about mid-february. thanks -- PMM
Hi Peter, Edgar, On 05/01/2018 06:03 AM, Peter Maydell wrote: > On 1 May 2018 at 04:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> 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é <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). >> >> I was pretty sure we ran Coverity before the stable release :| >> Maybe they added new 'uninitialized data' checks recently. > > No, we didn't get a coverity run for a long time (a combination > of Scan being down and then requiring a new version of the local > build tools, which we didn't realize because the website didn't > give a useful error message for that case). So this most recent > run has brought up a pile of accumulated issues since about > mid-february. I sent a fix for those here: http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg01130.html I went to the Coverity dashboard to update the Triage information but the 'apply' box appears grey so I can not update those CIDs. Regards, Phil.
On 4 May 2018 at 17:11, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > I went to the Coverity dashboard to update the Triage information but > the 'apply' box appears grey so I can not update those CIDs. You're only registered with Scan as a "Defect Viewer (read-only)". The Scan UI appears to be busted and doesn't provide any way for me as an admin to upgrade your account rights :-/ (except possibly by removing you from the user list entirely and having you reapply...) thanks -- PMM
diff --git a/hw/sd/core.c b/hw/sd/core.c index 295dc44ab7..498284f109 100644 --- a/hw/sd/core.c +++ b/hw/sd/core.c @@ -23,6 +23,12 @@ #include "hw/qdev-core.h" #include "sysemu/block-backend.h" #include "hw/sd/sd.h" +#include "trace.h" + +static inline const char *sdbus_name(SDBus *sdbus) +{ + return sdbus->qbus.name; +} static SDState *get_card(SDBus *sdbus) { @@ -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); @@ -52,6 +59,7 @@ void sdbus_write_data(SDBus *sdbus, uint8_t value) { SDState *card = get_card(sdbus); + trace_sdbus_write(sdbus_name(sdbus), value); if (card) { SDCardClass *sc = SD_CARD_GET_CLASS(card); @@ -62,14 +70,16 @@ void sdbus_write_data(SDBus *sdbus, uint8_t value) uint8_t sdbus_read_data(SDBus *sdbus) { SDState *card = get_card(sdbus); + uint8_t value = 0; if (card) { SDCardClass *sc = SD_CARD_GET_CLASS(card); - return sc->read_data(card); + value = sc->read_data(card); } + trace_sdbus_read(sdbus_name(sdbus), value); - return 0; + return value; } bool sdbus_data_ready(SDBus *sdbus) diff --git a/hw/sd/trace-events b/hw/sd/trace-events index 78d8707669..ea2746c8b7 100644 --- a/hw/sd/trace-events +++ b/hw/sd/trace-events @@ -1,5 +1,10 @@ # See docs/devel/tracing.txt for syntax documentation. +# hw/sd/core.c +sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s CMD%02d arg 0x%08x crc 0x%02x" +sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x" +sdbus_write(const char *bus_name, uint8_t value) "@%s value 0x%02x" + # hw/sd/sdhci.c sdhci_set_inserted(const char *level) "card state changed: %s" sdhci_send_command(uint8_t cmd, uint32_t arg) "CMD%02u ARG[0x%08x]"