[Qemu-trivial] [PATCH v2] hw/misc: add missing includes

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Qemu-trivial] [PATCH v2] hw/misc: add missing includes
@ 2017-06-09 15:06 ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Peter Maydell, Eric Blake
  Cc: Philippe Mathieu-Daudé

inlined create_unimplemented_device() calls sysbus_mmio_map_overlap().

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

v2: Addressed review feedback from Eric Blake

 include/hw/misc/unimp.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
index 3462d85836..52e068ec3e 100644
--- a/include/hw/misc/unimp.h
+++ b/include/hw/misc/unimp.h
@@ -8,6 +8,8 @@
 #ifndef HW_MISC_UNIMP_H
 #define HW_MISC_UNIMP_H
 
+#include "hw/sysbus.h"
+
 #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
 
 /**
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v2] hw/misc: add missing includes
@ 2017-06-09 15:06 ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Peter Maydell, Eric Blake
  Cc: Philippe Mathieu-Daudé

inlined create_unimplemented_device() calls sysbus_mmio_map_overlap().

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

v2: Addressed review feedback from Eric Blake

 include/hw/misc/unimp.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
index 3462d85836..52e068ec3e 100644
--- a/include/hw/misc/unimp.h
+++ b/include/hw/misc/unimp.h
@@ -8,6 +8,8 @@
 #ifndef HW_MISC_UNIMP_H
 #define HW_MISC_UNIMP_H
 
+#include "hw/sysbus.h"
+
 #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
 
 /**
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-trivial] [PATCH v2] hw/pci: define msi_nonbroken in pci-stub
  2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-09 15:06   ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Michael S . Tsirkin, Thomas Huth
  Cc: Philippe Mathieu-Daudé

The kludged field 'msi_nonbroken' is declared in "hw/pci/msi.h" and defined in
hw/pci/msi.c.
When using an ARM config with CONFIG_PCI disabled, hw/pci/msi.c is not included.
Without being PCI-related, the files hw/intc/arm_gicv[23*].c do access this
field (to enable the kludge if PCI is enabled).
The final link fails since hw/pci/msi.c is not included.
Defining this field in pci-stub is safe enough for configs without CONFIG_PCI.

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

v2: Addressed review feedback from Thomas Huth

Error reported:

$ ../configure --target-list=armeb-softmmu && make
[...]
  LINK    armeb-softmmu/qemu-system-armeb
../hw/intc/arm_gicv2m.o: In function `gicv2m_realize':
qemu/hw/intc/arm_gicv2m.c:154: undefined reference to `msi_nonbroken'
../hw/intc/arm_gicv3_its_common.o: In function `gicv3_its_init_mmio':
qemu/hw/intc/arm_gicv3_its_common.c:110: undefined reference to `msi_nonbroken'
collect2: error: ld returned 1 exit status
Makefile:201: recipe for target 'qemu-system-armeb' failed
make[1]: *** [qemu-system-armeb] Error 1
Makefile:327: recipe for target 'subdir-armeb-softmmu' failed
make: *** [subdir-armeb-softmmu] Error 2

branch used (wip):

https://github.com/qemu/qemu/compare/master...philmd:armeb_wip

with following config:

$ cat default-configs/armeb-softmmu.mak
# Default configuration for armeb-softmmu
CONFIG_ECC=y
CONFIG_SERIAL=y
CONFIG_PTIMER=y
CONFIG_SD=y
CONFIG_SSI=y
#CONFIG_USB=y
CONFIG_PLATFORM_BUS=y
CONFIG_ARM_V7M=y
CONFIG_ARM_GIC=y
CONFIG_ARM_TIMER=y
CONFIG_PL011=y
CONFIG_PL022=y
CONFIG_PL031=y
CONFIG_PL041=y
CONFIG_PL050=y
CONFIG_PL061=y
CONFIG_PL080=y
CONFIG_PL181=y
CONFIG_PL190=y
CONFIG_PL310=y
CONFIG_PL330=y
CONFIG_BITBANG_I2C=y
CONFIG_GPIO_KEY=y
CONFIG_HERCULES=y

 hw/pci/pci-stub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index 36d2c430c5..ecad664946 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -24,6 +24,9 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/pci/pci.h"
 #include "qmp-commands.h"
+#include "hw/pci/msi.h"
+
+bool msi_nonbroken;
 
 PciInfoList *qmp_query_pci(Error **errp)
 {
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v2] hw/pci: define msi_nonbroken in pci-stub
@ 2017-06-09 15:06   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Michael S . Tsirkin, Thomas Huth
  Cc: Philippe Mathieu-Daudé

The kludged field 'msi_nonbroken' is declared in "hw/pci/msi.h" and defined in
hw/pci/msi.c.
When using an ARM config with CONFIG_PCI disabled, hw/pci/msi.c is not included.
Without being PCI-related, the files hw/intc/arm_gicv[23*].c do access this
field (to enable the kludge if PCI is enabled).
The final link fails since hw/pci/msi.c is not included.
Defining this field in pci-stub is safe enough for configs without CONFIG_PCI.

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

v2: Addressed review feedback from Thomas Huth

Error reported:

$ ../configure --target-list=armeb-softmmu && make
[...]
  LINK    armeb-softmmu/qemu-system-armeb
../hw/intc/arm_gicv2m.o: In function `gicv2m_realize':
qemu/hw/intc/arm_gicv2m.c:154: undefined reference to `msi_nonbroken'
../hw/intc/arm_gicv3_its_common.o: In function `gicv3_its_init_mmio':
qemu/hw/intc/arm_gicv3_its_common.c:110: undefined reference to `msi_nonbroken'
collect2: error: ld returned 1 exit status
Makefile:201: recipe for target 'qemu-system-armeb' failed
make[1]: *** [qemu-system-armeb] Error 1
Makefile:327: recipe for target 'subdir-armeb-softmmu' failed
make: *** [subdir-armeb-softmmu] Error 2

branch used (wip):

https://github.com/qemu/qemu/compare/master...philmd:armeb_wip

with following config:

$ cat default-configs/armeb-softmmu.mak
# Default configuration for armeb-softmmu
CONFIG_ECC=y
CONFIG_SERIAL=y
CONFIG_PTIMER=y
CONFIG_SD=y
CONFIG_SSI=y
#CONFIG_USB=y
CONFIG_PLATFORM_BUS=y
CONFIG_ARM_V7M=y
CONFIG_ARM_GIC=y
CONFIG_ARM_TIMER=y
CONFIG_PL011=y
CONFIG_PL022=y
CONFIG_PL031=y
CONFIG_PL041=y
CONFIG_PL050=y
CONFIG_PL061=y
CONFIG_PL080=y
CONFIG_PL181=y
CONFIG_PL190=y
CONFIG_PL310=y
CONFIG_PL330=y
CONFIG_BITBANG_I2C=y
CONFIG_GPIO_KEY=y
CONFIG_HERCULES=y

 hw/pci/pci-stub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index 36d2c430c5..ecad664946 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -24,6 +24,9 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/pci/pci.h"
 #include "qmp-commands.h"
+#include "hw/pci/msi.h"
+
+bool msi_nonbroken;
 
 PciInfoList *qmp_query_pci(Error **errp)
 {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-trivial] [PATCH v2] hw/pci: define msi_nonbroken in pci-stub
  2017-06-09 15:06   ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-27 13:25     ` Michael Tokarev
  -1 siblings, 0 replies; 17+ messages in thread
From: Michael Tokarev @ 2017-06-27 13:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Michael S . Tsirkin, Thomas Huth

Applied to -trivial, thanks!

/mjt


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: define msi_nonbroken in pci-stub
@ 2017-06-27 13:25     ` Michael Tokarev
  0 siblings, 0 replies; 17+ messages in thread
From: Michael Tokarev @ 2017-06-27 13:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Michael S . Tsirkin, Thomas Huth

Applied to -trivial, thanks!

/mjt

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Qemu-trivial] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
  2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-09 15:06   ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Jason Wang, Alex Bennée
  Cc: Philippe Mathieu-Daudé

Ensure the format strings are always preprocessed, so any problem get reported.

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

v2: Addressed review feedback from Alex Bennée
    No changes in *printf() calls but using qemu_log_mask() is an option.

 hw/net/pcnet-pci.c |  13 ----
 hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
 2 files changed, 96 insertions(+), 107 deletions(-)

diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 0acf8a4879..331370a1f2 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -38,14 +38,6 @@
 
 #include "pcnet.h"
 
-//#define PCNET_DEBUG
-//#define PCNET_DEBUG_IO
-//#define PCNET_DEBUG_BCR
-//#define PCNET_DEBUG_CSR
-//#define PCNET_DEBUG_RMD
-//#define PCNET_DEBUG_TMD
-//#define PCNET_DEBUG_MATCH
-
 #define TYPE_PCI_PCNET "pcnet"
 
 #define PCI_PCNET(obj) \
@@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
     PCNetState *s = &d->state;
     uint8_t *pci_conf;
 
-#if 0
-    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
-        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
-#endif
-
     pci_conf = pci_dev->config;
 
     pci_set_word(pci_conf + PCI_STATUS,
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 654455355f..6e97c0b176 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -45,13 +45,13 @@
 
 #include "pcnet.h"
 
-//#define PCNET_DEBUG
-//#define PCNET_DEBUG_IO
-//#define PCNET_DEBUG_BCR
-//#define PCNET_DEBUG_CSR
-//#define PCNET_DEBUG_RMD
-//#define PCNET_DEBUG_TMD
-//#define PCNET_DEBUG_MATCH
+#define PCNET_DEBUG         0
+#define PCNET_DEBUG_IO      0
+#define PCNET_DEBUG_BCR     0
+#define PCNET_DEBUG_CSR     0
+#define PCNET_DEBUG_RMD     0
+#define PCNET_DEBUG_TMD     0
+#define PCNET_DEBUG_MATCH   0
 
 
 struct qemu_ether_header {
@@ -268,7 +268,9 @@ struct pcnet_RMD {
         GET_FIELD((T)->misc, TMDM, TDR),                \
         GET_FIELD((T)->misc, TMDM, TRC))
 
-#define PRINT_RMD(R) printf(                            \
+
+#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
+        printf(                                         \
         "RMD0 : RBADR=0x%08x\n"                         \
         "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
         "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
@@ -292,7 +294,9 @@ struct pcnet_RMD {
         GET_FIELD((R)->msg_length, RMDM, RCC),          \
         GET_FIELD((R)->msg_length, RMDM, RPC),          \
         GET_FIELD((R)->msg_length, RMDM, MCNT),         \
-        GET_FIELD((R)->msg_length, RMDM, ZEROS))
+        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
+        } while (0)
+
 
 static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
                                   hwaddr addr)
@@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
 
 #endif
 
-#define PRINT_PKTHDR(BUF) do {                  \
+#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
     struct qemu_ether_header *hdr = (void *)(BUF); \
     printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
            "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
@@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
            hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
            hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
            be16_to_cpu(hdr->ether_type));       \
-} while (0)
+    } } while (0)
 
 #define MULTICAST_FILTER_LEN 8
 
@@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
         s->csr[14] & 0xff, s->csr[14] >> 8
     };
     int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
-#ifdef PCNET_DEBUG_MATCH
-    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
-           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
-           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
-           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
-           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
-    printf("padr_match result=%d\n", result);
-#endif
+    if (PCNET_DEBUG_MATCH) {
+        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
+               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
+               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
+               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
+               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
+        printf("padr_match result=%d\n", result);
+    }
     return result;
 }
 
@@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
     static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
     struct qemu_ether_header *hdr = (void *)buf;
     int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
-#ifdef PCNET_DEBUG_MATCH
-    printf("padr_bcast result=%d\n", result);
-#endif
+    if (PCNET_DEBUG_MATCH) {
+        printf("padr_bcast result=%d\n", result);
+    }
     return result;
 }
 
@@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
     uint16_t padr[3], ladrf[4], mode;
     uint32_t rdra, tdra;
 
+    if (PCNET_DEBUG) {
+        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
+            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
+    }
     trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
 
     if (BCR_SSIZE32(s)) {
@@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
 
 static void pcnet_start(PCNetState *s)
 {
-#ifdef PCNET_DEBUG
-    printf("pcnet_start\n");
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_start\n");
+    }
 
     if (!CSR_DTX(s)) {
         s->csr[0] |= 0x0010;    /* set TXON */
@@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
 
 static void pcnet_stop(PCNetState *s)
 {
-#ifdef PCNET_DEBUG
-    printf("pcnet_stop\n");
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_stop\n");
+    }
     s->csr[0] &= ~0xffeb;
     s->csr[0] |= 0x0014;
     s->csr[4] &= ~0x02c2;
@@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
         RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
         CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
         CSR_CRST(s) = rmd.status;
-#ifdef PCNET_DEBUG_RMD_X
-        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
-                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
-                rmd.buf_length, rmd.status, rmd.msg_length);
+        if (PCNET_DEBUG_RMD) {
+            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
+                    "RMDS=0x%04x RMDM=0x%08x\n",
+                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
+                    rmd.buf_length, rmd.status, rmd.msg_length);
+        }
         PRINT_RMD(&rmd);
-#endif
     } else {
         CSR_CRBC(s) = CSR_CRST(s) = 0;
     }
@@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
         (CSR_LOOP(s) && !s->looptest)) {
         return -1;
     }
-#ifdef PCNET_DEBUG
-    printf("pcnet_receive size=%d\n", size);
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_receive size=%d\n", size);
+    }
 
     /* if too small buffer, then expand it */
     if (size < MIN_BUF_SIZE) {
@@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
                     (BCR_SWSTYLE(s) ? 16 : 8 );
                 RMDLOAD(&rmd, nrda);
                 if (GET_FIELD(rmd.status, RMDS, OWN)) {
-#ifdef PCNET_DEBUG_RMD
-                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
+                    if (PCNET_DEBUG_RMD) {
+                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
                                 rcvrc, CSR_RCVRC(s));
-#endif
+                    }
                     CSR_RCVRC(s) = rcvrc;
                     pcnet_rdte_poll(s);
                     break;
@@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
         }
 
         if (!(CSR_CRST(s) & 0x8000)) {
-#ifdef PCNET_DEBUG_RMD
-            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
-#endif
+            if (PCNET_DEBUG_RMD) {
+                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
+            }
             s->csr[0] |= 0x1000; /* Set MISS flag */
             CSR_MISSC(s)++;
         } else {
@@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
 
             if (!s->looptest) {
                 if (size > 4092) {
-#ifdef PCNET_DEBUG_RMD
-                    fprintf(stderr, "pcnet: truncates rx packet.\n");
-#endif
+                    if (PCNET_DEBUG_RMD) {
+                        fprintf(stderr, "pcnet: truncates rx packet.\n");
+                    }
                     size = 4092;
                 }
                 memcpy(src, buf, size);
@@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
                 crc_err = (*(uint32_t *)p != htonl(fcs));
             }
 
-#ifdef PCNET_DEBUG_MATCH
             PRINT_PKTHDR(buf);
-#endif
 
             RMDLOAD(&rmd, PHYSADDR(s,crda));
             /*if (!CSR_LAPPEN(s))*/
@@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
             PCNET_RECV_STORE();
             if ((remaining > 0) && CSR_NRDA(s)) {
                 hwaddr nrda = CSR_NRDA(s);
-#ifdef PCNET_DEBUG_RMD
                 PRINT_RMD(&rmd);
-#endif
                 RMDLOAD(&rmd, PHYSADDR(s,nrda));
                 if (GET_FIELD(rmd.status, RMDS, OWN)) {
                     crda = nrda;
                     PCNET_RECV_STORE();
-#ifdef PCNET_DEBUG_RMD
                     PRINT_RMD(&rmd);
-#endif
                     if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
                         RMDLOAD(&rmd, PHYSADDR(s,nrda));
                         if (GET_FIELD(rmd.status, RMDS, OWN)) {
@@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
             RMDSTORE(&rmd, PHYSADDR(s,crda));
             s->csr[0] |= 0x0400;
 
-#ifdef PCNET_DEBUG
-            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
-                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
-#endif
-#ifdef PCNET_DEBUG_RMD
+            if (PCNET_DEBUG) {
+                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
+                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
+            }
             PRINT_RMD(&rmd);
-#endif
 
             while (pktcount--) {
                 if (CSR_RCVRC(s) <= 1) {
@@ -1217,10 +1218,10 @@ txagain:
 
         TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
 
-#ifdef PCNET_DEBUG_TMD
-        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
-        PRINT_TMD(&tmd);
-#endif
+        if (PCNET_DEBUG_TMD) {
+            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
+            PRINT_TMD(&tmd);
+        }
         if (GET_FIELD(tmd.status, TMDS, STP)) {
             s->xmit_pos = 0;
             xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
@@ -1260,9 +1261,9 @@ txagain:
             goto txdone;
         }
 
-#ifdef PCNET_DEBUG
-        printf("pcnet_transmit size=%d\n", s->xmit_pos);
-#endif
+        if (PCNET_DEBUG) {
+            printf("pcnet_transmit size=%d\n", s->xmit_pos);
+        }
         if (CSR_LOOP(s)) {
             if (BCR_SWSTYLE(s) == 1)
                 add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
@@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
 static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
 {
     uint16_t val = new_value;
-#ifdef PCNET_DEBUG_CSR
-    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_CSR) {
+        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
+    }
     switch (rap) {
     case 0:
         s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
@@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
     default:
         val = s->csr[rap];
     }
-#ifdef PCNET_DEBUG_CSR
-    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_CSR) {
+        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
+    }
     return val;
 }
 
 static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
 {
     rap &= 127;
-#ifdef PCNET_DEBUG_BCR
-    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_BCR) {
+        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
+    }
     switch (rap) {
     case BCR_SWS:
         if (!(CSR_STOP(s) || CSR_SPND(s)))
@@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
             val = 0x0200;
             break;
         }
-#ifdef PCNET_DEBUG
-       printf("BCR_SWS=0x%04x\n", val);
-#endif
+        if (PCNET_DEBUG) {
+            printf("BCR_SWS=0x%04x\n", val);
+        }
         /* fall through */
     case BCR_LNKST:
     case BCR_LED1:
@@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
         val = rap < 32 ? s->bcr[rap] : 0;
         break;
     }
-#ifdef PCNET_DEBUG_BCR
-    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_BCR) {
+        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
+    }
     return val;
 }
 
@@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
 {
     PCNetState *s = opaque;
     pcnet_poll_timer(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
+    }
     if (!BCR_DWIO(s)) {
         switch (addr & 0x0f) {
         case 0x00: /* RDP */
@@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
         }
     }
     pcnet_update_irq(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
+                val & 0xffff);
+    }
     return val;
 }
 
@@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
 {
     PCNetState *s = opaque;
     pcnet_poll_timer(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
+    }
     if (BCR_DWIO(s)) {
         switch (addr & 0x0f) {
         case 0x00: /* RDP */
@@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
     } else if ((addr & 0x0f) == 0) {
         /* switch device to dword i/o mode */
         pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
-#ifdef PCNET_DEBUG_IO
-        printf("device switched into dword i/o mode\n");
-#endif
+        if (PCNET_DEBUG_IO) {
+            printf("device switched into dword i/o mode\n");
+        }
     }
     pcnet_update_irq(s);
 }
@@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
         }
     }
     pcnet_update_irq(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
+    }
     return val;
 }
 
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
@ 2017-06-09 15:06   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 15:06 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, Jason Wang, Alex Bennée
  Cc: Philippe Mathieu-Daudé

Ensure the format strings are always preprocessed, so any problem get reported.

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

v2: Addressed review feedback from Alex Bennée
    No changes in *printf() calls but using qemu_log_mask() is an option.

 hw/net/pcnet-pci.c |  13 ----
 hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
 2 files changed, 96 insertions(+), 107 deletions(-)

diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 0acf8a4879..331370a1f2 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -38,14 +38,6 @@
 
 #include "pcnet.h"
 
-//#define PCNET_DEBUG
-//#define PCNET_DEBUG_IO
-//#define PCNET_DEBUG_BCR
-//#define PCNET_DEBUG_CSR
-//#define PCNET_DEBUG_RMD
-//#define PCNET_DEBUG_TMD
-//#define PCNET_DEBUG_MATCH
-
 #define TYPE_PCI_PCNET "pcnet"
 
 #define PCI_PCNET(obj) \
@@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
     PCNetState *s = &d->state;
     uint8_t *pci_conf;
 
-#if 0
-    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
-        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
-#endif
-
     pci_conf = pci_dev->config;
 
     pci_set_word(pci_conf + PCI_STATUS,
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 654455355f..6e97c0b176 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -45,13 +45,13 @@
 
 #include "pcnet.h"
 
-//#define PCNET_DEBUG
-//#define PCNET_DEBUG_IO
-//#define PCNET_DEBUG_BCR
-//#define PCNET_DEBUG_CSR
-//#define PCNET_DEBUG_RMD
-//#define PCNET_DEBUG_TMD
-//#define PCNET_DEBUG_MATCH
+#define PCNET_DEBUG         0
+#define PCNET_DEBUG_IO      0
+#define PCNET_DEBUG_BCR     0
+#define PCNET_DEBUG_CSR     0
+#define PCNET_DEBUG_RMD     0
+#define PCNET_DEBUG_TMD     0
+#define PCNET_DEBUG_MATCH   0
 
 
 struct qemu_ether_header {
@@ -268,7 +268,9 @@ struct pcnet_RMD {
         GET_FIELD((T)->misc, TMDM, TDR),                \
         GET_FIELD((T)->misc, TMDM, TRC))
 
-#define PRINT_RMD(R) printf(                            \
+
+#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
+        printf(                                         \
         "RMD0 : RBADR=0x%08x\n"                         \
         "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
         "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
@@ -292,7 +294,9 @@ struct pcnet_RMD {
         GET_FIELD((R)->msg_length, RMDM, RCC),          \
         GET_FIELD((R)->msg_length, RMDM, RPC),          \
         GET_FIELD((R)->msg_length, RMDM, MCNT),         \
-        GET_FIELD((R)->msg_length, RMDM, ZEROS))
+        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
+        } while (0)
+
 
 static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
                                   hwaddr addr)
@@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
 
 #endif
 
-#define PRINT_PKTHDR(BUF) do {                  \
+#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
     struct qemu_ether_header *hdr = (void *)(BUF); \
     printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
            "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
@@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
            hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
            hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
            be16_to_cpu(hdr->ether_type));       \
-} while (0)
+    } } while (0)
 
 #define MULTICAST_FILTER_LEN 8
 
@@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
         s->csr[14] & 0xff, s->csr[14] >> 8
     };
     int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
-#ifdef PCNET_DEBUG_MATCH
-    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
-           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
-           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
-           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
-           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
-    printf("padr_match result=%d\n", result);
-#endif
+    if (PCNET_DEBUG_MATCH) {
+        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
+               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
+               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
+               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
+               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
+        printf("padr_match result=%d\n", result);
+    }
     return result;
 }
 
@@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
     static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
     struct qemu_ether_header *hdr = (void *)buf;
     int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
-#ifdef PCNET_DEBUG_MATCH
-    printf("padr_bcast result=%d\n", result);
-#endif
+    if (PCNET_DEBUG_MATCH) {
+        printf("padr_bcast result=%d\n", result);
+    }
     return result;
 }
 
@@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
     uint16_t padr[3], ladrf[4], mode;
     uint32_t rdra, tdra;
 
+    if (PCNET_DEBUG) {
+        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
+            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
+    }
     trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
 
     if (BCR_SSIZE32(s)) {
@@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
 
 static void pcnet_start(PCNetState *s)
 {
-#ifdef PCNET_DEBUG
-    printf("pcnet_start\n");
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_start\n");
+    }
 
     if (!CSR_DTX(s)) {
         s->csr[0] |= 0x0010;    /* set TXON */
@@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
 
 static void pcnet_stop(PCNetState *s)
 {
-#ifdef PCNET_DEBUG
-    printf("pcnet_stop\n");
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_stop\n");
+    }
     s->csr[0] &= ~0xffeb;
     s->csr[0] |= 0x0014;
     s->csr[4] &= ~0x02c2;
@@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
         RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
         CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
         CSR_CRST(s) = rmd.status;
-#ifdef PCNET_DEBUG_RMD_X
-        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
-                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
-                rmd.buf_length, rmd.status, rmd.msg_length);
+        if (PCNET_DEBUG_RMD) {
+            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
+                    "RMDS=0x%04x RMDM=0x%08x\n",
+                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
+                    rmd.buf_length, rmd.status, rmd.msg_length);
+        }
         PRINT_RMD(&rmd);
-#endif
     } else {
         CSR_CRBC(s) = CSR_CRST(s) = 0;
     }
@@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
         (CSR_LOOP(s) && !s->looptest)) {
         return -1;
     }
-#ifdef PCNET_DEBUG
-    printf("pcnet_receive size=%d\n", size);
-#endif
+    if (PCNET_DEBUG) {
+        printf("pcnet_receive size=%d\n", size);
+    }
 
     /* if too small buffer, then expand it */
     if (size < MIN_BUF_SIZE) {
@@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
                     (BCR_SWSTYLE(s) ? 16 : 8 );
                 RMDLOAD(&rmd, nrda);
                 if (GET_FIELD(rmd.status, RMDS, OWN)) {
-#ifdef PCNET_DEBUG_RMD
-                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
+                    if (PCNET_DEBUG_RMD) {
+                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
                                 rcvrc, CSR_RCVRC(s));
-#endif
+                    }
                     CSR_RCVRC(s) = rcvrc;
                     pcnet_rdte_poll(s);
                     break;
@@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
         }
 
         if (!(CSR_CRST(s) & 0x8000)) {
-#ifdef PCNET_DEBUG_RMD
-            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
-#endif
+            if (PCNET_DEBUG_RMD) {
+                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
+            }
             s->csr[0] |= 0x1000; /* Set MISS flag */
             CSR_MISSC(s)++;
         } else {
@@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
 
             if (!s->looptest) {
                 if (size > 4092) {
-#ifdef PCNET_DEBUG_RMD
-                    fprintf(stderr, "pcnet: truncates rx packet.\n");
-#endif
+                    if (PCNET_DEBUG_RMD) {
+                        fprintf(stderr, "pcnet: truncates rx packet.\n");
+                    }
                     size = 4092;
                 }
                 memcpy(src, buf, size);
@@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
                 crc_err = (*(uint32_t *)p != htonl(fcs));
             }
 
-#ifdef PCNET_DEBUG_MATCH
             PRINT_PKTHDR(buf);
-#endif
 
             RMDLOAD(&rmd, PHYSADDR(s,crda));
             /*if (!CSR_LAPPEN(s))*/
@@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
             PCNET_RECV_STORE();
             if ((remaining > 0) && CSR_NRDA(s)) {
                 hwaddr nrda = CSR_NRDA(s);
-#ifdef PCNET_DEBUG_RMD
                 PRINT_RMD(&rmd);
-#endif
                 RMDLOAD(&rmd, PHYSADDR(s,nrda));
                 if (GET_FIELD(rmd.status, RMDS, OWN)) {
                     crda = nrda;
                     PCNET_RECV_STORE();
-#ifdef PCNET_DEBUG_RMD
                     PRINT_RMD(&rmd);
-#endif
                     if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
                         RMDLOAD(&rmd, PHYSADDR(s,nrda));
                         if (GET_FIELD(rmd.status, RMDS, OWN)) {
@@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
             RMDSTORE(&rmd, PHYSADDR(s,crda));
             s->csr[0] |= 0x0400;
 
-#ifdef PCNET_DEBUG
-            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
-                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
-#endif
-#ifdef PCNET_DEBUG_RMD
+            if (PCNET_DEBUG) {
+                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
+                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
+            }
             PRINT_RMD(&rmd);
-#endif
 
             while (pktcount--) {
                 if (CSR_RCVRC(s) <= 1) {
@@ -1217,10 +1218,10 @@ txagain:
 
         TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
 
-#ifdef PCNET_DEBUG_TMD
-        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
-        PRINT_TMD(&tmd);
-#endif
+        if (PCNET_DEBUG_TMD) {
+            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
+            PRINT_TMD(&tmd);
+        }
         if (GET_FIELD(tmd.status, TMDS, STP)) {
             s->xmit_pos = 0;
             xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
@@ -1260,9 +1261,9 @@ txagain:
             goto txdone;
         }
 
-#ifdef PCNET_DEBUG
-        printf("pcnet_transmit size=%d\n", s->xmit_pos);
-#endif
+        if (PCNET_DEBUG) {
+            printf("pcnet_transmit size=%d\n", s->xmit_pos);
+        }
         if (CSR_LOOP(s)) {
             if (BCR_SWSTYLE(s) == 1)
                 add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
@@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
 static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
 {
     uint16_t val = new_value;
-#ifdef PCNET_DEBUG_CSR
-    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_CSR) {
+        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
+    }
     switch (rap) {
     case 0:
         s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
@@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
     default:
         val = s->csr[rap];
     }
-#ifdef PCNET_DEBUG_CSR
-    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_CSR) {
+        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
+    }
     return val;
 }
 
 static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
 {
     rap &= 127;
-#ifdef PCNET_DEBUG_BCR
-    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_BCR) {
+        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
+    }
     switch (rap) {
     case BCR_SWS:
         if (!(CSR_STOP(s) || CSR_SPND(s)))
@@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
             val = 0x0200;
             break;
         }
-#ifdef PCNET_DEBUG
-       printf("BCR_SWS=0x%04x\n", val);
-#endif
+        if (PCNET_DEBUG) {
+            printf("BCR_SWS=0x%04x\n", val);
+        }
         /* fall through */
     case BCR_LNKST:
     case BCR_LED1:
@@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
         val = rap < 32 ? s->bcr[rap] : 0;
         break;
     }
-#ifdef PCNET_DEBUG_BCR
-    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
-#endif
+    if (PCNET_DEBUG_BCR) {
+        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
+    }
     return val;
 }
 
@@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
 {
     PCNetState *s = opaque;
     pcnet_poll_timer(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
+    }
     if (!BCR_DWIO(s)) {
         switch (addr & 0x0f) {
         case 0x00: /* RDP */
@@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
         }
     }
     pcnet_update_irq(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
+                val & 0xffff);
+    }
     return val;
 }
 
@@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
 {
     PCNetState *s = opaque;
     pcnet_poll_timer(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
+    }
     if (BCR_DWIO(s)) {
         switch (addr & 0x0f) {
         case 0x00: /* RDP */
@@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
     } else if ((addr & 0x0f) == 0) {
         /* switch device to dword i/o mode */
         pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
-#ifdef PCNET_DEBUG_IO
-        printf("device switched into dword i/o mode\n");
-#endif
+        if (PCNET_DEBUG_IO) {
+            printf("device switched into dword i/o mode\n");
+        }
     }
     pcnet_update_irq(s);
 }
@@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
         }
     }
     pcnet_update_irq(s);
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
-#endif
+    if (PCNET_DEBUG_IO) {
+        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
+    }
     return val;
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-trivial] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
  2017-06-09 15:06   ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-09 16:04     ` Alex Bennée
  -1 siblings, 0 replies; 17+ messages in thread
From: Alex Bennée @ 2017-06-09 16:04 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-trivial, Jason Wang


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

> Ensure the format strings are always preprocessed, so any problem get reported.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> v2: Addressed review feedback from Alex Bennée
>     No changes in *printf() calls but using qemu_log_mask() is an option.
>
>  hw/net/pcnet-pci.c |  13 ----
>  hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
>  2 files changed, 96 insertions(+), 107 deletions(-)
>
> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
> index 0acf8a4879..331370a1f2 100644
> --- a/hw/net/pcnet-pci.c
> +++ b/hw/net/pcnet-pci.c
> @@ -38,14 +38,6 @@
>
>  #include "pcnet.h"
>
> -//#define PCNET_DEBUG
> -//#define PCNET_DEBUG_IO
> -//#define PCNET_DEBUG_BCR
> -//#define PCNET_DEBUG_CSR
> -//#define PCNET_DEBUG_RMD
> -//#define PCNET_DEBUG_TMD
> -//#define PCNET_DEBUG_MATCH
> -
>  #define TYPE_PCI_PCNET "pcnet"
>
>  #define PCI_PCNET(obj) \
> @@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
>      PCNetState *s = &d->state;
>      uint8_t *pci_conf;
>
> -#if 0
> -    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
> -        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
> -#endif
> -
>      pci_conf = pci_dev->config;
>
>      pci_set_word(pci_conf + PCI_STATUS,
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 654455355f..6e97c0b176 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -45,13 +45,13 @@
>
>  #include "pcnet.h"
>
> -//#define PCNET_DEBUG
> -//#define PCNET_DEBUG_IO
> -//#define PCNET_DEBUG_BCR
> -//#define PCNET_DEBUG_CSR
> -//#define PCNET_DEBUG_RMD
> -//#define PCNET_DEBUG_TMD
> -//#define PCNET_DEBUG_MATCH
> +#define PCNET_DEBUG         0
> +#define PCNET_DEBUG_IO      0
> +#define PCNET_DEBUG_BCR     0
> +#define PCNET_DEBUG_CSR     0
> +#define PCNET_DEBUG_RMD     0
> +#define PCNET_DEBUG_TMD     0
> +#define PCNET_DEBUG_MATCH   0

This is an aweful lot of debug knobs. Is there really such a variation
in debug output to bother having this granularity?

>
>
>  struct qemu_ether_header {
> @@ -268,7 +268,9 @@ struct pcnet_RMD {
>          GET_FIELD((T)->misc, TMDM, TDR),                \
>          GET_FIELD((T)->misc, TMDM, TRC))
>
> -#define PRINT_RMD(R) printf(                            \
> +
> +#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
> +        printf(                                         \
>          "RMD0 : RBADR=0x%08x\n"                         \
>          "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
>          "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
> @@ -292,7 +294,9 @@ struct pcnet_RMD {
>          GET_FIELD((R)->msg_length, RMDM, RCC),          \
>          GET_FIELD((R)->msg_length, RMDM, RPC),          \
>          GET_FIELD((R)->msg_length, RMDM, MCNT),         \
> -        GET_FIELD((R)->msg_length, RMDM, ZEROS))
> +        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
> +        } while (0)
> +
>
>  static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
>                                    hwaddr addr)
> @@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>
>  #endif
>
> -#define PRINT_PKTHDR(BUF) do {                  \
> +#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
>      struct qemu_ether_header *hdr = (void *)(BUF); \
>      printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>             "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
> @@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>             hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
>             hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
>             be16_to_cpu(hdr->ether_type));       \
> -} while (0)
> +    } } while (0)
>
>  #define MULTICAST_FILTER_LEN 8
>
> @@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
>          s->csr[14] & 0xff, s->csr[14] >> 8
>      };
>      int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
> -#ifdef PCNET_DEBUG_MATCH
> -    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
> -           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
> -           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
> -           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
> -           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
> -    printf("padr_match result=%d\n", result);
> -#endif
> +    if (PCNET_DEBUG_MATCH) {
> +        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
> +               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
> +               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
> +               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
> +               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
> +        printf("padr_match result=%d\n", result);
> +    }

While this improves things it might be cleaner to debug helper/defines
so we can have simpler:

   debug_pcnet("foo=%x bar=%d", foo, bar)

Or maybe:

   debug_pci(PCNET_DEBUG_MATCH, "foo=%x, bar=%x", foo, bar)

Depending on if we can rationlise the debug control knobs

>      return result;
>  }
>
> @@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
>      static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>      struct qemu_ether_header *hdr = (void *)buf;
>      int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
> -#ifdef PCNET_DEBUG_MATCH
> -    printf("padr_bcast result=%d\n", result);
> -#endif
> +    if (PCNET_DEBUG_MATCH) {
> +        printf("padr_bcast result=%d\n", result);
> +    }
>      return result;
>  }
>
> @@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
>      uint16_t padr[3], ladrf[4], mode;
>      uint32_t rdra, tdra;
>
> +    if (PCNET_DEBUG) {
> +        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
> +            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
> +    }
>      trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
>
>      if (BCR_SSIZE32(s)) {
> @@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
>
>  static void pcnet_start(PCNetState *s)
>  {
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_start\n");
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_start\n");
> +    }
>
>      if (!CSR_DTX(s)) {
>          s->csr[0] |= 0x0010;    /* set TXON */
> @@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
>
>  static void pcnet_stop(PCNetState *s)
>  {
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_stop\n");
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_stop\n");
> +    }
>      s->csr[0] &= ~0xffeb;
>      s->csr[0] |= 0x0014;
>      s->csr[4] &= ~0x02c2;
> @@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
>          RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
>          CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
>          CSR_CRST(s) = rmd.status;
> -#ifdef PCNET_DEBUG_RMD_X
> -        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
> -                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
> -                rmd.buf_length, rmd.status, rmd.msg_length);
> +        if (PCNET_DEBUG_RMD) {
> +            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
> +                    "RMDS=0x%04x RMDM=0x%08x\n",
> +                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
> +                    rmd.buf_length, rmd.status, rmd.msg_length);
> +        }
>          PRINT_RMD(&rmd);
> -#endif
>      } else {
>          CSR_CRBC(s) = CSR_CRST(s) = 0;
>      }
> @@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>          (CSR_LOOP(s) && !s->looptest)) {
>          return -1;
>      }
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_receive size=%d\n", size);
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_receive size=%d\n", size);
> +    }
>
>      /* if too small buffer, then expand it */
>      if (size < MIN_BUF_SIZE) {
> @@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>                      (BCR_SWSTYLE(s) ? 16 : 8 );
>                  RMDLOAD(&rmd, nrda);
>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
> -#ifdef PCNET_DEBUG_RMD
> -                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
> +                    if (PCNET_DEBUG_RMD) {
> +                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>                                  rcvrc, CSR_RCVRC(s));
> -#endif
> +                    }
>                      CSR_RCVRC(s) = rcvrc;
>                      pcnet_rdte_poll(s);
>                      break;
> @@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>          }
>
>          if (!(CSR_CRST(s) & 0x8000)) {
> -#ifdef PCNET_DEBUG_RMD
> -            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
> -#endif
> +            if (PCNET_DEBUG_RMD) {
> +                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
> +            }
>              s->csr[0] |= 0x1000; /* Set MISS flag */
>              CSR_MISSC(s)++;
>          } else {
> @@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>
>              if (!s->looptest) {
>                  if (size > 4092) {
> -#ifdef PCNET_DEBUG_RMD
> -                    fprintf(stderr, "pcnet: truncates rx packet.\n");
> -#endif
> +                    if (PCNET_DEBUG_RMD) {
> +                        fprintf(stderr, "pcnet: truncates rx packet.\n");
> +                    }
>                      size = 4092;
>                  }
>                  memcpy(src, buf, size);
> @@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>                  crc_err = (*(uint32_t *)p != htonl(fcs));
>              }
>
> -#ifdef PCNET_DEBUG_MATCH
>              PRINT_PKTHDR(buf);
> -#endif
>
>              RMDLOAD(&rmd, PHYSADDR(s,crda));
>              /*if (!CSR_LAPPEN(s))*/
> @@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>              PCNET_RECV_STORE();
>              if ((remaining > 0) && CSR_NRDA(s)) {
>                  hwaddr nrda = CSR_NRDA(s);
> -#ifdef PCNET_DEBUG_RMD
>                  PRINT_RMD(&rmd);
> -#endif
>                  RMDLOAD(&rmd, PHYSADDR(s,nrda));
>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>                      crda = nrda;
>                      PCNET_RECV_STORE();
> -#ifdef PCNET_DEBUG_RMD
>                      PRINT_RMD(&rmd);
> -#endif
>                      if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
>                          RMDLOAD(&rmd, PHYSADDR(s,nrda));
>                          if (GET_FIELD(rmd.status, RMDS, OWN)) {
> @@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>              RMDSTORE(&rmd, PHYSADDR(s,crda));
>              s->csr[0] |= 0x0400;
>
> -#ifdef PCNET_DEBUG
> -            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
> -                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
> -#endif
> -#ifdef PCNET_DEBUG_RMD
> +            if (PCNET_DEBUG) {
> +                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
> +                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
> +            }
>              PRINT_RMD(&rmd);
> -#endif
>
>              while (pktcount--) {
>                  if (CSR_RCVRC(s) <= 1) {
> @@ -1217,10 +1218,10 @@ txagain:
>
>          TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
>
> -#ifdef PCNET_DEBUG_TMD
> -        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
> -        PRINT_TMD(&tmd);
> -#endif
> +        if (PCNET_DEBUG_TMD) {
> +            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
> +            PRINT_TMD(&tmd);
> +        }
>          if (GET_FIELD(tmd.status, TMDS, STP)) {
>              s->xmit_pos = 0;
>              xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
> @@ -1260,9 +1261,9 @@ txagain:
>              goto txdone;
>          }
>
> -#ifdef PCNET_DEBUG
> -        printf("pcnet_transmit size=%d\n", s->xmit_pos);
> -#endif
> +        if (PCNET_DEBUG) {
> +            printf("pcnet_transmit size=%d\n", s->xmit_pos);
> +        }
>          if (CSR_LOOP(s)) {
>              if (BCR_SWSTYLE(s) == 1)
>                  add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
> @@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
>  static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
>  {
>      uint16_t val = new_value;
> -#ifdef PCNET_DEBUG_CSR
> -    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_CSR) {
> +        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
> +    }
>      switch (rap) {
>      case 0:
>          s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
> @@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
>      default:
>          val = s->csr[rap];
>      }
> -#ifdef PCNET_DEBUG_CSR
> -    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_CSR) {
> +        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
> +    }
>      return val;
>  }
>
>  static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>  {
>      rap &= 127;
> -#ifdef PCNET_DEBUG_BCR
> -    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_BCR) {
> +        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
> +    }
>      switch (rap) {
>      case BCR_SWS:
>          if (!(CSR_STOP(s) || CSR_SPND(s)))
> @@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>              val = 0x0200;
>              break;
>          }
> -#ifdef PCNET_DEBUG
> -       printf("BCR_SWS=0x%04x\n", val);
> -#endif
> +        if (PCNET_DEBUG) {
> +            printf("BCR_SWS=0x%04x\n", val);
> +        }
>          /* fall through */
>      case BCR_LNKST:
>      case BCR_LED1:
> @@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
>          val = rap < 32 ? s->bcr[rap] : 0;
>          break;
>      }
> -#ifdef PCNET_DEBUG_BCR
> -    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_BCR) {
> +        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
> +    }
>      return val;
>  }
>
> @@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>  {
>      PCNetState *s = opaque;
>      pcnet_poll_timer(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
> +    }
>      if (!BCR_DWIO(s)) {
>          switch (addr & 0x0f) {
>          case 0x00: /* RDP */
> @@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
>          }
>      }
>      pcnet_update_irq(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
> +                val & 0xffff);
> +    }
>      return val;
>  }
>
> @@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>  {
>      PCNetState *s = opaque;
>      pcnet_poll_timer(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
> +    }
>      if (BCR_DWIO(s)) {
>          switch (addr & 0x0f) {
>          case 0x00: /* RDP */
> @@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>      } else if ((addr & 0x0f) == 0) {
>          /* switch device to dword i/o mode */
>          pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
> -#ifdef PCNET_DEBUG_IO
> -        printf("device switched into dword i/o mode\n");
> -#endif
> +        if (PCNET_DEBUG_IO) {
> +            printf("device switched into dword i/o mode\n");
> +        }
>      }
>      pcnet_update_irq(s);
>  }
> @@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
>          }
>      }
>      pcnet_update_irq(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
> +    }
>      return val;
>  }


--
Alex Bennée


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
@ 2017-06-09 16:04     ` Alex Bennée
  0 siblings, 0 replies; 17+ messages in thread
From: Alex Bennée @ 2017-06-09 16:04 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-trivial, Jason Wang


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

> Ensure the format strings are always preprocessed, so any problem get reported.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> v2: Addressed review feedback from Alex Bennée
>     No changes in *printf() calls but using qemu_log_mask() is an option.
>
>  hw/net/pcnet-pci.c |  13 ----
>  hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
>  2 files changed, 96 insertions(+), 107 deletions(-)
>
> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
> index 0acf8a4879..331370a1f2 100644
> --- a/hw/net/pcnet-pci.c
> +++ b/hw/net/pcnet-pci.c
> @@ -38,14 +38,6 @@
>
>  #include "pcnet.h"
>
> -//#define PCNET_DEBUG
> -//#define PCNET_DEBUG_IO
> -//#define PCNET_DEBUG_BCR
> -//#define PCNET_DEBUG_CSR
> -//#define PCNET_DEBUG_RMD
> -//#define PCNET_DEBUG_TMD
> -//#define PCNET_DEBUG_MATCH
> -
>  #define TYPE_PCI_PCNET "pcnet"
>
>  #define PCI_PCNET(obj) \
> @@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
>      PCNetState *s = &d->state;
>      uint8_t *pci_conf;
>
> -#if 0
> -    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
> -        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
> -#endif
> -
>      pci_conf = pci_dev->config;
>
>      pci_set_word(pci_conf + PCI_STATUS,
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 654455355f..6e97c0b176 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -45,13 +45,13 @@
>
>  #include "pcnet.h"
>
> -//#define PCNET_DEBUG
> -//#define PCNET_DEBUG_IO
> -//#define PCNET_DEBUG_BCR
> -//#define PCNET_DEBUG_CSR
> -//#define PCNET_DEBUG_RMD
> -//#define PCNET_DEBUG_TMD
> -//#define PCNET_DEBUG_MATCH
> +#define PCNET_DEBUG         0
> +#define PCNET_DEBUG_IO      0
> +#define PCNET_DEBUG_BCR     0
> +#define PCNET_DEBUG_CSR     0
> +#define PCNET_DEBUG_RMD     0
> +#define PCNET_DEBUG_TMD     0
> +#define PCNET_DEBUG_MATCH   0

This is an aweful lot of debug knobs. Is there really such a variation
in debug output to bother having this granularity?

>
>
>  struct qemu_ether_header {
> @@ -268,7 +268,9 @@ struct pcnet_RMD {
>          GET_FIELD((T)->misc, TMDM, TDR),                \
>          GET_FIELD((T)->misc, TMDM, TRC))
>
> -#define PRINT_RMD(R) printf(                            \
> +
> +#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
> +        printf(                                         \
>          "RMD0 : RBADR=0x%08x\n"                         \
>          "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
>          "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
> @@ -292,7 +294,9 @@ struct pcnet_RMD {
>          GET_FIELD((R)->msg_length, RMDM, RCC),          \
>          GET_FIELD((R)->msg_length, RMDM, RPC),          \
>          GET_FIELD((R)->msg_length, RMDM, MCNT),         \
> -        GET_FIELD((R)->msg_length, RMDM, ZEROS))
> +        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
> +        } while (0)
> +
>
>  static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
>                                    hwaddr addr)
> @@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>
>  #endif
>
> -#define PRINT_PKTHDR(BUF) do {                  \
> +#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
>      struct qemu_ether_header *hdr = (void *)(BUF); \
>      printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>             "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
> @@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>             hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
>             hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
>             be16_to_cpu(hdr->ether_type));       \
> -} while (0)
> +    } } while (0)
>
>  #define MULTICAST_FILTER_LEN 8
>
> @@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
>          s->csr[14] & 0xff, s->csr[14] >> 8
>      };
>      int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
> -#ifdef PCNET_DEBUG_MATCH
> -    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
> -           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
> -           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
> -           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
> -           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
> -    printf("padr_match result=%d\n", result);
> -#endif
> +    if (PCNET_DEBUG_MATCH) {
> +        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
> +               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
> +               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
> +               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
> +               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
> +        printf("padr_match result=%d\n", result);
> +    }

While this improves things it might be cleaner to debug helper/defines
so we can have simpler:

   debug_pcnet("foo=%x bar=%d", foo, bar)

Or maybe:

   debug_pci(PCNET_DEBUG_MATCH, "foo=%x, bar=%x", foo, bar)

Depending on if we can rationlise the debug control knobs

>      return result;
>  }
>
> @@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
>      static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>      struct qemu_ether_header *hdr = (void *)buf;
>      int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
> -#ifdef PCNET_DEBUG_MATCH
> -    printf("padr_bcast result=%d\n", result);
> -#endif
> +    if (PCNET_DEBUG_MATCH) {
> +        printf("padr_bcast result=%d\n", result);
> +    }
>      return result;
>  }
>
> @@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
>      uint16_t padr[3], ladrf[4], mode;
>      uint32_t rdra, tdra;
>
> +    if (PCNET_DEBUG) {
> +        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
> +            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
> +    }
>      trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
>
>      if (BCR_SSIZE32(s)) {
> @@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
>
>  static void pcnet_start(PCNetState *s)
>  {
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_start\n");
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_start\n");
> +    }
>
>      if (!CSR_DTX(s)) {
>          s->csr[0] |= 0x0010;    /* set TXON */
> @@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
>
>  static void pcnet_stop(PCNetState *s)
>  {
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_stop\n");
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_stop\n");
> +    }
>      s->csr[0] &= ~0xffeb;
>      s->csr[0] |= 0x0014;
>      s->csr[4] &= ~0x02c2;
> @@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
>          RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
>          CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
>          CSR_CRST(s) = rmd.status;
> -#ifdef PCNET_DEBUG_RMD_X
> -        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
> -                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
> -                rmd.buf_length, rmd.status, rmd.msg_length);
> +        if (PCNET_DEBUG_RMD) {
> +            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
> +                    "RMDS=0x%04x RMDM=0x%08x\n",
> +                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
> +                    rmd.buf_length, rmd.status, rmd.msg_length);
> +        }
>          PRINT_RMD(&rmd);
> -#endif
>      } else {
>          CSR_CRBC(s) = CSR_CRST(s) = 0;
>      }
> @@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>          (CSR_LOOP(s) && !s->looptest)) {
>          return -1;
>      }
> -#ifdef PCNET_DEBUG
> -    printf("pcnet_receive size=%d\n", size);
> -#endif
> +    if (PCNET_DEBUG) {
> +        printf("pcnet_receive size=%d\n", size);
> +    }
>
>      /* if too small buffer, then expand it */
>      if (size < MIN_BUF_SIZE) {
> @@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>                      (BCR_SWSTYLE(s) ? 16 : 8 );
>                  RMDLOAD(&rmd, nrda);
>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
> -#ifdef PCNET_DEBUG_RMD
> -                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
> +                    if (PCNET_DEBUG_RMD) {
> +                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>                                  rcvrc, CSR_RCVRC(s));
> -#endif
> +                    }
>                      CSR_RCVRC(s) = rcvrc;
>                      pcnet_rdte_poll(s);
>                      break;
> @@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>          }
>
>          if (!(CSR_CRST(s) & 0x8000)) {
> -#ifdef PCNET_DEBUG_RMD
> -            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
> -#endif
> +            if (PCNET_DEBUG_RMD) {
> +                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
> +            }
>              s->csr[0] |= 0x1000; /* Set MISS flag */
>              CSR_MISSC(s)++;
>          } else {
> @@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>
>              if (!s->looptest) {
>                  if (size > 4092) {
> -#ifdef PCNET_DEBUG_RMD
> -                    fprintf(stderr, "pcnet: truncates rx packet.\n");
> -#endif
> +                    if (PCNET_DEBUG_RMD) {
> +                        fprintf(stderr, "pcnet: truncates rx packet.\n");
> +                    }
>                      size = 4092;
>                  }
>                  memcpy(src, buf, size);
> @@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>                  crc_err = (*(uint32_t *)p != htonl(fcs));
>              }
>
> -#ifdef PCNET_DEBUG_MATCH
>              PRINT_PKTHDR(buf);
> -#endif
>
>              RMDLOAD(&rmd, PHYSADDR(s,crda));
>              /*if (!CSR_LAPPEN(s))*/
> @@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>              PCNET_RECV_STORE();
>              if ((remaining > 0) && CSR_NRDA(s)) {
>                  hwaddr nrda = CSR_NRDA(s);
> -#ifdef PCNET_DEBUG_RMD
>                  PRINT_RMD(&rmd);
> -#endif
>                  RMDLOAD(&rmd, PHYSADDR(s,nrda));
>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>                      crda = nrda;
>                      PCNET_RECV_STORE();
> -#ifdef PCNET_DEBUG_RMD
>                      PRINT_RMD(&rmd);
> -#endif
>                      if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
>                          RMDLOAD(&rmd, PHYSADDR(s,nrda));
>                          if (GET_FIELD(rmd.status, RMDS, OWN)) {
> @@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>              RMDSTORE(&rmd, PHYSADDR(s,crda));
>              s->csr[0] |= 0x0400;
>
> -#ifdef PCNET_DEBUG
> -            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
> -                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
> -#endif
> -#ifdef PCNET_DEBUG_RMD
> +            if (PCNET_DEBUG) {
> +                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
> +                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
> +            }
>              PRINT_RMD(&rmd);
> -#endif
>
>              while (pktcount--) {
>                  if (CSR_RCVRC(s) <= 1) {
> @@ -1217,10 +1218,10 @@ txagain:
>
>          TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
>
> -#ifdef PCNET_DEBUG_TMD
> -        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
> -        PRINT_TMD(&tmd);
> -#endif
> +        if (PCNET_DEBUG_TMD) {
> +            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
> +            PRINT_TMD(&tmd);
> +        }
>          if (GET_FIELD(tmd.status, TMDS, STP)) {
>              s->xmit_pos = 0;
>              xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
> @@ -1260,9 +1261,9 @@ txagain:
>              goto txdone;
>          }
>
> -#ifdef PCNET_DEBUG
> -        printf("pcnet_transmit size=%d\n", s->xmit_pos);
> -#endif
> +        if (PCNET_DEBUG) {
> +            printf("pcnet_transmit size=%d\n", s->xmit_pos);
> +        }
>          if (CSR_LOOP(s)) {
>              if (BCR_SWSTYLE(s) == 1)
>                  add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
> @@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
>  static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
>  {
>      uint16_t val = new_value;
> -#ifdef PCNET_DEBUG_CSR
> -    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_CSR) {
> +        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
> +    }
>      switch (rap) {
>      case 0:
>          s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
> @@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
>      default:
>          val = s->csr[rap];
>      }
> -#ifdef PCNET_DEBUG_CSR
> -    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_CSR) {
> +        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
> +    }
>      return val;
>  }
>
>  static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>  {
>      rap &= 127;
> -#ifdef PCNET_DEBUG_BCR
> -    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_BCR) {
> +        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
> +    }
>      switch (rap) {
>      case BCR_SWS:
>          if (!(CSR_STOP(s) || CSR_SPND(s)))
> @@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>              val = 0x0200;
>              break;
>          }
> -#ifdef PCNET_DEBUG
> -       printf("BCR_SWS=0x%04x\n", val);
> -#endif
> +        if (PCNET_DEBUG) {
> +            printf("BCR_SWS=0x%04x\n", val);
> +        }
>          /* fall through */
>      case BCR_LNKST:
>      case BCR_LED1:
> @@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
>          val = rap < 32 ? s->bcr[rap] : 0;
>          break;
>      }
> -#ifdef PCNET_DEBUG_BCR
> -    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
> -#endif
> +    if (PCNET_DEBUG_BCR) {
> +        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
> +    }
>      return val;
>  }
>
> @@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>  {
>      PCNetState *s = opaque;
>      pcnet_poll_timer(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
> +    }
>      if (!BCR_DWIO(s)) {
>          switch (addr & 0x0f) {
>          case 0x00: /* RDP */
> @@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
>          }
>      }
>      pcnet_update_irq(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
> +                val & 0xffff);
> +    }
>      return val;
>  }
>
> @@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>  {
>      PCNetState *s = opaque;
>      pcnet_poll_timer(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
> +    }
>      if (BCR_DWIO(s)) {
>          switch (addr & 0x0f) {
>          case 0x00: /* RDP */
> @@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>      } else if ((addr & 0x0f) == 0) {
>          /* switch device to dword i/o mode */
>          pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
> -#ifdef PCNET_DEBUG_IO
> -        printf("device switched into dword i/o mode\n");
> -#endif
> +        if (PCNET_DEBUG_IO) {
> +            printf("device switched into dword i/o mode\n");
> +        }
>      }
>      pcnet_update_irq(s);
>  }
> @@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
>          }
>      }
>      pcnet_update_irq(s);
> -#ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
> -#endif
> +    if (PCNET_DEBUG_IO) {
> +        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
> +    }
>      return val;
>  }


--
Alex Bennée

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-trivial] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
  2017-06-09 16:04     ` [Qemu-devel] " Alex Bennée
@ 2017-06-09 21:41       ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 21:41 UTC (permalink / raw)
  To: Alex Bennée; +Cc: qemu-devel, qemu-trivial, Jason Wang

On 06/09/2017 01:04 PM, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> Ensure the format strings are always preprocessed, so any problem get reported.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>
>> v2: Addressed review feedback from Alex Bennée
>>     No changes in *printf() calls but using qemu_log_mask() is an option.
>>
>>  hw/net/pcnet-pci.c |  13 ----
>>  hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
>>  2 files changed, 96 insertions(+), 107 deletions(-)
>>
>> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
>> index 0acf8a4879..331370a1f2 100644
>> --- a/hw/net/pcnet-pci.c
>> +++ b/hw/net/pcnet-pci.c
>> @@ -38,14 +38,6 @@
>>
>>  #include "pcnet.h"
>>
>> -//#define PCNET_DEBUG
>> -//#define PCNET_DEBUG_IO
>> -//#define PCNET_DEBUG_BCR
>> -//#define PCNET_DEBUG_CSR
>> -//#define PCNET_DEBUG_RMD
>> -//#define PCNET_DEBUG_TMD
>> -//#define PCNET_DEBUG_MATCH
>> -
>>  #define TYPE_PCI_PCNET "pcnet"
>>
>>  #define PCI_PCNET(obj) \
>> @@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
>>      PCNetState *s = &d->state;
>>      uint8_t *pci_conf;
>>
>> -#if 0
>> -    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
>> -        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
>> -#endif
>> -
>>      pci_conf = pci_dev->config;
>>
>>      pci_set_word(pci_conf + PCI_STATUS,
>> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
>> index 654455355f..6e97c0b176 100644
>> --- a/hw/net/pcnet.c
>> +++ b/hw/net/pcnet.c
>> @@ -45,13 +45,13 @@
>>
>>  #include "pcnet.h"
>>
>> -//#define PCNET_DEBUG
>> -//#define PCNET_DEBUG_IO
>> -//#define PCNET_DEBUG_BCR
>> -//#define PCNET_DEBUG_CSR
>> -//#define PCNET_DEBUG_RMD
>> -//#define PCNET_DEBUG_TMD
>> -//#define PCNET_DEBUG_MATCH
>> +#define PCNET_DEBUG         0
>> +#define PCNET_DEBUG_IO      0
>> +#define PCNET_DEBUG_BCR     0
>> +#define PCNET_DEBUG_CSR     0
>> +#define PCNET_DEBUG_RMD     0
>> +#define PCNET_DEBUG_TMD     0
>> +#define PCNET_DEBUG_MATCH   0
>
> This is an aweful lot of debug knobs. Is there really such a variation
> in debug output to bother having this granularity?

Don't tell me! I just wanted to fix a format string at first then ended 
here. I believe those knobs were useful during interface model 
bootstrapping and were never used once this interface worked enough.

I'll try using the trace API it should get cleaner I hope.

The only message I ever saw is about "Bad SWSTYLE" in pcnet_bcr_writew() 
with Linux/mipsel which should get reported through 
qemu_log_mask(LOG_GUEST_ERROR).

>
>>
>>
>>  struct qemu_ether_header {
>> @@ -268,7 +268,9 @@ struct pcnet_RMD {
>>          GET_FIELD((T)->misc, TMDM, TDR),                \
>>          GET_FIELD((T)->misc, TMDM, TRC))
>>
>> -#define PRINT_RMD(R) printf(                            \
>> +
>> +#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
>> +        printf(                                         \
>>          "RMD0 : RBADR=0x%08x\n"                         \
>>          "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
>>          "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
>> @@ -292,7 +294,9 @@ struct pcnet_RMD {
>>          GET_FIELD((R)->msg_length, RMDM, RCC),          \
>>          GET_FIELD((R)->msg_length, RMDM, RPC),          \
>>          GET_FIELD((R)->msg_length, RMDM, MCNT),         \
>> -        GET_FIELD((R)->msg_length, RMDM, ZEROS))
>> +        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
>> +        } while (0)
>> +
>>
>>  static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
>>                                    hwaddr addr)
>> @@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>>
>>  #endif
>>
>> -#define PRINT_PKTHDR(BUF) do {                  \
>> +#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
>>      struct qemu_ether_header *hdr = (void *)(BUF); \
>>      printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>>             "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>> @@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>>             hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
>>             hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
>>             be16_to_cpu(hdr->ether_type));       \
>> -} while (0)
>> +    } } while (0)
>>
>>  #define MULTICAST_FILTER_LEN 8
>>
>> @@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
>>          s->csr[14] & 0xff, s->csr[14] >> 8
>>      };
>>      int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
>> -#ifdef PCNET_DEBUG_MATCH
>> -    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
>> -           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
>> -           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
>> -           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
>> -           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
>> -    printf("padr_match result=%d\n", result);
>> -#endif
>> +    if (PCNET_DEBUG_MATCH) {
>> +        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
>> +               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
>> +               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
>> +               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
>> +               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
>> +        printf("padr_match result=%d\n", result);
>> +    }
>
> While this improves things it might be cleaner to debug helper/defines
> so we can have simpler:
>
>    debug_pcnet("foo=%x bar=%d", foo, bar)
>
> Or maybe:
>
>    debug_pci(PCNET_DEBUG_MATCH, "foo=%x, bar=%x", foo, bar)
>
> Depending on if we can rationlise the debug control knobs
>
>>      return result;
>>  }
>>
>> @@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
>>      static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>>      struct qemu_ether_header *hdr = (void *)buf;
>>      int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
>> -#ifdef PCNET_DEBUG_MATCH
>> -    printf("padr_bcast result=%d\n", result);
>> -#endif
>> +    if (PCNET_DEBUG_MATCH) {
>> +        printf("padr_bcast result=%d\n", result);
>> +    }
>>      return result;
>>  }
>>
>> @@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
>>      uint16_t padr[3], ladrf[4], mode;
>>      uint32_t rdra, tdra;
>>
>> +    if (PCNET_DEBUG) {
>> +        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
>> +            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
>> +    }
>>      trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
>>
>>      if (BCR_SSIZE32(s)) {
>> @@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
>>
>>  static void pcnet_start(PCNetState *s)
>>  {
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_start\n");
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_start\n");
>> +    }
>>
>>      if (!CSR_DTX(s)) {
>>          s->csr[0] |= 0x0010;    /* set TXON */
>> @@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
>>
>>  static void pcnet_stop(PCNetState *s)
>>  {
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_stop\n");
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_stop\n");
>> +    }
>>      s->csr[0] &= ~0xffeb;
>>      s->csr[0] |= 0x0014;
>>      s->csr[4] &= ~0x02c2;
>> @@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
>>          RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
>>          CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
>>          CSR_CRST(s) = rmd.status;
>> -#ifdef PCNET_DEBUG_RMD_X
>> -        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
>> -                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
>> -                rmd.buf_length, rmd.status, rmd.msg_length);
>> +        if (PCNET_DEBUG_RMD) {
>> +            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
>> +                    "RMDS=0x%04x RMDM=0x%08x\n",
>> +                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
>> +                    rmd.buf_length, rmd.status, rmd.msg_length);
>> +        }
>>          PRINT_RMD(&rmd);
>> -#endif
>>      } else {
>>          CSR_CRBC(s) = CSR_CRST(s) = 0;
>>      }
>> @@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>          (CSR_LOOP(s) && !s->looptest)) {
>>          return -1;
>>      }
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_receive size=%d\n", size);
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_receive size=%d\n", size);
>> +    }
>>
>>      /* if too small buffer, then expand it */
>>      if (size < MIN_BUF_SIZE) {
>> @@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>                      (BCR_SWSTYLE(s) ? 16 : 8 );
>>                  RMDLOAD(&rmd, nrda);
>>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>> -#ifdef PCNET_DEBUG_RMD
>> -                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>> +                    if (PCNET_DEBUG_RMD) {
>> +                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>>                                  rcvrc, CSR_RCVRC(s));
>> -#endif
>> +                    }
>>                      CSR_RCVRC(s) = rcvrc;
>>                      pcnet_rdte_poll(s);
>>                      break;
>> @@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>          }
>>
>>          if (!(CSR_CRST(s) & 0x8000)) {
>> -#ifdef PCNET_DEBUG_RMD
>> -            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
>> -#endif
>> +            if (PCNET_DEBUG_RMD) {
>> +                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
>> +            }
>>              s->csr[0] |= 0x1000; /* Set MISS flag */
>>              CSR_MISSC(s)++;
>>          } else {
>> @@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>
>>              if (!s->looptest) {
>>                  if (size > 4092) {
>> -#ifdef PCNET_DEBUG_RMD
>> -                    fprintf(stderr, "pcnet: truncates rx packet.\n");
>> -#endif
>> +                    if (PCNET_DEBUG_RMD) {
>> +                        fprintf(stderr, "pcnet: truncates rx packet.\n");
>> +                    }
>>                      size = 4092;
>>                  }
>>                  memcpy(src, buf, size);
>> @@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>                  crc_err = (*(uint32_t *)p != htonl(fcs));
>>              }
>>
>> -#ifdef PCNET_DEBUG_MATCH
>>              PRINT_PKTHDR(buf);
>> -#endif
>>
>>              RMDLOAD(&rmd, PHYSADDR(s,crda));
>>              /*if (!CSR_LAPPEN(s))*/
>> @@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>              PCNET_RECV_STORE();
>>              if ((remaining > 0) && CSR_NRDA(s)) {
>>                  hwaddr nrda = CSR_NRDA(s);
>> -#ifdef PCNET_DEBUG_RMD
>>                  PRINT_RMD(&rmd);
>> -#endif
>>                  RMDLOAD(&rmd, PHYSADDR(s,nrda));
>>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>>                      crda = nrda;
>>                      PCNET_RECV_STORE();
>> -#ifdef PCNET_DEBUG_RMD
>>                      PRINT_RMD(&rmd);
>> -#endif
>>                      if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
>>                          RMDLOAD(&rmd, PHYSADDR(s,nrda));
>>                          if (GET_FIELD(rmd.status, RMDS, OWN)) {
>> @@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>              RMDSTORE(&rmd, PHYSADDR(s,crda));
>>              s->csr[0] |= 0x0400;
>>
>> -#ifdef PCNET_DEBUG
>> -            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
>> -                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
>> -#endif
>> -#ifdef PCNET_DEBUG_RMD
>> +            if (PCNET_DEBUG) {
>> +                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
>> +                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
>> +            }
>>              PRINT_RMD(&rmd);
>> -#endif
>>
>>              while (pktcount--) {
>>                  if (CSR_RCVRC(s) <= 1) {
>> @@ -1217,10 +1218,10 @@ txagain:
>>
>>          TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
>>
>> -#ifdef PCNET_DEBUG_TMD
>> -        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
>> -        PRINT_TMD(&tmd);
>> -#endif
>> +        if (PCNET_DEBUG_TMD) {
>> +            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
>> +            PRINT_TMD(&tmd);
>> +        }
>>          if (GET_FIELD(tmd.status, TMDS, STP)) {
>>              s->xmit_pos = 0;
>>              xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
>> @@ -1260,9 +1261,9 @@ txagain:
>>              goto txdone;
>>          }
>>
>> -#ifdef PCNET_DEBUG
>> -        printf("pcnet_transmit size=%d\n", s->xmit_pos);
>> -#endif
>> +        if (PCNET_DEBUG) {
>> +            printf("pcnet_transmit size=%d\n", s->xmit_pos);
>> +        }
>>          if (CSR_LOOP(s)) {
>>              if (BCR_SWSTYLE(s) == 1)
>>                  add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
>> @@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
>>  static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
>>  {
>>      uint16_t val = new_value;
>> -#ifdef PCNET_DEBUG_CSR
>> -    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_CSR) {
>> +        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      switch (rap) {
>>      case 0:
>>          s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
>> @@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
>>      default:
>>          val = s->csr[rap];
>>      }
>> -#ifdef PCNET_DEBUG_CSR
>> -    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_CSR) {
>> +        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      return val;
>>  }
>>
>>  static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>>  {
>>      rap &= 127;
>> -#ifdef PCNET_DEBUG_BCR
>> -    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_BCR) {
>> +        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      switch (rap) {
>>      case BCR_SWS:
>>          if (!(CSR_STOP(s) || CSR_SPND(s)))
>> @@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>>              val = 0x0200;
>>              break;
>>          }
>> -#ifdef PCNET_DEBUG
>> -       printf("BCR_SWS=0x%04x\n", val);
>> -#endif
>> +        if (PCNET_DEBUG) {
>> +            printf("BCR_SWS=0x%04x\n", val);
>> +        }
>>          /* fall through */
>>      case BCR_LNKST:
>>      case BCR_LED1:
>> @@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
>>          val = rap < 32 ? s->bcr[rap] : 0;
>>          break;
>>      }
>> -#ifdef PCNET_DEBUG_BCR
>> -    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_BCR) {
>> +        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      return val;
>>  }
>>
>> @@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>>  {
>>      PCNetState *s = opaque;
>>      pcnet_poll_timer(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
>> +    }
>>      if (!BCR_DWIO(s)) {
>>          switch (addr & 0x0f) {
>>          case 0x00: /* RDP */
>> @@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
>>          }
>>      }
>>      pcnet_update_irq(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
>> +                val & 0xffff);
>> +    }
>>      return val;
>>  }
>>
>> @@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>>  {
>>      PCNetState *s = opaque;
>>      pcnet_poll_timer(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
>> +    }
>>      if (BCR_DWIO(s)) {
>>          switch (addr & 0x0f) {
>>          case 0x00: /* RDP */
>> @@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>>      } else if ((addr & 0x0f) == 0) {
>>          /* switch device to dword i/o mode */
>>          pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
>> -#ifdef PCNET_DEBUG_IO
>> -        printf("device switched into dword i/o mode\n");
>> -#endif
>> +        if (PCNET_DEBUG_IO) {
>> +            printf("device switched into dword i/o mode\n");
>> +        }
>>      }
>>      pcnet_update_irq(s);
>>  }
>> @@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
>>          }
>>      }
>>      pcnet_update_irq(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
>> +    }
>>      return val;
>>  }
>
>
> --
> Alex Bennée
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] net/pcnet: modernise PCNET_DEBUG
@ 2017-06-09 21:41       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 21:41 UTC (permalink / raw)
  To: Alex Bennée; +Cc: qemu-devel, qemu-trivial, Jason Wang

On 06/09/2017 01:04 PM, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> Ensure the format strings are always preprocessed, so any problem get reported.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>
>> v2: Addressed review feedback from Alex Bennée
>>     No changes in *printf() calls but using qemu_log_mask() is an option.
>>
>>  hw/net/pcnet-pci.c |  13 ----
>>  hw/net/pcnet.c     | 190 +++++++++++++++++++++++++++--------------------------
>>  2 files changed, 96 insertions(+), 107 deletions(-)
>>
>> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
>> index 0acf8a4879..331370a1f2 100644
>> --- a/hw/net/pcnet-pci.c
>> +++ b/hw/net/pcnet-pci.c
>> @@ -38,14 +38,6 @@
>>
>>  #include "pcnet.h"
>>
>> -//#define PCNET_DEBUG
>> -//#define PCNET_DEBUG_IO
>> -//#define PCNET_DEBUG_BCR
>> -//#define PCNET_DEBUG_CSR
>> -//#define PCNET_DEBUG_RMD
>> -//#define PCNET_DEBUG_TMD
>> -//#define PCNET_DEBUG_MATCH
>> -
>>  #define TYPE_PCI_PCNET "pcnet"
>>
>>  #define PCI_PCNET(obj) \
>> @@ -284,11 +276,6 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
>>      PCNetState *s = &d->state;
>>      uint8_t *pci_conf;
>>
>> -#if 0
>> -    printf("sizeof(RMD)=%d, sizeof(TMD)=%d\n",
>> -        sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
>> -#endif
>> -
>>      pci_conf = pci_dev->config;
>>
>>      pci_set_word(pci_conf + PCI_STATUS,
>> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
>> index 654455355f..6e97c0b176 100644
>> --- a/hw/net/pcnet.c
>> +++ b/hw/net/pcnet.c
>> @@ -45,13 +45,13 @@
>>
>>  #include "pcnet.h"
>>
>> -//#define PCNET_DEBUG
>> -//#define PCNET_DEBUG_IO
>> -//#define PCNET_DEBUG_BCR
>> -//#define PCNET_DEBUG_CSR
>> -//#define PCNET_DEBUG_RMD
>> -//#define PCNET_DEBUG_TMD
>> -//#define PCNET_DEBUG_MATCH
>> +#define PCNET_DEBUG         0
>> +#define PCNET_DEBUG_IO      0
>> +#define PCNET_DEBUG_BCR     0
>> +#define PCNET_DEBUG_CSR     0
>> +#define PCNET_DEBUG_RMD     0
>> +#define PCNET_DEBUG_TMD     0
>> +#define PCNET_DEBUG_MATCH   0
>
> This is an aweful lot of debug knobs. Is there really such a variation
> in debug output to bother having this granularity?

Don't tell me! I just wanted to fix a format string at first then ended 
here. I believe those knobs were useful during interface model 
bootstrapping and were never used once this interface worked enough.

I'll try using the trace API it should get cleaner I hope.

The only message I ever saw is about "Bad SWSTYLE" in pcnet_bcr_writew() 
with Linux/mipsel which should get reported through 
qemu_log_mask(LOG_GUEST_ERROR).

>
>>
>>
>>  struct qemu_ether_header {
>> @@ -268,7 +268,9 @@ struct pcnet_RMD {
>>          GET_FIELD((T)->misc, TMDM, TDR),                \
>>          GET_FIELD((T)->misc, TMDM, TRC))
>>
>> -#define PRINT_RMD(R) printf(                            \
>> +
>> +#define PRINT_RMD(R) do { if (PCNET_DEBUG_RMD) {        \
>> +        printf(                                         \
>>          "RMD0 : RBADR=0x%08x\n"                         \
>>          "RMD1 : OWN=%d, ERR=%d, FRAM=%d, OFLO=%d, "     \
>>          "CRC=%d, BUFF=%d, STP=%d, ENP=%d,\n       "     \
>> @@ -292,7 +294,9 @@ struct pcnet_RMD {
>>          GET_FIELD((R)->msg_length, RMDM, RCC),          \
>>          GET_FIELD((R)->msg_length, RMDM, RPC),          \
>>          GET_FIELD((R)->msg_length, RMDM, MCNT),         \
>> -        GET_FIELD((R)->msg_length, RMDM, ZEROS))
>> +        GET_FIELD((R)->msg_length, RMDM, ZEROS)); }     \
>> +        } while (0)
>> +
>>
>>  static inline void pcnet_tmd_load(PCNetState *s, struct pcnet_TMD *tmd,
>>                                    hwaddr addr)
>> @@ -510,7 +514,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>>
>>  #endif
>>
>> -#define PRINT_PKTHDR(BUF) do {                  \
>> +#define PRINT_PKTHDR(BUF) do { if (PCNET_DEBUG_MATCH) { \
>>      struct qemu_ether_header *hdr = (void *)(BUF); \
>>      printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>>             "shost=%02x:%02x:%02x:%02x:%02x:%02x, " \
>> @@ -520,7 +524,7 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
>>             hdr->ether_shost[0],hdr->ether_shost[1],hdr->ether_shost[2], \
>>             hdr->ether_shost[3],hdr->ether_shost[4],hdr->ether_shost[5], \
>>             be16_to_cpu(hdr->ether_type));       \
>> -} while (0)
>> +    } } while (0)
>>
>>  #define MULTICAST_FILTER_LEN 8
>>
>> @@ -623,14 +627,14 @@ static inline int padr_match(PCNetState *s, const uint8_t *buf, int size)
>>          s->csr[14] & 0xff, s->csr[14] >> 8
>>      };
>>      int result = (!CSR_DRCVPA(s)) && !memcmp(hdr->ether_dhost, padr, 6);
>> -#ifdef PCNET_DEBUG_MATCH
>> -    printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
>> -           "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
>> -           hdr->ether_dhost[0],hdr->ether_dhost[1],hdr->ether_dhost[2],
>> -           hdr->ether_dhost[3],hdr->ether_dhost[4],hdr->ether_dhost[5],
>> -           padr[0],padr[1],padr[2],padr[3],padr[4],padr[5]);
>> -    printf("padr_match result=%d\n", result);
>> -#endif
>> +    if (PCNET_DEBUG_MATCH) {
>> +        printf("packet dhost=%02x:%02x:%02x:%02x:%02x:%02x, "
>> +               "padr=%02x:%02x:%02x:%02x:%02x:%02x\n",
>> +               hdr->ether_dhost[0], hdr->ether_dhost[1], hdr->ether_dhost[2],
>> +               hdr->ether_dhost[3], hdr->ether_dhost[4], hdr->ether_dhost[5],
>> +               padr[0], padr[1], padr[2], padr[3], padr[4], padr[5]);
>> +        printf("padr_match result=%d\n", result);
>> +    }
>
> While this improves things it might be cleaner to debug helper/defines
> so we can have simpler:
>
>    debug_pcnet("foo=%x bar=%d", foo, bar)
>
> Or maybe:
>
>    debug_pci(PCNET_DEBUG_MATCH, "foo=%x, bar=%x", foo, bar)
>
> Depending on if we can rationlise the debug control knobs
>
>>      return result;
>>  }
>>
>> @@ -639,9 +643,9 @@ static inline int padr_bcast(PCNetState *s, const uint8_t *buf, int size)
>>      static const uint8_t BCAST[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>>      struct qemu_ether_header *hdr = (void *)buf;
>>      int result = !CSR_DRCVBC(s) && !memcmp(hdr->ether_dhost, BCAST, 6);
>> -#ifdef PCNET_DEBUG_MATCH
>> -    printf("padr_bcast result=%d\n", result);
>> -#endif
>> +    if (PCNET_DEBUG_MATCH) {
>> +        printf("padr_bcast result=%d\n", result);
>> +    }
>>      return result;
>>  }
>>
>> @@ -790,6 +794,10 @@ static void pcnet_init(PCNetState *s)
>>      uint16_t padr[3], ladrf[4], mode;
>>      uint32_t rdra, tdra;
>>
>> +    if (PCNET_DEBUG) {
>> +        printf("sizeof(RMD)=%zu, sizeof(TMD)=%zu\n",
>> +            sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
>> +    }
>>      trace_pcnet_init(s, PHYSADDR(s, CSR_IADR(s)));
>>
>>      if (BCR_SSIZE32(s)) {
>> @@ -858,9 +866,9 @@ static void pcnet_init(PCNetState *s)
>>
>>  static void pcnet_start(PCNetState *s)
>>  {
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_start\n");
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_start\n");
>> +    }
>>
>>      if (!CSR_DTX(s)) {
>>          s->csr[0] |= 0x0010;    /* set TXON */
>> @@ -877,9 +885,9 @@ static void pcnet_start(PCNetState *s)
>>
>>  static void pcnet_stop(PCNetState *s)
>>  {
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_stop\n");
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_stop\n");
>> +    }
>>      s->csr[0] &= ~0xffeb;
>>      s->csr[0] |= 0x0014;
>>      s->csr[4] &= ~0x02c2;
>> @@ -939,12 +947,13 @@ static void pcnet_rdte_poll(PCNetState *s)
>>          RMDLOAD(&rmd, PHYSADDR(s,CSR_CRDA(s)));
>>          CSR_CRBC(s) = GET_FIELD(rmd.buf_length, RMDL, BCNT);
>>          CSR_CRST(s) = rmd.status;
>> -#ifdef PCNET_DEBUG_RMD_X
>> -        printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x RMDS=0x%04x RMDM=0x%08x\n",
>> -                PHYSADDR(s,CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
>> -                rmd.buf_length, rmd.status, rmd.msg_length);
>> +        if (PCNET_DEBUG_RMD) {
>> +            printf("CRDA=0x%08x CRST=0x%04x RCVRC=%d RMDL=0x%04x "
>> +                    "RMDS=0x%04x RMDM=0x%08x\n",
>> +                    PHYSADDR(s, CSR_CRDA(s)), CSR_CRST(s), CSR_RCVRC(s),
>> +                    rmd.buf_length, rmd.status, rmd.msg_length);
>> +        }
>>          PRINT_RMD(&rmd);
>> -#endif
>>      } else {
>>          CSR_CRBC(s) = CSR_CRST(s) = 0;
>>      }
>> @@ -1013,9 +1022,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>          (CSR_LOOP(s) && !s->looptest)) {
>>          return -1;
>>      }
>> -#ifdef PCNET_DEBUG
>> -    printf("pcnet_receive size=%d\n", size);
>> -#endif
>> +    if (PCNET_DEBUG) {
>> +        printf("pcnet_receive size=%d\n", size);
>> +    }
>>
>>      /* if too small buffer, then expand it */
>>      if (size < MIN_BUF_SIZE) {
>> @@ -1044,10 +1053,10 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>                      (BCR_SWSTYLE(s) ? 16 : 8 );
>>                  RMDLOAD(&rmd, nrda);
>>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>> -#ifdef PCNET_DEBUG_RMD
>> -                    printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>> +                    if (PCNET_DEBUG_RMD) {
>> +                        printf("pcnet - scan buffer: RCVRC=%d PREV_RCVRC=%d\n",
>>                                  rcvrc, CSR_RCVRC(s));
>> -#endif
>> +                    }
>>                      CSR_RCVRC(s) = rcvrc;
>>                      pcnet_rdte_poll(s);
>>                      break;
>> @@ -1056,9 +1065,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>          }
>>
>>          if (!(CSR_CRST(s) & 0x8000)) {
>> -#ifdef PCNET_DEBUG_RMD
>> -            printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
>> -#endif
>> +            if (PCNET_DEBUG_RMD) {
>> +                printf("pcnet - no buffer: RCVRC=%d\n", CSR_RCVRC(s));
>> +            }
>>              s->csr[0] |= 0x1000; /* Set MISS flag */
>>              CSR_MISSC(s)++;
>>          } else {
>> @@ -1069,9 +1078,9 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>
>>              if (!s->looptest) {
>>                  if (size > 4092) {
>> -#ifdef PCNET_DEBUG_RMD
>> -                    fprintf(stderr, "pcnet: truncates rx packet.\n");
>> -#endif
>> +                    if (PCNET_DEBUG_RMD) {
>> +                        fprintf(stderr, "pcnet: truncates rx packet.\n");
>> +                    }
>>                      size = 4092;
>>                  }
>>                  memcpy(src, buf, size);
>> @@ -1099,9 +1108,7 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>                  crc_err = (*(uint32_t *)p != htonl(fcs));
>>              }
>>
>> -#ifdef PCNET_DEBUG_MATCH
>>              PRINT_PKTHDR(buf);
>> -#endif
>>
>>              RMDLOAD(&rmd, PHYSADDR(s,crda));
>>              /*if (!CSR_LAPPEN(s))*/
>> @@ -1121,16 +1128,12 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>              PCNET_RECV_STORE();
>>              if ((remaining > 0) && CSR_NRDA(s)) {
>>                  hwaddr nrda = CSR_NRDA(s);
>> -#ifdef PCNET_DEBUG_RMD
>>                  PRINT_RMD(&rmd);
>> -#endif
>>                  RMDLOAD(&rmd, PHYSADDR(s,nrda));
>>                  if (GET_FIELD(rmd.status, RMDS, OWN)) {
>>                      crda = nrda;
>>                      PCNET_RECV_STORE();
>> -#ifdef PCNET_DEBUG_RMD
>>                      PRINT_RMD(&rmd);
>> -#endif
>>                      if ((remaining > 0) && (nrda=CSR_NNRD(s))) {
>>                          RMDLOAD(&rmd, PHYSADDR(s,nrda));
>>                          if (GET_FIELD(rmd.status, RMDS, OWN)) {
>> @@ -1162,13 +1165,11 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
>>              RMDSTORE(&rmd, PHYSADDR(s,crda));
>>              s->csr[0] |= 0x0400;
>>
>> -#ifdef PCNET_DEBUG
>> -            printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
>> -                CSR_RCVRC(s), PHYSADDR(s,CSR_CRDA(s)), pktcount);
>> -#endif
>> -#ifdef PCNET_DEBUG_RMD
>> +            if (PCNET_DEBUG) {
>> +                printf("RCVRC=%d CRDA=0x%08x BLKS=%d\n",
>> +                    CSR_RCVRC(s), PHYSADDR(s, CSR_CRDA(s)), pktcount);
>> +            }
>>              PRINT_RMD(&rmd);
>> -#endif
>>
>>              while (pktcount--) {
>>                  if (CSR_RCVRC(s) <= 1) {
>> @@ -1217,10 +1218,10 @@ txagain:
>>
>>          TMDLOAD(&tmd, PHYSADDR(s,CSR_CXDA(s)));
>>
>> -#ifdef PCNET_DEBUG_TMD
>> -        printf("  TMDLOAD 0x%08x\n", PHYSADDR(s,CSR_CXDA(s)));
>> -        PRINT_TMD(&tmd);
>> -#endif
>> +        if (PCNET_DEBUG_TMD) {
>> +            printf("  TMDLOAD 0x%08x\n", PHYSADDR(s, CSR_CXDA(s)));
>> +            PRINT_TMD(&tmd);
>> +        }
>>          if (GET_FIELD(tmd.status, TMDS, STP)) {
>>              s->xmit_pos = 0;
>>              xmit_cxda = PHYSADDR(s,CSR_CXDA(s));
>> @@ -1260,9 +1261,9 @@ txagain:
>>              goto txdone;
>>          }
>>
>> -#ifdef PCNET_DEBUG
>> -        printf("pcnet_transmit size=%d\n", s->xmit_pos);
>> -#endif
>> +        if (PCNET_DEBUG) {
>> +            printf("pcnet_transmit size=%d\n", s->xmit_pos);
>> +        }
>>          if (CSR_LOOP(s)) {
>>              if (BCR_SWSTYLE(s) == 1)
>>                  add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
>> @@ -1363,9 +1364,9 @@ static void pcnet_poll_timer(void *opaque)
>>  static void pcnet_csr_writew(PCNetState *s, uint32_t rap, uint32_t new_value)
>>  {
>>      uint16_t val = new_value;
>> -#ifdef PCNET_DEBUG_CSR
>> -    printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_CSR) {
>> +        printf("pcnet_csr_writew rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      switch (rap) {
>>      case 0:
>>          s->csr[0] &= ~(val & 0x7f00); /* Clear any interrupt flags */
>> @@ -1491,18 +1492,18 @@ static uint32_t pcnet_csr_readw(PCNetState *s, uint32_t rap)
>>      default:
>>          val = s->csr[rap];
>>      }
>> -#ifdef PCNET_DEBUG_CSR
>> -    printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_CSR) {
>> +        printf("pcnet_csr_readw rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      return val;
>>  }
>>
>>  static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>>  {
>>      rap &= 127;
>> -#ifdef PCNET_DEBUG_BCR
>> -    printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_BCR) {
>> +        printf("pcnet_bcr_writew rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      switch (rap) {
>>      case BCR_SWS:
>>          if (!(CSR_STOP(s) || CSR_SPND(s)))
>> @@ -1524,9 +1525,9 @@ static void pcnet_bcr_writew(PCNetState *s, uint32_t rap, uint32_t val)
>>              val = 0x0200;
>>              break;
>>          }
>> -#ifdef PCNET_DEBUG
>> -       printf("BCR_SWS=0x%04x\n", val);
>> -#endif
>> +        if (PCNET_DEBUG) {
>> +            printf("BCR_SWS=0x%04x\n", val);
>> +        }
>>          /* fall through */
>>      case BCR_LNKST:
>>      case BCR_LED1:
>> @@ -1560,9 +1561,9 @@ uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap)
>>          val = rap < 32 ? s->bcr[rap] : 0;
>>          break;
>>      }
>> -#ifdef PCNET_DEBUG_BCR
>> -    printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
>> -#endif
>> +    if (PCNET_DEBUG_BCR) {
>> +        printf("pcnet_bcr_readw rap=%d val=0x%04x\n", rap, val);
>> +    }
>>      return val;
>>  }
>>
>> @@ -1592,9 +1593,9 @@ void pcnet_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>>  {
>>      PCNetState *s = opaque;
>>      pcnet_poll_timer(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_writew addr=0x%08x val=0x%04x\n", addr, val);
>> +    }
>>      if (!BCR_DWIO(s)) {
>>          switch (addr & 0x0f) {
>>          case 0x00: /* RDP */
>> @@ -1634,9 +1635,10 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr)
>>          }
>>      }
>>      pcnet_update_irq(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr, val & 0xffff);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_readw addr=0x%08x val=0x%04x\n", addr,
>> +                val & 0xffff);
>> +    }
>>      return val;
>>  }
>>
>> @@ -1644,9 +1646,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>>  {
>>      PCNetState *s = opaque;
>>      pcnet_poll_timer(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_writel addr=0x%08x val=0x%08x\n", addr, val);
>> +    }
>>      if (BCR_DWIO(s)) {
>>          switch (addr & 0x0f) {
>>          case 0x00: /* RDP */
>> @@ -1662,9 +1664,9 @@ void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
>>      } else if ((addr & 0x0f) == 0) {
>>          /* switch device to dword i/o mode */
>>          pcnet_bcr_writew(s, BCR_BSBC, pcnet_bcr_readw(s, BCR_BSBC) | 0x0080);
>> -#ifdef PCNET_DEBUG_IO
>> -        printf("device switched into dword i/o mode\n");
>> -#endif
>> +        if (PCNET_DEBUG_IO) {
>> +            printf("device switched into dword i/o mode\n");
>> +        }
>>      }
>>      pcnet_update_irq(s);
>>  }
>> @@ -1692,9 +1694,9 @@ uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
>>          }
>>      }
>>      pcnet_update_irq(s);
>> -#ifdef PCNET_DEBUG_IO
>> -    printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
>> -#endif
>> +    if (PCNET_DEBUG_IO) {
>> +        printf("pcnet_ioport_readl addr=0x%08x val=0x%08x\n", addr, val);
>> +    }
>>      return val;
>>  }
>
>
> --
> Alex Bennée
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/misc: add missing includes
  2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  (?)
@ 2017-06-09 21:32 ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-06-09 21:32 UTC (permalink / raw)
  To: qemu-devel, Eric Blake

I intended to send those 3 patches as separate since unrelated, but 
failed again, sorry :S

On 06/09/2017 12:06 PM, Philippe Mathieu-Daudé wrote:
> inlined create_unimplemented_device() calls sysbus_mmio_map_overlap().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> v2: Addressed review feedback from Eric Blake
>
>  include/hw/misc/unimp.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
> index 3462d85836..52e068ec3e 100644
> --- a/include/hw/misc/unimp.h
> +++ b/include/hw/misc/unimp.h
> @@ -8,6 +8,8 @@
>  #ifndef HW_MISC_UNIMP_H
>  #define HW_MISC_UNIMP_H
>
> +#include "hw/sysbus.h"
> +
>  #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
>
>  /**
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-trivial] [PATCH v2] hw/misc: add missing includes
  2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-09 21:38   ` Eric Blake
  -1 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2017-06-09 21:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Peter Maydell


[-- Attachment #1.1: Type: text/plain, Size: 875 bytes --]

On 06/09/2017 10:06 AM, Philippe Mathieu-Daudé wrote:
> inlined create_unimplemented_device() calls sysbus_mmio_map_overlap().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
> v2: Addressed review feedback from Eric Blake
> 
>  include/hw/misc/unimp.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
> index 3462d85836..52e068ec3e 100644
> --- a/include/hw/misc/unimp.h
> +++ b/include/hw/misc/unimp.h
> @@ -8,6 +8,8 @@
>  #ifndef HW_MISC_UNIMP_H
>  #define HW_MISC_UNIMP_H
>  
> +#include "hw/sysbus.h"
> +
>  #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
>  
>  /**
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/misc: add missing includes
@ 2017-06-09 21:38   ` Eric Blake
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2017-06-09 21:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Peter Maydell

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

On 06/09/2017 10:06 AM, Philippe Mathieu-Daudé wrote:
> inlined create_unimplemented_device() calls sysbus_mmio_map_overlap().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
> v2: Addressed review feedback from Eric Blake
> 
>  include/hw/misc/unimp.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
> index 3462d85836..52e068ec3e 100644
> --- a/include/hw/misc/unimp.h
> +++ b/include/hw/misc/unimp.h
> @@ -8,6 +8,8 @@
>  #ifndef HW_MISC_UNIMP_H
>  #define HW_MISC_UNIMP_H
>  
> +#include "hw/sysbus.h"
> +
>  #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
>  
>  /**
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-trivial] [PATCH v2] hw/misc: add missing includes
  2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
@ 2017-06-27 13:24   ` Michael Tokarev
  -1 siblings, 0 replies; 17+ messages in thread
From: Michael Tokarev @ 2017-06-27 13:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Peter Maydell, Eric Blake

Applied to -trivial, thanks!

/mjt


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/misc: add missing includes
@ 2017-06-27 13:24   ` Michael Tokarev
  0 siblings, 0 replies; 17+ messages in thread
From: Michael Tokarev @ 2017-06-27 13:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, qemu-trivial,
	Peter Maydell, Eric Blake

Applied to -trivial, thanks!

/mjt

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-06-27 13:35 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-09 15:06 [Qemu-trivial] [PATCH v2] hw/misc: add missing includes Philippe Mathieu-Daudé
2017-06-09 15:06 ` [Qemu-devel] " Philippe Mathieu-Daudé
2017-06-09 15:06 ` [Qemu-trivial] [PATCH v2] hw/pci: define msi_nonbroken in pci-stub Philippe Mathieu-Daudé
2017-06-09 15:06   ` [Qemu-devel] " Philippe Mathieu-Daudé
2017-06-27 13:25   ` [Qemu-trivial] " Michael Tokarev
2017-06-27 13:25     ` [Qemu-devel] " Michael Tokarev
2017-06-09 15:06 ` [Qemu-trivial] [PATCH v2] net/pcnet: modernise PCNET_DEBUG Philippe Mathieu-Daudé
2017-06-09 15:06   ` [Qemu-devel] " Philippe Mathieu-Daudé
2017-06-09 16:04   ` [Qemu-trivial] " Alex Bennée
2017-06-09 16:04     ` [Qemu-devel] " Alex Bennée
2017-06-09 21:41     ` [Qemu-trivial] " Philippe Mathieu-Daudé
2017-06-09 21:41       ` [Qemu-devel] " Philippe Mathieu-Daudé
2017-06-09 21:32 ` [Qemu-devel] [PATCH v2] hw/misc: add missing includes Philippe Mathieu-Daudé
2017-06-09 21:38 ` [Qemu-trivial] " Eric Blake
2017-06-09 21:38   ` [Qemu-devel] " Eric Blake
2017-06-27 13:24 ` [Qemu-trivial] " Michael Tokarev
2017-06-27 13:24   ` [Qemu-devel] " Michael Tokarev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.