Patchwork ehci: avoid string arguments in trace events

login
register
mail settings
Submitter Stefan Hajnoczi
Date Sept. 3, 2011, 3:22 p.m.
Message ID <1315063347-26961-1-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/113235/
State New
Headers show

Comments

Stefan Hajnoczi - Sept. 3, 2011, 3:22 p.m.
String arguments are not supported by all trace backends.  This patch
replaces existing string arguments in hw/usb-ehci.c either with
individual trace events that remain human-friendly or by printing raw
addresses when there is no alternative or downside to that.

States and usbsts bits remain human-friendly since it is hard to
remember all of them.  MMIO addresses are printed raw because they would
create many individual trace events and the addresses are usually easy
to remember when debugging.  Queue actions remain human-friendly while
device attach only prints the USBDevice pointer.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/usb-ehci.c |  138 +++++++++++++++++++++++++++-----------------------------
 trace-events  |   40 ++++++++++++++---
 2 files changed, 100 insertions(+), 78 deletions(-)
Gerd Hoffmann - Sept. 5, 2011, 8:38 a.m.
On 09/03/11 17:22, Stefan Hajnoczi wrote:
> String arguments are not supported by all trace backends.  This patch
> replaces existing string arguments in hw/usb-ehci.c either with
> individual trace events that remain human-friendly or by printing raw
> addresses when there is no alternative or downside to that.

Printing raw addresses *is* a downside.

> States and usbsts bits remain human-friendly since it is hard to
> remember all of them.  MMIO addresses are printed raw because they would
> create many individual trace events and the addresses are usually easy
> to remember when debugging.

I find it hard to rememeber them.  There is a reason why the code to 
print the names for the mmio addresses is there in the first place. I 
don't want to loose that.

Can't we just fix the backends instead?  Replacing debug fprintf with 
trace points isn't going to work if tracing can't handle strings.

cheers,
   Gerd
Stefan Hajnoczi - Sept. 5, 2011, 1:08 p.m.
On Mon, Sep 5, 2011 at 9:38 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 09/03/11 17:22, Stefan Hajnoczi wrote:
>>
>> String arguments are not supported by all trace backends.  This patch
>> replaces existing string arguments in hw/usb-ehci.c either with
>> individual trace events that remain human-friendly or by printing raw
>> addresses when there is no alternative or downside to that.
>
> Printing raw addresses *is* a downside.
>
>> States and usbsts bits remain human-friendly since it is hard to
>> remember all of them.  MMIO addresses are printed raw because they would
>> create many individual trace events and the addresses are usually easy
>> to remember when debugging.
>
> I find it hard to rememeber them.  There is a reason why the code to print
> the names for the mmio addresses is there in the first place. I don't want
> to loose that.
>
> Can't we just fix the backends instead?  Replacing debug fprintf with trace
> points isn't going to work if tracing can't handle strings.

I looked at the capabilities of the backends again and I think we
*can* allow strings.  The simple trace backend does not support them
but it's possible to add that.  I thought SystemTap doesn't either but
it turns out there is a userspace string copy function available.
Both stderr and ust are fine with strings.

Let's drop this patch.  I will update the tracing documentation.

Stefan
Gerd Hoffmann - Sept. 5, 2011, 1:15 p.m.
Hi,

> Let's drop this patch.  I will update the tracing documentation.

Great.

thanks,
   Gerd

Patch

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 47a7fb9..e101fc7 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -435,93 +435,89 @@  struct EHCIState {
     *data = val; \
     } while(0)
 
-static const char *ehci_state_names[] = {
-    [ EST_INACTIVE ]     = "INACTIVE",
-    [ EST_ACTIVE ]       = "ACTIVE",
-    [ EST_EXECUTING ]    = "EXECUTING",
-    [ EST_SLEEPING ]     = "SLEEPING",
-    [ EST_WAITLISTHEAD ] = "WAITLISTHEAD",
-    [ EST_FETCHENTRY ]   = "FETCH ENTRY",
-    [ EST_FETCHQH ]      = "FETCH QH",
-    [ EST_FETCHITD ]     = "FETCH ITD",
-    [ EST_ADVANCEQUEUE ] = "ADVANCEQUEUE",
-    [ EST_FETCHQTD ]     = "FETCH QTD",
-    [ EST_EXECUTE ]      = "EXECUTE",
-    [ EST_WRITEBACK ]    = "WRITEBACK",
-    [ EST_HORIZONTALQH ] = "HORIZONTALQH",
-};
-
-static const char *ehci_mmio_names[] = {
-    [ CAPLENGTH ]        = "CAPLENGTH",
-    [ HCIVERSION ]       = "HCIVERSION",
-    [ HCSPARAMS ]        = "HCSPARAMS",
-    [ HCCPARAMS ]        = "HCCPARAMS",
-    [ USBCMD ]           = "USBCMD",
-    [ USBSTS ]           = "USBSTS",
-    [ USBINTR ]          = "USBINTR",
-    [ FRINDEX ]          = "FRINDEX",
-    [ PERIODICLISTBASE ] = "P-LIST BASE",
-    [ ASYNCLISTADDR ]    = "A-LIST ADDR",
-    [ PORTSC_BEGIN ]     = "PORTSC #0",
-    [ PORTSC_BEGIN + 4]  = "PORTSC #1",
-    [ PORTSC_BEGIN + 8]  = "PORTSC #2",
-    [ PORTSC_BEGIN + 12] = "PORTSC #3",
-    [ CONFIGFLAG ]       = "CONFIGFLAG",
-};
-
-static const char *nr2str(const char **n, size_t len, uint32_t nr)
+/* Individual human-friendly trace events for states */
+static void ehci_trace_set_state(int async, uint32_t state)
 {
-    if (nr < len && n[nr] != NULL) {
-        return n[nr];
-    } else {
-        return "unknown";
+    switch (state) {
+    case EST_INACTIVE:
+        trace_usb_ehci_state_inactive(async);
+        break;
+    case EST_ACTIVE:
+        trace_usb_ehci_state_active(async);
+        break;
+    case EST_EXECUTING:
+        trace_usb_ehci_state_executing(async);
+        break;
+    case EST_SLEEPING:
+        trace_usb_ehci_state_sleeping(async);
+        break;
+    case EST_WAITLISTHEAD:
+        trace_usb_ehci_state_waitlisthead(async);
+        break;
+    case EST_FETCHENTRY:
+        trace_usb_ehci_state_fetchentry(async);
+        break;
+    case EST_FETCHQH:
+        trace_usb_ehci_state_fetchqh(async);
+        break;
+    case EST_FETCHITD:
+        trace_usb_ehci_state_fetchitd(async);
+        break;
+    case EST_ADVANCEQUEUE:
+        trace_usb_ehci_state_advancequeue(async);
+        break;
+    case EST_FETCHQTD:
+        trace_usb_ehci_state_fetchqtd(async);
+        break;
+    case EST_EXECUTE:
+        trace_usb_ehci_state_execute(async);
+        break;
+    case EST_WRITEBACK:
+        trace_usb_ehci_state_writeback(async);
+        break;
+    case EST_HORIZONTALQH:
+        trace_usb_ehci_state_horizontalqh(async);
+        break;
+    default:
+        trace_usb_ehci_state_unknown(async, state);
+        break;
     }
 }
 
-static const char *state2str(uint32_t state)
-{
-    return nr2str(ehci_state_names, ARRAY_SIZE(ehci_state_names), state);
-}
-
-static const char *addr2str(target_phys_addr_t addr)
-{
-    return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names), addr);
-}
-
 static void ehci_trace_usbsts(uint32_t mask, int state)
 {
     /* interrupts */
     if (mask & USBSTS_INT) {
-        trace_usb_ehci_usbsts("INT", state);
+        trace_usb_ehci_usbsts_int(state);
     }
     if (mask & USBSTS_ERRINT) {
-        trace_usb_ehci_usbsts("ERRINT", state);
+        trace_usb_ehci_usbsts_errint(state);
     }
     if (mask & USBSTS_PCD) {
-        trace_usb_ehci_usbsts("PCD", state);
+        trace_usb_ehci_usbsts_pcd(state);
     }
     if (mask & USBSTS_FLR) {
-        trace_usb_ehci_usbsts("FLR", state);
+        trace_usb_ehci_usbsts_flr(state);
     }
     if (mask & USBSTS_HSE) {
-        trace_usb_ehci_usbsts("HSE", state);
+        trace_usb_ehci_usbsts_hse(state);
     }
     if (mask & USBSTS_IAA) {
-        trace_usb_ehci_usbsts("IAA", state);
+        trace_usb_ehci_usbsts_iaa(state);
     }
 
     /* status */
     if (mask & USBSTS_HALT) {
-        trace_usb_ehci_usbsts("HALT", state);
+        trace_usb_ehci_usbsts_halt(state);
     }
     if (mask & USBSTS_REC) {
-        trace_usb_ehci_usbsts("REC", state);
+        trace_usb_ehci_usbsts_rec(state);
     }
     if (mask & USBSTS_PSS) {
-        trace_usb_ehci_usbsts("PSS", state);
+        trace_usb_ehci_usbsts_pss(state);
     }
     if (mask & USBSTS_ASS) {
-        trace_usb_ehci_usbsts("ASS", state);
+        trace_usb_ehci_usbsts_ass(state);
     }
 }
 
@@ -574,11 +570,11 @@  static inline void ehci_commit_interrupt(EHCIState *s)
 
 static void ehci_set_state(EHCIState *s, int async, int state)
 {
+    ehci_trace_set_state(async, state);
+
     if (async) {
-        trace_usb_ehci_state("async", state2str(state));
         s->astate = state;
     } else {
-        trace_usb_ehci_state("periodic", state2str(state));
         s->pstate = state;
     }
 }
@@ -656,13 +652,13 @@  static EHCIQueue *ehci_alloc_queue(EHCIState *ehci, int async)
     q->ehci = ehci;
     q->async_schedule = async;
     QTAILQ_INSERT_HEAD(&ehci->queues, q, next);
-    trace_usb_ehci_queue_action(q, "alloc");
+    trace_usb_ehci_alloc_queue(q);
     return q;
 }
 
 static void ehci_free_queue(EHCIQueue *q)
 {
-    trace_usb_ehci_queue_action(q, "free");
+    trace_usb_ehci_free_queue(q);
     if (q->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&q->packet);
     }
@@ -728,7 +724,7 @@  static void ehci_attach(USBPort *port)
     EHCIState *s = port->opaque;
     uint32_t *portsc = &s->portsc[port->index];
 
-    trace_usb_ehci_port_attach(port->index, port->dev->product_desc);
+    trace_usb_ehci_port_attach(port->index, port->dev);
 
     if (*portsc & PORTSC_POWNER) {
         USBPort *companion = s->companion_ports[port->index];
@@ -905,7 +901,7 @@  static uint32_t ehci_mem_readl(void *ptr, target_phys_addr_t addr)
     val = s->mmio[addr] | (s->mmio[addr+1] << 8) |
           (s->mmio[addr+2] << 16) | (s->mmio[addr+3] << 24);
 
-    trace_usb_ehci_mmio_readl(addr, addr2str(addr), val);
+    trace_usb_ehci_mmio_readl(addr, val);
     return val;
 }
 
@@ -995,7 +991,7 @@  static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
     uint32_t old = *mmio;
     int i;
 
-    trace_usb_ehci_mmio_writel(addr, addr2str(addr), val);
+    trace_usb_ehci_mmio_writel(addr, val);
 
     /* Only aligned reads are allowed on OHCI */
     if (addr & 3) {
@@ -1006,7 +1002,7 @@  static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
 
     if (addr >= PORTSC && addr < PORTSC + 4 * NB_PORTS) {
         handle_port_status_write(s, (addr-PORTSC)/4, val);
-        trace_usb_ehci_mmio_change(addr, addr2str(addr), *mmio, old);
+        trace_usb_ehci_mmio_change(addr, *mmio, old);
         return;
     }
 
@@ -1086,7 +1082,7 @@  static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
     }
 
     *mmio = val;
-    trace_usb_ehci_mmio_change(addr, addr2str(addr), *mmio, old);
+    trace_usb_ehci_mmio_change(addr, *mmio, old);
 }
 
 
@@ -1231,7 +1227,7 @@  static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
     }
 
     q = container_of(packet, EHCIQueue, packet);
-    trace_usb_ehci_queue_action(q, "wakeup");
+    trace_usb_ehci_wakeup_queue(q);
     assert(q->async == EHCI_ASYNC_INFLIGHT);
     q->async = EHCI_ASYNC_FINISHED;
     q->usb_status = packet->result;
@@ -1626,7 +1622,7 @@  static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     }
     if (q->async == EHCI_ASYNC_FINISHED) {
         /* I/O finished -- continue processing queue */
-        trace_usb_ehci_queue_action(q, "resume");
+        trace_usb_ehci_resume_queue(q);
         ehci_set_state(ehci, async, EST_EXECUTING);
         goto out;
     }
@@ -1839,7 +1835,7 @@  static int ehci_state_execute(EHCIQueue *q, int async)
     }
     if (q->usb_status == USB_RET_ASYNC) {
         ehci_flush_qh(q);
-        trace_usb_ehci_queue_action(q, "suspend");
+        trace_usb_ehci_suspend_queue(q);
         q->async = EHCI_ASYNC_INFLIGHT;
         ehci_set_state(q->ehci, async, EST_HORIZONTALQH);
         again = 1;
diff --git a/trace-events b/trace-events
index 08ffedf..120d731 100644
--- a/trace-events
+++ b/trace-events
@@ -211,11 +211,33 @@  sun4m_iommu_bad_addr(uint64_t addr) "bad addr %"PRIx64""
 
 # hw/usb-ehci.c
 usb_ehci_reset(void) "=== RESET ==="
-usb_ehci_mmio_readl(uint32_t addr, const char *str, uint32_t val) "rd mmio %04x [%s] = %x"
-usb_ehci_mmio_writel(uint32_t addr, const char *str, uint32_t val) "wr mmio %04x [%s] = %x"
-usb_ehci_mmio_change(uint32_t addr, const char *str, uint32_t new, uint32_t old) "ch mmio %04x [%s] = %x (old: %x)"
-usb_ehci_usbsts(const char *sts, int state) "usbsts %s %d"
-usb_ehci_state(const char *schedule, const char *state) "%s schedule %s"
+usb_ehci_mmio_readl(uint32_t addr, uint32_t val) "rd mmio %04x = %x"
+usb_ehci_mmio_writel(uint32_t addr, uint32_t val) "wr mmio %04x = %x"
+usb_ehci_mmio_change(uint32_t addr, uint32_t new, uint32_t old) "ch mmio %04x = %x (old: %x)"
+usb_ehci_usbsts_int(int state) "usbsts INT %d"
+usb_ehci_usbsts_errint(int state) "usbsts ERRINT %d"
+usb_ehci_usbsts_pcd(int state) "usbsts PCD %d"
+usb_ehci_usbsts_flr(int state) "usbsts FLR %d"
+usb_ehci_usbsts_hse(int state) "usbsts HSE %d"
+usb_ehci_usbsts_iaa(int state) "usbsts IAA %d"
+usb_ehci_usbsts_halt(int state) "usbsts HALT %d"
+usb_ehci_usbsts_rec(int state) "usbsts REC %d"
+usb_ehci_usbsts_pss(int state) "usbsts PSS %d"
+usb_ehci_usbsts_ass(int state) "usbsts ASS %d"
+usb_ehci_state_inactive(bool async) "schedule async %d INACTIVE"
+usb_ehci_state_active(bool async) "schedule async %d ACTIVE"
+usb_ehci_state_executing(bool async) "schedule async %d EXECUTING"
+usb_ehci_state_sleeping(bool async) "schedule async %d SLEEPING"
+usb_ehci_state_waitlisthead(bool async) "schedule async %d WAITLISTHEAD"
+usb_ehci_state_fetchentry(bool async) "schedule async %d FETCH ENTRY"
+usb_ehci_state_fetchqh(bool async) "schedule async %d FETCH QH"
+usb_ehci_state_fetchitd(bool async) "schedule async %d FETCH ITD"
+usb_ehci_state_advancequeue(bool async) "schedule async %d ADVANCE QUEUE"
+usb_ehci_state_fetchqtd(bool async) "schedule async %d FETCH QTD"
+usb_ehci_state_execute(bool async) "schedule async %d EXECUTE"
+usb_ehci_state_writeback(bool async) "schedule async %d WRITEBACK"
+usb_ehci_state_horizontalqh(bool async) "schedule async %d HORIZONTAL QH"
+usb_ehci_state_unknown(bool async, uint32_t state) "schedule async %d state %08x"
 usb_ehci_qh_ptrs(void *q, uint32_t addr, uint32_t nxt, uint32_t c_qtd, uint32_t n_qtd, uint32_t a_qtd) "q %p - QH @ %08x: next %08x qtds %08x,%08x,%08x"
 usb_ehci_qh_fields(uint32_t addr, int rl, int mplen, int eps, int ep, int devaddr) "QH @ %08x - rl %d, mplen %d, eps %d, ep %d, dev %d"
 usb_ehci_qh_bits(uint32_t addr, int c, int h, int dtc, int i) "QH @ %08x - c %d, h %d, dtc %d, i %d"
@@ -223,11 +245,15 @@  usb_ehci_qtd_ptrs(void *q, uint32_t addr, uint32_t nxt, uint32_t altnext) "q %p
 usb_ehci_qtd_fields(uint32_t addr, int tbytes, int cpage, int cerr, int pid) "QTD @ %08x - tbytes %d, cpage %d, cerr %d, pid %d"
 usb_ehci_qtd_bits(uint32_t addr, int ioc, int active, int halt, int babble, int xacterr) "QTD @ %08x - ioc %d, active %d, halt %d, babble %d, xacterr %d"
 usb_ehci_itd(uint32_t addr, uint32_t nxt, uint32_t mplen, uint32_t mult, uint32_t ep, uint32_t devaddr) "ITD @ %08x: next %08x - mplen %d, mult %d, ep %d, dev %d"
-usb_ehci_port_attach(uint32_t port, const char *device) "attach port #%d - %s"
+usb_ehci_port_attach(uint32_t port, void *dev) "attach port #%d - %p"
 usb_ehci_port_detach(uint32_t port) "detach port #%d"
 usb_ehci_port_reset(uint32_t port, int enable) "reset port #%d - %d"
 usb_ehci_data(int rw, uint32_t cpage, uint32_t offset, uint32_t addr, uint32_t len, uint32_t bufpos) "write %d, cpage %d, offset 0x%03x, addr 0x%08x, len %d, bufpos %d"
-usb_ehci_queue_action(void *q, const char *action) "q %p: %s"
+usb_ehci_alloc_queue(void *q) "q %p"
+usb_ehci_free_queue(void *q) "q %p"
+usb_ehci_wakeup_queue(void *q) "q %p"
+usb_ehci_resume_queue(void *q) "q %p"
+usb_ehci_suspend_queue(void *q) "q %p"
 
 # hw/usb-desc.c
 usb_desc_device(int addr, int len, int ret) "dev %d query device, len %d, ret %d"