[Qemu-devel] [PATCH 0/2] ahci: small cleanups

[Qemu-devel] [PATCH 0/2] ahci: small cleanups

[Stefan Hajnoczi]

Two small cleanups for QEMU 2.3:

1. Avoid DEBUG_AHCI bitrot
2. Use FIS type constants instead of magic numbers

See commit descriptions for details.

Stefan Hajnoczi (2):
  ahci: avoid #ifdef DEBUG_AHCI bitrot
  ahci: replace SATA FIS type magic numbers with constants

 hw/ide/ahci.c | 24 +++++++++++-------------
 hw/ide/ahci.h |  5 ++++-
 2 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] ahci: avoid #ifdef DEBUG_AHCI bitrot

[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

[Qemu-devel] [PATCH 2/2] ahci: replace SATA FIS type magic numbers with constants

[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

Re: [Qemu-devel] [PATCH 0/2] ahci: small cleanups

[John Snow]

On 11/13/2014 05:24 AM, Stefan Hajnoczi wrote:
> Two small cleanups for QEMU 2.3:
>
> 1. Avoid DEBUG_AHCI bitrot
> 2. Use FIS type constants instead of magic numbers
>
> See commit descriptions for details.
>
> Stefan Hajnoczi (2):
>    ahci: avoid #ifdef DEBUG_AHCI bitrot
>    ahci: replace SATA FIS type magic numbers with constants
>
>   hw/ide/ahci.c | 24 +++++++++++-------------
>   hw/ide/ahci.h |  5 ++++-
>   2 files changed, 15 insertions(+), 14 deletions(-)
>

Thanks!

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] ahci: small cleanups

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Thu, Nov 13, 2014 at 10:24:39AM +0000, Stefan Hajnoczi wrote:
> Two small cleanups for QEMU 2.3:
> 
> 1. Avoid DEBUG_AHCI bitrot
> 2. Use FIS type constants instead of magic numbers
> 
> See commit descriptions for details.
> 
> Stefan Hajnoczi (2):
>   ahci: avoid #ifdef DEBUG_AHCI bitrot
>   ahci: replace SATA FIS type magic numbers with constants
> 
>  hw/ide/ahci.c | 24 +++++++++++-------------
>  hw/ide/ahci.h |  5 ++++-
>  2 files changed, 15 insertions(+), 14 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]