* [Qemu-devel] [PATCH 1/2] ahci: avoid #ifdef DEBUG_AHCI bitrot
2014-11-13 10:24 [Qemu-devel] [PATCH 0/2] ahci: small cleanups Stefan Hajnoczi
@ 2014-11-13 10:24 ` Stefan Hajnoczi
2014-11-13 10:24 ` [Qemu-devel] [PATCH 2/2] ahci: replace SATA FIS type magic numbers with constants Stefan Hajnoczi
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2014-11-13 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, Stefan Hajnoczi
Debug code using #ifdef is susceptible to bitrot because the compiler
never checks the debug code.
This is easy to avoid, change the DPRINTF() macro to use if (DEBUG_AHCI)
and always give it a 0 or 1 value.
This also allows us to drop an #ifdef DEBUG_AHCI in ahci_start_dma()
since the compiler can now see the local variable is used.
The motivation for this change is a recent DEBUG_AHCI build failure due
to an outdated DPRINTF() format string. From now on the compiler will
catch these errors.
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/ide/ahci.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 61dbed1..3f1e39e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -34,15 +34,15 @@
#include <hw/ide/pci.h>
#include <hw/ide/ahci.h>
-/* #define DEBUG_AHCI */
+#define DEBUG_AHCI 0
-#ifdef DEBUG_AHCI
#define DPRINTF(port, fmt, ...) \
-do { fprintf(stderr, "ahci: %s: [%d] ", __FUNCTION__, port); \
- fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(port, fmt, ...) do {} while(0)
-#endif
+do { \
+ if (DEBUG_AHCI) { \
+ fprintf(stderr, "ahci: %s: [%d] ", __func__, port); \
+ fprintf(stderr, fmt, ## __VA_ARGS__); \
+ } \
+} while (0)
static void check_cmd(AHCIState *s, int port);
static int handle_cmd(AHCIState *s,int port,int slot);
@@ -551,7 +551,7 @@ static void ahci_reset_port(AHCIState *s, int port)
static void debug_print_fis(uint8_t *fis, int cmd_len)
{
-#ifdef DEBUG_AHCI
+#if DEBUG_AHCI
int i;
fprintf(stderr, "fis:");
@@ -1126,9 +1126,7 @@ out:
static void ahci_start_dma(IDEDMA *dma, IDEState *s,
BlockCompletionFunc *dma_cb)
{
-#ifdef DEBUG_AHCI
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-#endif
DPRINTF(ad->port_no, "\n");
s->io_buffer_offset = 0;
dma_cb(s, 0);
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH 2/2] ahci: replace SATA FIS type magic numbers with constants
2014-11-13 10:24 [Qemu-devel] [PATCH 0/2] ahci: small cleanups Stefan Hajnoczi
2014-11-13 10:24 ` [Qemu-devel] [PATCH 1/2] ahci: avoid #ifdef DEBUG_AHCI bitrot Stefan Hajnoczi
@ 2014-11-13 10:24 ` Stefan Hajnoczi
2014-11-13 18:02 ` [Qemu-devel] [PATCH 0/2] ahci: small cleanups John Snow
2014-11-26 16:03 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2014-11-13 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, Stefan Hajnoczi
SATA 3.0 "10.3.1 FIS Type values" defines the constants used to
differentiate between FIS types.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/ide/ahci.c | 6 +++---
hw/ide/ahci.h | 5 ++++-
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3f1e39e..e4cd7e5 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -580,7 +580,7 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
sdb_fis = (SDBFIS *)&ad->res_fis[RES_FIS_SDBFIS];
ide_state = &ad->port.ifs[0];
- sdb_fis->type = 0xA1;
+ sdb_fis->type = SATA_FIS_TYPE_SDB;
/* Interrupt pending & Notification bit */
sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
sdb_fis->status = ide_state->status & 0x77;
@@ -631,7 +631,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
pio_fis = &ad->res_fis[RES_FIS_PSFIS];
- pio_fis[0] = 0x5f;
+ pio_fis[0] = SATA_FIS_TYPE_PIO_SETUP;
pio_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
pio_fis[2] = s->status;
pio_fis[3] = s->error;
@@ -690,7 +690,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
d2h_fis = &ad->res_fis[RES_FIS_RFIS];
- d2h_fis[0] = 0x34;
+ d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H;
d2h_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
d2h_fis[2] = s->status;
d2h_fis[3] = s->error;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index b123237..09564bb 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -156,7 +156,10 @@
#define AHCI_SCR_SCTL_DET 0xf
#define SATA_FIS_TYPE_REGISTER_H2D 0x27
-#define SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER 0x80
+#define SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER 0x80
+#define SATA_FIS_TYPE_REGISTER_D2H 0x34
+#define SATA_FIS_TYPE_PIO_SETUP 0x5f
+#define SATA_FIS_TYPE_SDB 0xA1
#define AHCI_CMD_HDR_CMD_FIS_LEN 0x1f
#define AHCI_CMD_HDR_PRDT_LEN 16
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread