* [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-08 4:44 ` Bin Meng
0 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-08-08 4:44 UTC (permalink / raw)
To: Alistair Francis, Edgar E. Iglesias, Jason Wang, Peter Maydell,
Stefano Garzarella, qemu-devel, qemu-arm
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
2019-08-08 4:44 ` [Qemu-devel] " Bin Meng
@ 2019-08-08 5:21 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 5:21 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Edgar E. Iglesias, Jason Wang,
Peter Maydell, Stefano Garzarella, qemu-devel, qemu-arm
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;
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-08 5:21 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 5:21 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Edgar E. Iglesias, Jason Wang,
Peter Maydell, Stefano Garzarella, qemu-devel, qemu-arm
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;
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
2019-08-08 5:21 ` Philippe Mathieu-Daudé
@ 2019-08-08 6:36 ` Bin Meng
-1 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-08-08 6:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Jason Wang, qemu-devel@nongnu.org Developers,
qemu-arm, Alistair Francis, Stefano Garzarella
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-08 6:36 ` Bin Meng
0 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-08-08 6:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Jason Wang, qemu-devel@nongnu.org Developers,
qemu-arm, Alistair Francis, Edgar E. Iglesias, Stefano Garzarella
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
2019-08-08 6:36 ` Bin Meng
@ 2019-08-08 7:01 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 7:01 UTC (permalink / raw)
To: Bin Meng
Cc: Peter Maydell, Jason Wang, qemu-devel@nongnu.org Developers,
qemu-arm, Alistair Francis, Stefano Garzarella
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;
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-08 7:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 7:01 UTC (permalink / raw)
To: Bin Meng
Cc: Peter Maydell, Jason Wang, qemu-devel@nongnu.org Developers,
qemu-arm, Alistair Francis, Edgar E. Iglesias, Stefano Garzarella
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;
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
2019-08-08 7:01 ` Philippe Mathieu-Daudé
(?)
@ 2019-08-08 7:18 ` Bin Meng
-1 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-08-08 7:18 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Jason Wang, qemu-devel@nongnu.org Developers,
qemu-arm, Alistair Francis, Edgar E. Iglesias, Stefano Garzarella
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-arm] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
2019-08-08 4:44 ` [Qemu-devel] " Bin Meng
@ 2019-08-08 9:08 ` Alex Bennée
-1 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2019-08-08 9:08 UTC (permalink / raw)
To: qemu-arm
Cc: Alistair Francis, Edgar E. Iglesias, Jason Wang, Peter Maydell,
Stefano Garzarella, qemu-devel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-08 9:08 ` Alex Bennée
0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2019-08-08 9:08 UTC (permalink / raw)
To: qemu-arm
Cc: Peter Maydell, Jason Wang, qemu-devel, Alistair Francis,
Edgar E. Iglesias, Stefano Garzarella
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
^ permalink raw reply [flat|nested] 16+ messages in thread