From 13606b99515e8c5f81eab7fd88a70fb2ad506cd8 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 29 Jun 2018 15:11:19 +0100 Subject: sd: Don't trace SDRequest crc field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Message-id: 20180626180324.5537-1-peter.maydell@linaro.org --- hw/sd/core.c | 2 +- hw/sd/trace-events | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'hw/sd') diff --git a/hw/sd/core.c b/hw/sd/core.c index 820345f704..107e6d71dd 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 bfd1d62efc..43cffab8b1 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)" -- cgit v1.2.3 From b3141c0625a18d35c45c175a20826271b3241d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 29 Jun 2018 15:11:20 +0100 Subject: sdcard: Use the ldst API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The load/store API will ease further code movement. Per the Physical Layer Simplified Spec. "3.6 Bus Protocol": "In the CMD line the Most Significant Bit (MSB) is transmitted first, the Least Significant Bit (LSB) is the last." Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/sd/bcm2835_sdhost.c | 13 +++++-------- hw/sd/milkymist-memcard.c | 3 +-- hw/sd/omap_mmc.c | 6 ++---- hw/sd/pl181.c | 11 ++++------- hw/sd/sdhci.c | 15 +++++---------- hw/sd/ssi-sd.c | 6 ++---- 6 files changed, 19 insertions(+), 35 deletions(-) (limited to 'hw/sd') diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c index ebf3b926c2..4df4de7d67 100644 --- a/hw/sd/bcm2835_sdhost.c +++ b/hw/sd/bcm2835_sdhost.c @@ -118,8 +118,6 @@ static void bcm2835_sdhost_send_command(BCM2835SDHostState *s) goto error; } if (!(s->cmd & SDCMD_NO_RESPONSE)) { -#define RWORD(n) (((uint32_t)rsp[n] << 24) | (rsp[n + 1] << 16) \ - | (rsp[n + 2] << 8) | rsp[n + 3]) if (rlen == 0 || (rlen == 4 && (s->cmd & SDCMD_LONG_RESPONSE))) { goto error; } @@ -127,15 +125,14 @@ static void bcm2835_sdhost_send_command(BCM2835SDHostState *s) goto error; } if (rlen == 4) { - s->rsp[0] = RWORD(0); + s->rsp[0] = ldl_be_p(&rsp[0]); s->rsp[1] = s->rsp[2] = s->rsp[3] = 0; } else { - s->rsp[0] = RWORD(12); - s->rsp[1] = RWORD(8); - s->rsp[2] = RWORD(4); - s->rsp[3] = RWORD(0); + s->rsp[0] = ldl_be_p(&rsp[12]); + s->rsp[1] = ldl_be_p(&rsp[8]); + s->rsp[2] = ldl_be_p(&rsp[4]); + s->rsp[3] = ldl_be_p(&rsp[0]); } -#undef RWORD } /* We never really delay commands, so if this was a 'busywait' command * then we've completed it now and can raise the interrupt. diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c index fcbccf54ea..df42aa1c54 100644 --- a/hw/sd/milkymist-memcard.c +++ b/hw/sd/milkymist-memcard.c @@ -100,8 +100,7 @@ static void memcard_sd_command(MilkymistMemcardState *s) SDRequest req; req.cmd = s->command[0] & 0x3f; - req.arg = (s->command[1] << 24) | (s->command[2] << 16) - | (s->command[3] << 8) | s->command[4]; + req.arg = ldl_be_p(s->command + 1); req.crc = s->command[5]; s->response[0] = req.cmd; diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c index aa2a816f76..671264b650 100644 --- a/hw/sd/omap_mmc.c +++ b/hw/sd/omap_mmc.c @@ -163,8 +163,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir, CID_CSD_OVERWRITE; if (host->sdio & (1 << 13)) mask |= AKE_SEQ_ERROR; - rspstatus = (response[0] << 24) | (response[1] << 16) | - (response[2] << 8) | (response[3] << 0); + rspstatus = ldl_be_p(response); break; case sd_r2: @@ -182,8 +181,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir, } rsplen = 4; - rspstatus = (response[0] << 24) | (response[1] << 16) | - (response[2] << 8) | (response[3] << 0); + rspstatus = ldl_be_p(response); if (rspstatus & 0x80000000) host->status &= 0xe000; else diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c index 1cc94dbfdf..3ad7e925c5 100644 --- a/hw/sd/pl181.c +++ b/hw/sd/pl181.c @@ -182,23 +182,20 @@ static void pl181_send_command(PL181State *s) if (rlen < 0) goto error; if (s->cmd & PL181_CMD_RESPONSE) { -#define RWORD(n) (((uint32_t)response[n] << 24) | (response[n + 1] << 16) \ - | (response[n + 2] << 8) | response[n + 3]) if (rlen == 0 || (rlen == 4 && (s->cmd & PL181_CMD_LONGRESP))) goto error; if (rlen != 4 && rlen != 16) goto error; - s->response[0] = RWORD(0); + s->response[0] = ldl_be_p(&response[0]); if (rlen == 4) { s->response[1] = s->response[2] = s->response[3] = 0; } else { - s->response[1] = RWORD(4); - s->response[2] = RWORD(8); - s->response[3] = RWORD(12) & ~1; + s->response[1] = ldl_be_p(&response[4]); + s->response[2] = ldl_be_p(&response[8]); + s->response[3] = ldl_be_p(&response[12]) & ~1; } DPRINTF("Response received\n"); s->status |= PL181_STATUS_CMDRESPEND; -#undef RWORD } else { DPRINTF("Command sent\n"); s->status |= PL181_STATUS_CMDSENT; diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 3017e5a95a..321d02d75a 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -342,17 +342,13 @@ static void sdhci_send_command(SDHCIState *s) if (s->cmdreg & SDHC_CMD_RESPONSE) { if (rlen == 4) { - s->rspreg[0] = (response[0] << 24) | (response[1] << 16) | - (response[2] << 8) | response[3]; + s->rspreg[0] = ldl_be_p(response); s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0; trace_sdhci_response4(s->rspreg[0]); } else if (rlen == 16) { - s->rspreg[0] = (response[11] << 24) | (response[12] << 16) | - (response[13] << 8) | response[14]; - s->rspreg[1] = (response[7] << 24) | (response[8] << 16) | - (response[9] << 8) | response[10]; - s->rspreg[2] = (response[3] << 24) | (response[4] << 16) | - (response[5] << 8) | response[6]; + s->rspreg[0] = ldl_be_p(&response[11]); + s->rspreg[1] = ldl_be_p(&response[7]); + s->rspreg[2] = ldl_be_p(&response[3]); s->rspreg[3] = (response[0] << 16) | (response[1] << 8) | response[2]; trace_sdhci_response16(s->rspreg[3], s->rspreg[2], @@ -396,8 +392,7 @@ static void sdhci_end_transfer(SDHCIState *s) trace_sdhci_end_transfer(request.cmd, request.arg); sdbus_do_command(&s->sdbus, &request, response); /* Auto CMD12 response goes to the upper Response register */ - s->rspreg[3] = (response[0] << 24) | (response[1] << 16) | - (response[2] << 8) | response[3]; + s->rspreg[3] = ldl_be_p(response); } s->prnsts &= ~(SDHC_DOING_READ | SDHC_DOING_WRITE | diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 96542ecd62..95a143bfba 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -96,8 +96,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val) uint8_t longresp[16]; /* FIXME: Check CRC. */ request.cmd = s->cmd; - request.arg = (s->cmdarg[0] << 24) | (s->cmdarg[1] << 16) - | (s->cmdarg[2] << 8) | s->cmdarg[3]; + request.arg = ldl_be_p(s->cmdarg); DPRINTF("CMD%d arg 0x%08x\n", s->cmd, request.arg); s->arglen = sdbus_do_command(&s->sdbus, &request, longresp); if (s->arglen <= 0) { @@ -122,8 +121,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val) /* CMD13 returns a 2-byte statuse work. Other commands only return the first byte. */ s->arglen = (s->cmd == 13) ? 2 : 1; - cardstatus = (longresp[0] << 24) | (longresp[1] << 16) - | (longresp[2] << 8) | longresp[3]; + cardstatus = ldl_be_p(longresp); status = 0; if (((cardstatus >> 9) & 0xf) < 4) status |= SSI_SDR_IDLE; -- cgit v1.2.3