hw/net/cadence_gem.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
When CADENCE_GEM_ERR_DEBUG is turned on, there are several
compilation errors in DB_PRINT(). Fix them.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/net/cadence_gem.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index d412085..7516e8f 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -983,8 +983,9 @@ 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 " TARGET_FMT_plx "\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]) +
@@ -1157,7 +1158,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%lx\n", (unsigned)packet_desc_addr,
(unsigned)tx_desc_get_length(desc),
sizeof(tx_packet) - (p - tx_packet));
break;
--
2.7.4
On Mon, Aug 05, 2019 at 08:52:54AM -0700, Bin Meng wrote:
> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> compilation errors in DB_PRINT(). Fix them.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/net/cadence_gem.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d412085..7516e8f 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -983,8 +983,9 @@ 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 " TARGET_FMT_plx "\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]) +
> @@ -1157,7 +1158,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%lx\n", (unsigned)packet_desc_addr,
What about using 'z' modifier? I mean "0x%zx" to print sizeof(..).
Thanks,
Stefano
On Tue, Aug 6, 2019 at 6:57 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Aug 05, 2019 at 08:52:54AM -0700, Bin Meng wrote:
> > When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> > compilation errors in DB_PRINT(). Fix them.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > hw/net/cadence_gem.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index d412085..7516e8f 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -983,8 +983,9 @@ 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 " TARGET_FMT_plx "\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]) +
> > @@ -1157,7 +1158,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%lx\n", (unsigned)packet_desc_addr,
>
> What about using 'z' modifier? I mean "0x%zx" to print sizeof(..).
Yes, good idea. Will do in v2. Thanks!
Regards,
Bin
When CADENCE_GEM_ERR_DEBUG is turned on, there are several
compilation errors in DB_PRINT(). Fix them.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
Changes in v2:
- use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
- use 'z' modifier to print sizeof(..)
hw/net/cadence_gem.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index d412085..b6ff2c1 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -983,8 +983,9 @@ 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]) +
@@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
(unsigned)tx_desc_get_length(desc),
sizeof(tx_packet) - (p - tx_packet));
break;
--
2.7.4
Bin Meng <bmeng.cn@gmail.com> writes:
> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> compilation errors in DB_PRINT(). Fix them.
The first fix should be to ensure the format strings are validated in
normal compilation. This can be achieved by allowing the compiler to
optimise away debug strings with constant folding... for example:
#define tlb_debug(fmt, ...) do { \
if (DEBUG_TLB_LOG_GATE) { \
qemu_log_mask(CPU_LOG_MMU, "%s: " fmt, __func__, \
## __VA_ARGS__); \
} else if (DEBUG_TLB_GATE) { \
fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
} \
} while (0)
However ultimately most debug printfs are either a) stale leftovers from
original development or b) could be considered for conversion to
tracepoints.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
> - use 'z' modifier to print sizeof(..)
>
> hw/net/cadence_gem.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d412085..b6ff2c1 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -983,8 +983,9 @@ 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]) +
> @@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
> (unsigned)tx_desc_get_length(desc),
> sizeof(tx_packet) - (p - tx_packet));
> break;
--
Alex Bennée
Hi,
On 8/8/19 6:44 AM, Bin Meng wrote:
> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> compilation errors in DB_PRINT(). Fix them.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
Please don't reply to previous version, post as a new thread (it is
harder to notice your new versions in a emails threaded view).
> - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
> - use 'z' modifier to print sizeof(..)
>
> hw/net/cadence_gem.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d412085..b6ff2c1 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -983,8 +983,9 @@ 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",
rx_desc_get_buffer() returns a uint64_t, shouldn't you use a PRIx64
format here?
> + MIN(bytes_to_copy, rxbufsize),
Nitpick #1: since you are cleaning this file up, bytes_to_copy and
rxbufsize are both unsigned, so the first format should be %u instead of %d.
> + 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]) +
> @@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
Nitpick #2: packet_desc_addr is of type hwaddr, so removing the cast the
1st format is HWADDR_PRIx, also removing the 2nd cast the 2nd format is
PRIx64.
Then the 3rd format is now correct.
> (unsigned)tx_desc_get_length(desc),
> sizeof(tx_packet) - (p - tx_packet));
> break;
>
On Thu, Aug 8, 2019 at 1:21 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi,
>
> On 8/8/19 6:44 AM, Bin Meng wrote:
> > When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> > compilation errors in DB_PRINT(). Fix them.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > ---
> >
> > Changes in v2:
>
> Please don't reply to previous version, post as a new thread (it is
> harder to notice your new versions in a emails threaded view).
>
OK.
> > - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
> > - use 'z' modifier to print sizeof(..)
> >
> > hw/net/cadence_gem.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index d412085..b6ff2c1 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -983,8 +983,9 @@ 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",
>
> rx_desc_get_buffer() returns a uint64_t, shouldn't you use a PRIx64
> format here?
HWADDR_PRIx expands to PRIx64. I got your point that since it does not
return hwaddr, so we should use PRIx64 directly. Correct?
>
> > + MIN(bytes_to_copy, rxbufsize),
>
> Nitpick #1: since you are cleaning this file up, bytes_to_copy and
> rxbufsize are both unsigned, so the first format should be %u instead of %d.
Sure, will do in v3.
>
> > + 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]) +
> > @@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
>
> Nitpick #2: packet_desc_addr is of type hwaddr, so removing the cast the
> 1st format is HWADDR_PRIx, also removing the 2nd cast the 2nd format is
> PRIx64.
packet_desc_addr() return unsigned, so %x should be OK.
>
> Then the 3rd format is now correct.
>
> > (unsigned)tx_desc_get_length(desc),
> > sizeof(tx_packet) - (p - tx_packet));
> > break;
> >
Regards,
Bin
On 8/8/19 8:36 AM, Bin Meng wrote:
> On Thu, Aug 8, 2019 at 1:21 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi,
>>
>> On 8/8/19 6:44 AM, Bin Meng wrote:
>>> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
>>> compilation errors in DB_PRINT(). Fix them.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>
>> Please don't reply to previous version, post as a new thread (it is
>> harder to notice your new versions in a emails threaded view).
>>
>
> OK.
>
>>> - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
>>> - use 'z' modifier to print sizeof(..)
>>>
>>> hw/net/cadence_gem.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index d412085..b6ff2c1 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -983,8 +983,9 @@ 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",
>>
>> rx_desc_get_buffer() returns a uint64_t, shouldn't you use a PRIx64
>> format here?
>
> HWADDR_PRIx expands to PRIx64. I got your point that since it does not
> return hwaddr, so we should use PRIx64 directly. Correct?
>
>>
>>> + MIN(bytes_to_copy, rxbufsize),
>>
>> Nitpick #1: since you are cleaning this file up, bytes_to_copy and
>> rxbufsize are both unsigned, so the first format should be %u instead of %d.
>
> Sure, will do in v3.
>
>>
>>> + 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]) +
>>> @@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
>>
>> Nitpick #2: packet_desc_addr is of type hwaddr, so removing the cast the
>> 1st format is HWADDR_PRIx, also removing the 2nd cast the 2nd format is
>> PRIx64.
>
> packet_desc_addr() return unsigned, so %x should be OK.
'packet_desc_addr' is of type hwaddr,
'(unsigned)packet_desc_addr' is casted to type unsigned.
Anyhow I now remember I already reviewed this patch:
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03263.html
>>
>> Then the 3rd format is now correct.
>>
>>> (unsigned)tx_desc_get_length(desc),
>>> sizeof(tx_packet) - (p - tx_packet));
>>> break;
>>>
On Thu, Aug 8, 2019 at 3:01 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/8/19 8:36 AM, Bin Meng wrote:
> > On Thu, Aug 8, 2019 at 1:21 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 8/8/19 6:44 AM, Bin Meng wrote:
> >>> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> >>> compilation errors in DB_PRINT(). Fix them.
> >>>
> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >>>
> >>> ---
> >>>
> >>> Changes in v2:
> >>
> >> Please don't reply to previous version, post as a new thread (it is
> >> harder to notice your new versions in a emails threaded view).
> >>
> >
> > OK.
> >
> >>> - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
> >>> - use 'z' modifier to print sizeof(..)
> >>>
> >>> hw/net/cadence_gem.c | 7 ++++---
> >>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> >>> index d412085..b6ff2c1 100644
> >>> --- a/hw/net/cadence_gem.c
> >>> +++ b/hw/net/cadence_gem.c
> >>> @@ -983,8 +983,9 @@ 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",
> >>
> >> rx_desc_get_buffer() returns a uint64_t, shouldn't you use a PRIx64
> >> format here?
> >
> > HWADDR_PRIx expands to PRIx64. I got your point that since it does not
> > return hwaddr, so we should use PRIx64 directly. Correct?
> >
> >>
> >>> + MIN(bytes_to_copy, rxbufsize),
> >>
> >> Nitpick #1: since you are cleaning this file up, bytes_to_copy and
> >> rxbufsize are both unsigned, so the first format should be %u instead of %d.
> >
> > Sure, will do in v3.
> >
> >>
> >>> + 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]) +
> >>> @@ -1157,7 +1158,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%zx\n", (unsigned)packet_desc_addr,
> >>
> >> Nitpick #2: packet_desc_addr is of type hwaddr, so removing the cast the
> >> 1st format is HWADDR_PRIx, also removing the 2nd cast the 2nd format is
> >> PRIx64.
> >
> > packet_desc_addr() return unsigned, so %x should be OK.
>
> 'packet_desc_addr' is of type hwaddr,
Ah, a typo! I meant to say: tx_desc_get_length() returns unsigned, so
just removing the 2nd cast is correct. But you wanted to change to
PRIx64 which I don't understand.
> '(unsigned)packet_desc_addr' is casted to type unsigned.
>
> Anyhow I now remember I already reviewed this patch:
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03263.html
>
OK, looks the same issue was discovered by someone else :)
Regards,
Bin
© 2016 - 2024 Red Hat, Inc.