| Message ID | 20200614142840.10245-23-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | ADB: fix autopoll issues and rework mac_via state machine | expand |
Hi Mark, On 6/14/20 4:28 PM, Mark Cave-Ayland wrote: > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/input/adb.c | 23 ++++++++++++++++++++++- > hw/input/trace-events | 7 +++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/hw/input/adb.c b/hw/input/adb.c > index fe0f6c7ef3..4976f52c36 100644 > --- a/hw/input/adb.c > +++ b/hw/input/adb.c > @@ -29,10 +29,18 @@ > #include "qemu/module.h" > #include "qemu/timer.h" > #include "adb-internal.h" > +#include "trace.h" > > /* error codes */ > #define ADB_RET_NOTPRESENT (-2) > > +static const char *adb_commands[] = { > + "RESET", "FLUSH", "(Reserved 0x2)", "(Reserved 0x3)", > + "Reserved (0x4)", "(Reserved 0x5)", "(Reserved 0x6)", "(Reserved 0x7)", > + "LISTEN r0", "LISTEN r1", "LISTEN r2", "LISTEN r3", > + "TALK r0", "TALK r1", "TALK r2", "TALK r3", > +}; > + > static void adb_device_reset(ADBDevice *d) > { > qdev_reset_all(DEVICE(d)); > @@ -86,9 +94,16 @@ static int do_adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, > > int adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int len) > { > + int ret; > + > + trace_adb_bus_request(buf[0] >> 4, adb_commands[buf[0] & 0xf], len); > + > assert(s->autopoll_blocked); > > - return do_adb_request(s, obuf, buf, len); > + ret = do_adb_request(s, obuf, buf, len); > + > + trace_adb_bus_request_done(buf[0] >> 4, adb_commands[buf[0] & 0xf], ret); > + return ret; > } > > int adb_poll(ADBBusState *s, uint8_t *obuf, uint16_t poll_mask) > @@ -160,6 +175,8 @@ void adb_set_autopoll_mask(ADBBusState *s, uint16_t mask) > > void adb_autopoll_block(ADBBusState *s) > { > + trace_adb_bus_autopoll_block("autopoll BLOCKED"); Regarding how trace backends work, in this case it is better to use a boolean value and let the backend do the formatting: trace_adb_bus_autopoll_block(true); The rationale is it is easier for backends to filter on a bool (register) arg rather than fetching memory for strcmp. So format can be: adb_bus_autopoll_block(bool state) "autopoll is_blocked:%u" Anyway if you want to keep as it, it is cleaner to change the format as "autopoll %s". > + > s->autopoll_blocked = true; This can also be: trace_adb_bus_autopoll_block(s->autopoll_blocked); > > if (s->autopoll_enabled) { > @@ -169,6 +186,8 @@ void adb_autopoll_block(ADBBusState *s) > > void adb_autopoll_unblock(ADBBusState *s) > { > + trace_adb_bus_autopoll_block("autopoll UNBLOCKED"); > + > s->autopoll_blocked = false; Ditto: trace_adb_bus_autopoll_block(s->autopoll_blocked); > > if (s->autopoll_enabled) { > @@ -183,7 +202,9 @@ static void adb_autopoll(void *opaque) > ADBBusState *s = opaque; > > if (!s->autopoll_blocked) { > + trace_adb_bus_autopoll_cb(s->autopoll_mask); > s->autopoll_cb(s->autopoll_cb_opaque); > + trace_adb_bus_autopoll_cb_done(s->autopoll_mask); > } > > timer_mod(s->autopoll_timer, > diff --git a/hw/input/trace-events b/hw/input/trace-events > index 6f0d78241c..119d1ce2bd 100644 > --- a/hw/input/trace-events > +++ b/hw/input/trace-events > @@ -14,6 +14,13 @@ adb_device_mouse_readreg(int reg, uint8_t val0, uint8_t val1) "reg %d obuf[0] 0x > adb_device_mouse_request_change_addr(int devaddr) "change addr to 0x%x" > adb_device_mouse_request_change_addr_and_handler(int devaddr, int handler) "change addr and handler to 0x%x, 0x%x" > > +# adb.c > +adb_bus_request(uint8_t addr, const char *cmd, int size) "device 0x%x %s cmdsize=%d" > +adb_bus_request_done(uint8_t addr, const char *cmd, int size) "device 0x%x %s replysize=%d" > +adb_bus_autopoll_block(const char *s) "%s" > +adb_bus_autopoll_cb(uint16_t mask) "executing autopoll_cb with autopoll mask 0x%x" > +adb_bus_autopoll_cb_done(uint16_t mask) "done executing autopoll_cb with autopoll mask 0x%x" > + > # pckbd.c > pckbd_kbd_read_data(uint32_t val) "0x%02x" > pckbd_kbd_read_status(int status) "0x%02x" > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 14/06/2020 18:16, Philippe Mathieu-Daudé wrote: > Hi Mark, > > On 6/14/20 4:28 PM, Mark Cave-Ayland wrote: >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/input/adb.c | 23 ++++++++++++++++++++++- >> hw/input/trace-events | 7 +++++++ >> 2 files changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/hw/input/adb.c b/hw/input/adb.c >> index fe0f6c7ef3..4976f52c36 100644 >> --- a/hw/input/adb.c >> +++ b/hw/input/adb.c >> @@ -29,10 +29,18 @@ >> #include "qemu/module.h" >> #include "qemu/timer.h" >> #include "adb-internal.h" >> +#include "trace.h" >> >> /* error codes */ >> #define ADB_RET_NOTPRESENT (-2) >> >> +static const char *adb_commands[] = { >> + "RESET", "FLUSH", "(Reserved 0x2)", "(Reserved 0x3)", >> + "Reserved (0x4)", "(Reserved 0x5)", "(Reserved 0x6)", "(Reserved 0x7)", >> + "LISTEN r0", "LISTEN r1", "LISTEN r2", "LISTEN r3", >> + "TALK r0", "TALK r1", "TALK r2", "TALK r3", >> +}; >> + >> static void adb_device_reset(ADBDevice *d) >> { >> qdev_reset_all(DEVICE(d)); >> @@ -86,9 +94,16 @@ static int do_adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, >> >> int adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int len) >> { >> + int ret; >> + >> + trace_adb_bus_request(buf[0] >> 4, adb_commands[buf[0] & 0xf], len); >> + >> assert(s->autopoll_blocked); >> >> - return do_adb_request(s, obuf, buf, len); >> + ret = do_adb_request(s, obuf, buf, len); >> + >> + trace_adb_bus_request_done(buf[0] >> 4, adb_commands[buf[0] & 0xf], ret); >> + return ret; >> } >> >> int adb_poll(ADBBusState *s, uint8_t *obuf, uint16_t poll_mask) >> @@ -160,6 +175,8 @@ void adb_set_autopoll_mask(ADBBusState *s, uint16_t mask) >> >> void adb_autopoll_block(ADBBusState *s) >> { >> + trace_adb_bus_autopoll_block("autopoll BLOCKED"); > > Regarding how trace backends work, in this case it is better > to use a boolean value and let the backend do the formatting: > > trace_adb_bus_autopoll_block(true); > > The rationale is it is easier for backends to filter on a > bool (register) arg rather than fetching memory for strcmp. > > So format can be: > > adb_bus_autopoll_block(bool state) "autopoll is_blocked:%u" > > Anyway if you want to keep as it, it is cleaner to change the > format as "autopoll %s". > >> + >> s->autopoll_blocked = true; > > This can also be: > > trace_adb_bus_autopoll_block(s->autopoll_blocked); > >> >> if (s->autopoll_enabled) { >> @@ -169,6 +186,8 @@ void adb_autopoll_block(ADBBusState *s) >> >> void adb_autopoll_unblock(ADBBusState *s) >> { >> + trace_adb_bus_autopoll_block("autopoll UNBLOCKED"); >> + >> s->autopoll_blocked = false; > > Ditto: > > trace_adb_bus_autopoll_block(s->autopoll_blocked); > >> >> if (s->autopoll_enabled) { >> @@ -183,7 +202,9 @@ static void adb_autopoll(void *opaque) >> ADBBusState *s = opaque; >> >> if (!s->autopoll_blocked) { >> + trace_adb_bus_autopoll_cb(s->autopoll_mask); >> s->autopoll_cb(s->autopoll_cb_opaque); >> + trace_adb_bus_autopoll_cb_done(s->autopoll_mask); >> } >> >> timer_mod(s->autopoll_timer, >> diff --git a/hw/input/trace-events b/hw/input/trace-events >> index 6f0d78241c..119d1ce2bd 100644 >> --- a/hw/input/trace-events >> +++ b/hw/input/trace-events >> @@ -14,6 +14,13 @@ adb_device_mouse_readreg(int reg, uint8_t val0, uint8_t val1) "reg %d obuf[0] 0x >> adb_device_mouse_request_change_addr(int devaddr) "change addr to 0x%x" >> adb_device_mouse_request_change_addr_and_handler(int devaddr, int handler) "change addr and handler to 0x%x, 0x%x" >> >> +# adb.c >> +adb_bus_request(uint8_t addr, const char *cmd, int size) "device 0x%x %s cmdsize=%d" >> +adb_bus_request_done(uint8_t addr, const char *cmd, int size) "device 0x%x %s replysize=%d" >> +adb_bus_autopoll_block(const char *s) "%s" >> +adb_bus_autopoll_cb(uint16_t mask) "executing autopoll_cb with autopoll mask 0x%x" >> +adb_bus_autopoll_cb_done(uint16_t mask) "done executing autopoll_cb with autopoll mask 0x%x" >> + >> # pckbd.c >> pckbd_kbd_read_data(uint32_t val) "0x%02x" >> pckbd_kbd_read_status(int status) "0x%02x" >> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Thanks for the review: I think you're right in that the boolean makes things easier for other trace backends, so I'll implement your changes in a v2. ATB, Mark.
diff --git a/hw/input/adb.c b/hw/input/adb.c index fe0f6c7ef3..4976f52c36 100644 --- a/hw/input/adb.c +++ b/hw/input/adb.c @@ -29,10 +29,18 @@ #include "qemu/module.h" #include "qemu/timer.h" #include "adb-internal.h" +#include "trace.h" /* error codes */ #define ADB_RET_NOTPRESENT (-2) +static const char *adb_commands[] = { + "RESET", "FLUSH", "(Reserved 0x2)", "(Reserved 0x3)", + "Reserved (0x4)", "(Reserved 0x5)", "(Reserved 0x6)", "(Reserved 0x7)", + "LISTEN r0", "LISTEN r1", "LISTEN r2", "LISTEN r3", + "TALK r0", "TALK r1", "TALK r2", "TALK r3", +}; + static void adb_device_reset(ADBDevice *d) { qdev_reset_all(DEVICE(d)); @@ -86,9 +94,16 @@ static int do_adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int len) { + int ret; + + trace_adb_bus_request(buf[0] >> 4, adb_commands[buf[0] & 0xf], len); + assert(s->autopoll_blocked); - return do_adb_request(s, obuf, buf, len); + ret = do_adb_request(s, obuf, buf, len); + + trace_adb_bus_request_done(buf[0] >> 4, adb_commands[buf[0] & 0xf], ret); + return ret; } int adb_poll(ADBBusState *s, uint8_t *obuf, uint16_t poll_mask) @@ -160,6 +175,8 @@ void adb_set_autopoll_mask(ADBBusState *s, uint16_t mask) void adb_autopoll_block(ADBBusState *s) { + trace_adb_bus_autopoll_block("autopoll BLOCKED"); + s->autopoll_blocked = true; if (s->autopoll_enabled) { @@ -169,6 +186,8 @@ void adb_autopoll_block(ADBBusState *s) void adb_autopoll_unblock(ADBBusState *s) { + trace_adb_bus_autopoll_block("autopoll UNBLOCKED"); + s->autopoll_blocked = false; if (s->autopoll_enabled) { @@ -183,7 +202,9 @@ static void adb_autopoll(void *opaque) ADBBusState *s = opaque; if (!s->autopoll_blocked) { + trace_adb_bus_autopoll_cb(s->autopoll_mask); s->autopoll_cb(s->autopoll_cb_opaque); + trace_adb_bus_autopoll_cb_done(s->autopoll_mask); } timer_mod(s->autopoll_timer, diff --git a/hw/input/trace-events b/hw/input/trace-events index 6f0d78241c..119d1ce2bd 100644 --- a/hw/input/trace-events +++ b/hw/input/trace-events @@ -14,6 +14,13 @@ adb_device_mouse_readreg(int reg, uint8_t val0, uint8_t val1) "reg %d obuf[0] 0x adb_device_mouse_request_change_addr(int devaddr) "change addr to 0x%x" adb_device_mouse_request_change_addr_and_handler(int devaddr, int handler) "change addr and handler to 0x%x, 0x%x" +# adb.c +adb_bus_request(uint8_t addr, const char *cmd, int size) "device 0x%x %s cmdsize=%d" +adb_bus_request_done(uint8_t addr, const char *cmd, int size) "device 0x%x %s replysize=%d" +adb_bus_autopoll_block(const char *s) "%s" +adb_bus_autopoll_cb(uint16_t mask) "executing autopoll_cb with autopoll mask 0x%x" +adb_bus_autopoll_cb_done(uint16_t mask) "done executing autopoll_cb with autopoll mask 0x%x" + # pckbd.c pckbd_kbd_read_data(uint32_t val) "0x%02x" pckbd_kbd_read_status(int status) "0x%02x"
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/input/adb.c | 23 ++++++++++++++++++++++- hw/input/trace-events | 7 +++++++ 2 files changed, 29 insertions(+), 1 deletion(-)