qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT()
@ 2019-08-05 15:52 Bin Meng
  2019-08-06 10:57 ` Stefano Garzarella
  2019-08-08  4:44 ` [Qemu-devel] [PATCH v2] " Bin Meng
  0 siblings, 2 replies; 9+ messages in thread
From: Bin Meng @ 2019-08-05 15:52 UTC (permalink / raw)
  To: Edgar E. Iglesias, Jason Wang, Peter Maydell, qemu-arm, qemu-devel

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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT()
  2019-08-05 15:52 [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT() Bin Meng
@ 2019-08-06 10:57 ` Stefano Garzarella
  2019-08-08  4:45   ` Bin Meng
  2019-08-08  4:44 ` [Qemu-devel] [PATCH v2] " Bin Meng
  1 sibling, 1 reply; 9+ messages in thread
From: Stefano Garzarella @ 2019-08-06 10:57 UTC (permalink / raw)
  To: Bin Meng
  Cc: Edgar E. Iglesias, Jason Wang, qemu-arm, qemu-devel, Peter Maydell

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
  2019-08-05 15:52 [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT() Bin Meng
  2019-08-06 10:57 ` Stefano Garzarella
@ 2019-08-08  4:44 ` Bin Meng
  2019-08-08  5:21   ` Philippe Mathieu-Daudé
  2019-08-08  9:08   ` [Qemu-devel] [Qemu-arm] " Alex Bennée
  1 sibling, 2 replies; 9+ 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] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT()
  2019-08-06 10:57 ` Stefano Garzarella
@ 2019-08-08  4:45   ` Bin Meng
  0 siblings, 0 replies; 9+ messages in thread
From: Bin Meng @ 2019-08-08  4:45 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Edgar E. Iglesias, Jason Wang, qemu-arm,
	qemu-devel@nongnu.org Developers, Peter Maydell

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
  2019-08-08  4:44 ` [Qemu-devel] [PATCH v2] " Bin Meng
@ 2019-08-08  5:21   ` Philippe Mathieu-Daudé
  2019-08-08  6:36     ` Bin Meng
  2019-08-08  9:08   ` [Qemu-devel] [Qemu-arm] " Alex Bennée
  1 sibling, 1 reply; 9+ 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] 9+ 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é
@ 2019-08-08  6:36     ` Bin Meng
  2019-08-08  7:01       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 9+ 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] 9+ 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
@ 2019-08-08  7:01       ` Philippe Mathieu-Daudé
  2019-08-08  7:18         ` Bin Meng
  0 siblings, 1 reply; 9+ 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] 9+ 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
  0 siblings, 0 replies; 9+ 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] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-arm] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()
  2019-08-08  4:44 ` [Qemu-devel] [PATCH v2] " Bin Meng
  2019-08-08  5:21   ` Philippe Mathieu-Daudé
@ 2019-08-08  9:08   ` Alex Bennée
  1 sibling, 0 replies; 9+ 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] 9+ messages in thread

end of thread, other threads:[~2019-08-08  9:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-05 15:52 [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT() Bin Meng
2019-08-06 10:57 ` Stefano Garzarella
2019-08-08  4:45   ` Bin Meng
2019-08-08  4:44 ` [Qemu-devel] [PATCH v2] " Bin Meng
2019-08-08  5:21   ` Philippe Mathieu-Daudé
2019-08-08  6:36     ` Bin Meng
2019-08-08  7:01       ` Philippe Mathieu-Daudé
2019-08-08  7:18         ` Bin Meng
2019-08-08  9:08   ` [Qemu-devel] [Qemu-arm] " Alex Bennée

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).