[Qemu-devel] [PATCH] sd: Don't trace SDRequest crc field

[Qemu-devel] [PATCH] sd: Don't trace SDRequest crc field

[Peter Maydell]

We don't actually implement SD command CRC checking, because
for almost all of our SD controllers the CRC generation is
done in hardware, and so modelling CRC generation and checking
would be a bit pointless. (The exception is that milkymist-memcard
makes the guest software compute the CRC.)

As a result almost all of our SD controller models don't bother
to set the SDRequest crc field, and the SD card model doesn't
check it. So the tracing of it in sdbus_do_command() provokes
Coverity warnings about use of uninitialized data.

Drop the CRC field from the trace; we can always add it back
if and when we do anything useful with the CRC.

Fixes Coverity issues 1386072, 1386074, 1386076, 1390571.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We might also want to try to improve our CRC handling
(eg so that the controller can say "I have not set the CRC,
don't bother checking it" or "I have set the CRC, do check
it because it's come from the guest software"), but let's
put in the simple tweak to make Coverity happy for the moment.
---
 hw/sd/core.c       | 2 +-
 hw/sd/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/core.c b/hw/sd/core.c
index 820345f704b..107e6d71ddb 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -91,7 +91,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);
+    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg);
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index bfd1d62efcb..43cffab8b17 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -7,7 +7,7 @@ bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
 bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
 
 # 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_command(const char *bus_name, uint8_t cmd, uint32_t arg) "@%s CMD%02d arg 0x%08x"
 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"
 sdbus_set_voltage(const char *bus_name, uint16_t millivolts) "@%s %u (mV)"
-- 
2.17.1

Re: [Qemu-devel] [PATCH] sd: Don't trace SDRequest crc field

[Philippe Mathieu-Daudé]

On 06/26/2018 03:03 PM, Peter Maydell wrote:
> We don't actually implement SD command CRC checking, because
> for almost all of our SD controllers the CRC generation is
> done in hardware, and so modelling CRC generation and checking
> would be a bit pointless. (The exception is that milkymist-memcard
> makes the guest software compute the CRC.)
> 
> As a result almost all of our SD controller models don't bother
> to set the SDRequest crc field, and the SD card model doesn't
> check it. So the tracing of it in sdbus_do_command() provokes
> Coverity warnings about use of uninitialized data.
> 
> Drop the CRC field from the trace; we can always add it back
> if and when we do anything useful with the CRC.
> 
> Fixes Coverity issues 1386072, 1386074, 1386076, 1390571.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> We might also want to try to improve our CRC handling
> (eg so that the controller can say "I have not set the CRC,
> don't bother checking it" or "I have set the CRC, do check
> it because it's come from the guest software"), but let's
> put in the simple tweak to make Coverity happy for the moment.

Thanks for this simple patch... So simple I couldn't even think of it...

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/sd/core.c       | 2 +-
>  hw/sd/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index 820345f704b..107e6d71ddb 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -91,7 +91,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);
> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg);
>      if (card) {
>          SDCardClass *sc = SD_CARD_GET_CLASS(card);
>  
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index bfd1d62efcb..43cffab8b17 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -7,7 +7,7 @@ bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
>  bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
>  
>  # 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_command(const char *bus_name, uint8_t cmd, uint32_t arg) "@%s CMD%02d arg 0x%08x"
>  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"
>  sdbus_set_voltage(const char *bus_name, uint16_t millivolts) "@%s %u (mV)"
> 

Re: [Qemu-devel] [PATCH] sd: Don't trace SDRequest crc field

[Peter Maydell]

On 26 June 2018 at 19:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 06/26/2018 03:03 PM, Peter Maydell wrote:
>> We don't actually implement SD command CRC checking, because
>> for almost all of our SD controllers the CRC generation is
>> done in hardware, and so modelling CRC generation and checking
>> would be a bit pointless. (The exception is that milkymist-memcard
>> makes the guest software compute the CRC.)
>>
>> As a result almost all of our SD controller models don't bother
>> to set the SDRequest crc field, and the SD card model doesn't
>> check it. So the tracing of it in sdbus_do_command() provokes
>> Coverity warnings about use of uninitialized data.
>>
>> Drop the CRC field from the trace; we can always add it back
>> if and when we do anything useful with the CRC.
>>
>> Fixes Coverity issues 1386072, 1386074, 1386076, 1390571.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> We might also want to try to improve our CRC handling
>> (eg so that the controller can say "I have not set the CRC,
>> don't bother checking it" or "I have set the CRC, do check
>> it because it's come from the guest software"), but let's
>> put in the simple tweak to make Coverity happy for the moment.
>
> Thanks for this simple patch... So simple I couldn't even think of it...
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks; applied to target-arm.next.

-- PMM