| Message ID | 20190611145556.12940-1-rfried.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] net: cadence_gem: fix compilation error when debug is on | expand |
Le 11/06/2019 à 16:55, Ramon Fried a écrit : > defining CADENCE_GEM_ERR_DEBUG causes compilation > errors, fix that. > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com> > --- > v2: change %lx to HWADDR_PRIx and %lx to %zdx HWADDR_PRIx is to use with hwaddr type (packet_desc_addr). rx_desc_get_buffer() is uint64_t, you must use PRIx64. It may be better to remove also the cast to unsigned. Thanks, Laurent > > hw/net/cadence_gem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 7f63411430..e9b1b1e2eb 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -982,8 +982,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) > return -1; > } > > - DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize), > - rx_desc_get_buffer(s->rx_desc[q])); > + DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n", MIN(bytes_to_copy, rxbufsize), > + rx_desc_get_buffer(s, s->rx_desc[q])); > > /* Copy packet data to emulated DMA buffer */ > address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) + > @@ -1156,7 +1156,7 @@ static void gem_transmit(CadenceGEMState *s) > if (tx_desc_get_length(desc) > sizeof(tx_packet) - > (p - tx_packet)) { > DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \ > - "0x%x\n", (unsigned)packet_desc_addr, > + "0x%zdx\n", (unsigned)packet_desc_addr, > (unsigned)tx_desc_get_length(desc), > sizeof(tx_packet) - (p - tx_packet)); > break; >
On Tue, Jun 11, 2019 at 7:21 PM Laurent Vivier <laurent@vivier.eu> wrote: > Le 11/06/2019 à 16:55, Ramon Fried a écrit : > > defining CADENCE_GEM_ERR_DEBUG causes compilation > > errors, fix that. > > > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com> > > --- > > v2: change %lx to HWADDR_PRIx and %lx to %zdx > > HWADDR_PRIx is to use with hwaddr type (packet_desc_addr). > rx_desc_get_buffer() is uint64_t, you must use PRIx64. > It may be better to remove also the cast to unsigned. > Other places in the code also use HWADDR_PRIx with rx_desc_get_buffer(). Should I change them also ? > > Thanks, > Laurent > > > > hw/net/cadence_gem.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > > index 7f63411430..e9b1b1e2eb 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -982,8 +982,8 @@ static ssize_t gem_receive(NetClientState *nc, const > uint8_t *buf, size_t size) > > return -1; > > } > > > > - DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, > rxbufsize), > > - rx_desc_get_buffer(s->rx_desc[q])); > > + DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n", > MIN(bytes_to_copy, rxbufsize), > > + rx_desc_get_buffer(s, s->rx_desc[q])); > > > > /* Copy packet data to emulated DMA buffer */ > > address_space_write(&s->dma_as, rx_desc_get_buffer(s, > s->rx_desc[q]) + > > @@ -1156,7 +1156,7 @@ static void gem_transmit(CadenceGEMState *s) > > if (tx_desc_get_length(desc) > sizeof(tx_packet) - > > (p - tx_packet)) { > > DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x > space " \ > > - "0x%x\n", (unsigned)packet_desc_addr, > > + "0x%zdx\n", (unsigned)packet_desc_addr, > > (unsigned)tx_desc_get_length(desc), > > sizeof(tx_packet) - (p - tx_packet)); > > break; > > > >
Le 12/06/2019 à 06:27, Ramon Fried a écrit : > > > On Tue, Jun 11, 2019 at 7:21 PM Laurent Vivier <laurent@vivier.eu > <mailto:laurent@vivier.eu>> wrote: > > Le 11/06/2019 à 16:55, Ramon Fried a écrit : > > defining CADENCE_GEM_ERR_DEBUG causes compilation > > errors, fix that. > > > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com > <mailto:rfried.dev@gmail.com>> > > --- > > v2: change %lx to HWADDR_PRIx and %lx to %zdx > > HWADDR_PRIx is to use with hwaddr type (packet_desc_addr). > rx_desc_get_buffer() is uint64_t, you must use PRIx64. > It may be better to remove also the cast to unsigned. > > Other places in the code also use HWADDR_PRIx with rx_desc_get_buffer(). > Should I change them also ? No, in fact you should change the return type of rx_desc_get_buffer() to hwaddr. Thanks, Laurent
On 6/11/19 4:55 PM, Ramon Fried wrote: > defining CADENCE_GEM_ERR_DEBUG causes compilation > errors, fix that. > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com> > --- > v2: change %lx to HWADDR_PRIx and %lx to %zdx > > hw/net/cadence_gem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 7f63411430..e9b1b1e2eb 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -982,8 +982,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) > return -1; > } > > - DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize), > - rx_desc_get_buffer(s->rx_desc[q])); > + DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n", MIN(bytes_to_copy, rxbufsize), > + rx_desc_get_buffer(s, s->rx_desc[q])); > > /* Copy packet data to emulated DMA buffer */ > address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) + > @@ -1156,7 +1156,7 @@ static void gem_transmit(CadenceGEMState *s) > if (tx_desc_get_length(desc) > sizeof(tx_packet) - > (p - tx_packet)) { > DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \ > - "0x%x\n", (unsigned)packet_desc_addr, > + "0x%zdx\n", (unsigned)packet_desc_addr, The format is either "zd" or "zx". I misunderstood how you want to print it, if you want hexadecimal, it should be "zx" then. > (unsigned)tx_desc_get_length(desc), > sizeof(tx_packet) - (p - tx_packet)); > break; >
Ramon Fried <rfried.dev@gmail.com> writes: > defining CADENCE_GEM_ERR_DEBUG causes compilation > errors, fix that. It would be worth doing something like: #ifdef CADENCE_GEM_ERR_DEBUG #define CADENCE_GEM_GATE 1 #else #define CADENCE_GEM_GATE 0 #endif #define DB_PRINT(...) do { \ if (CADENCE_GEM_GATE) { \ fprintf(stderr, ": %s: ", __func__); \ fprintf(stderr, ## __VA_ARGS__); \ } \ } while (0) So these format strings don't go stale in and only get detected on --debug builds. While your at it I suspect the fprintf debug would be better of as: qemu_log("%s: " fmt, __func__, ## args); > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com> > --- > v2: change %lx to HWADDR_PRIx and %lx to %zdx > > hw/net/cadence_gem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 7f63411430..e9b1b1e2eb 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -982,8 +982,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) > return -1; > } > > - DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize), > - rx_desc_get_buffer(s->rx_desc[q])); > + DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n", MIN(bytes_to_copy, rxbufsize), > + rx_desc_get_buffer(s, s->rx_desc[q])); > > /* Copy packet data to emulated DMA buffer */ > address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) + > @@ -1156,7 +1156,7 @@ static void gem_transmit(CadenceGEMState *s) > if (tx_desc_get_length(desc) > sizeof(tx_packet) - > (p - tx_packet)) { > DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \ > - "0x%x\n", (unsigned)packet_desc_addr, > + "0x%zdx\n", (unsigned)packet_desc_addr, > (unsigned)tx_desc_get_length(desc), > sizeof(tx_packet) - (p - tx_packet)); > break; -- Alex Bennée
On 6/12/19 1:03 PM, Alex Bennée wrote: > > Ramon Fried <rfried.dev@gmail.com> writes: > >> defining CADENCE_GEM_ERR_DEBUG causes compilation >> errors, fix that. > > It would be worth doing something like: > > #ifdef CADENCE_GEM_ERR_DEBUG > #define CADENCE_GEM_GATE 1 > #else > #define CADENCE_GEM_GATE 0 > #endif > > #define DB_PRINT(...) do { \ > if (CADENCE_GEM_GATE) { \ > fprintf(stderr, ": %s: ", __func__); \ > fprintf(stderr, ## __VA_ARGS__); \ > } \ > } while (0) > > So these format strings don't go stale in and only get detected on > --debug builds. > > While your at it I suspect the fprintf debug would be better of as: > > qemu_log("%s: " fmt, __func__, ## args); Indeed, or even see if it is worth converting to full-on trace points for dynamic control of whether to catch these things without having to recompile debug on or off.
On Wed, Jun 12, 2019 at 10:40 AM Laurent Vivier <laurent@vivier.eu> wrote: > Le 12/06/2019 à 06:27, Ramon Fried a écrit : > > > > > > On Tue, Jun 11, 2019 at 7:21 PM Laurent Vivier <laurent@vivier.eu > > <mailto:laurent@vivier.eu>> wrote: > > > > Le 11/06/2019 à 16:55, Ramon Fried a écrit : > > > defining CADENCE_GEM_ERR_DEBUG causes compilation > > > errors, fix that. > > > > > > Signed-off-by: Ramon Fried <rfried.dev@gmail.com > > <mailto:rfried.dev@gmail.com>> > > > --- > > > v2: change %lx to HWADDR_PRIx and %lx to %zdx > > > > HWADDR_PRIx is to use with hwaddr type (packet_desc_addr). > > rx_desc_get_buffer() is uint64_t, you must use PRIx64. > > It may be better to remove also the cast to unsigned. > > > > Other places in the code also use HWADDR_PRIx with rx_desc_get_buffer(). > > Should I change them also ? > > No, in fact you should change the return type of rx_desc_get_buffer() to > hwaddr. > Make sense. I'll do it in a different patch. > > Thanks, > Laurent >
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 7f63411430..e9b1b1e2eb 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -982,8 +982,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) return -1; } - DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize), - rx_desc_get_buffer(s->rx_desc[q])); + DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n", MIN(bytes_to_copy, rxbufsize), + rx_desc_get_buffer(s, s->rx_desc[q])); /* Copy packet data to emulated DMA buffer */ address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) + @@ -1156,7 +1156,7 @@ static void gem_transmit(CadenceGEMState *s) if (tx_desc_get_length(desc) > sizeof(tx_packet) - (p - tx_packet)) { DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \ - "0x%x\n", (unsigned)packet_desc_addr, + "0x%zdx\n", (unsigned)packet_desc_addr, (unsigned)tx_desc_get_length(desc), sizeof(tx_packet) - (p - tx_packet)); break;
defining CADENCE_GEM_ERR_DEBUG causes compilation errors, fix that. Signed-off-by: Ramon Fried <rfried.dev@gmail.com> --- v2: change %lx to HWADDR_PRIx and %lx to %zdx hw/net/cadence_gem.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)