[Qemu-devel] [PATCH net v1 0/4] Cadence GEM patches

[Qemu-devel] [PATCH net v1 0/4] Cadence GEM patches

[Peter Crosthwaite]

Hi Stefan, Peter,

Found a bug in the Cadence GEM. Fixed in P1. Have some follow us trivials
as well (P2-4).

Regards,
Peter


Peter Crosthwaite (4):
  net: cadence_gem: Fix Tx descriptor update
  net: cadence_gem: Add Tx descriptor fetch printf
  net: cadence_gem: Fix top comment
  net: cadence_gem: Comment spelling sweep

 hw/net/cadence_gem.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

-- 
1.9.2.1.g06c4abd

[Qemu-devel] [PATCH net v1 1/4] net: cadence_gem: Fix Tx descriptor update

[Peter Crosthwaite]

The local variable "desc" was being used to read-modify-write the
first descriptor (of a multi-desc packet) upon packet completion.
desc however continues to be used by the code as the current
descriptor. Give this first desc RMW it's own local variable to
avoid trampling.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index e34b25e..1c36e48 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -917,14 +917,16 @@ static void gem_transmit(GemState *s)
 
         /* Last descriptor for this packet; hand the whole thing off */
         if (tx_desc_get_last(desc)) {
+            unsigned    desc_first[2];
+
             /* Modify the 1st descriptor of this packet to be owned by
              * the processor.
              */
             cpu_physical_memory_read(s->tx_desc_addr,
-                                     (uint8_t *)&desc[0], sizeof(desc));
-            tx_desc_set_used(desc);
+                                     (uint8_t *)&desc_first[0], sizeof(desc));
+            tx_desc_set_used(desc_first);
             cpu_physical_memory_write(s->tx_desc_addr,
-                                      (uint8_t *)&desc[0], sizeof(desc));
+                                      (uint8_t *)&desc_first[0], sizeof(desc));
             /* Advance the hardare current descriptor past this packet */
             if (tx_desc_get_wrap(desc)) {
                 s->tx_desc_addr = s->regs[GEM_TXQBASE];
-- 
1.9.2.1.g06c4abd

Re: [Qemu-devel] [net, v1, 1/4] net: cadence_gem: Fix Tx descriptor update

[Michael Tokarev]

[...]
         if (tx_desc_get_last(desc)) {
+            unsigned    desc_first[2];
+
             /* Modify the 1st descriptor of this packet to be owned by
              * the processor.
              */
             cpu_physical_memory_read(s->tx_desc_addr,
-                                     (uint8_t *)&desc[0], sizeof(desc));
-            tx_desc_set_used(desc);
+                                     (uint8_t *)&desc_first[0], sizeof(desc));
+            tx_desc_set_used(desc_first);
             cpu_physical_memory_write(s->tx_desc_addr,
-                                      (uint8_t *)&desc[0], sizeof(desc));
+                                      (uint8_t *)&desc_first[0], sizeof(desc));
             /* Advance the hardare current descriptor past this packet */

This is quite fun.

Can we, please,

 a) s/unsigned/uint8_t/ in the variable declaration,
   and remove the useless casts and "readdressing" everywhere?

and, more interesting,

 b) s/sizeof(desc)/sizeof(desc_first)/g
    ? :)

Thanks,

/mjt

Re: [Qemu-devel] [net, v1, 1/4] net: cadence_gem: Fix Tx descriptor update

[Michael Tokarev]

24.05.2014 10:38, Michael Tokarev wrote:
> [...]
>          if (tx_desc_get_last(desc)) {
> +            unsigned    desc_first[2];
> +
>              /* Modify the 1st descriptor of this packet to be owned by
>               * the processor.
>               */
>              cpu_physical_memory_read(s->tx_desc_addr,
> -                                     (uint8_t *)&desc[0], sizeof(desc));
> -            tx_desc_set_used(desc);
> +                                     (uint8_t *)&desc_first[0], sizeof(desc));
> +            tx_desc_set_used(desc_first);
>              cpu_physical_memory_write(s->tx_desc_addr,
> -                                      (uint8_t *)&desc[0], sizeof(desc));
> +                                      (uint8_t *)&desc_first[0], sizeof(desc));
>              /* Advance the hardare current descriptor past this packet */
> 
> This is quite fun.
> 
> Can we, please,
> 
>  a) s/unsigned/uint8_t/ in the variable declaration,
>    and remove the useless casts and "readdressing" everywhere?

Um, no, this is ofcourse wrong - tx_desc_set_used() expects unsigned.
Still it's possible to remove "readdressing", instead of

  (uint8_t *)&desc[0]

use

  (uint8_t *)desc

The second point stands.. ;)

> and, more interesting,
> 
>  b) s/sizeof(desc)/sizeof(desc_first)/g
>     ? :)
> 
> Thanks,
> 
> /mjt
> 

[Qemu-devel] [PATCH net v1 2/4] net: cadence_gem: Add Tx descriptor fetch printf

[Peter Crosthwaite]

Add a debug printf for TX descriptor fetching. This helpful to anyone
needing to debug TX ring buffer traversal. Its also now consistent with
the RX code which has a similar printf.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 1c36e48..f999886 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -886,6 +886,8 @@ static void gem_transmit(GemState *s)
 
     /* read current descriptor */
     packet_desc_addr = s->tx_desc_addr;
+
+    DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
     cpu_physical_memory_read(packet_desc_addr,
                              (uint8_t *)&desc[0], sizeof(desc));
     /* Handle all descriptors owned by hardware */
@@ -968,6 +970,7 @@ static void gem_transmit(GemState *s)
         } else {
             packet_desc_addr += 8;
         }
+        DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
         cpu_physical_memory_read(packet_desc_addr,
                                  (uint8_t *)&desc[0], sizeof(desc));
     }
-- 
1.9.2.1.g06c4abd

[Qemu-devel] [PATCH net v1 3/4] net: cadence_gem: Fix top comment

[Peter Crosthwaite]

To indicate Cadence GEM not Xilinx.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index f999886..7d13e7c 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1,5 +1,5 @@
 /*
- * QEMU Xilinx GEM emulation
+ * QEMU Cadence GEM emulation
  *
  * Copyright (c) 2011 Xilinx, Inc.
  *
-- 
1.9.2.1.g06c4abd

[Qemu-devel] [PATCH net v1 4/4] net: cadence_gem: Comment spelling sweep

[Peter Crosthwaite]

Fix some typos in comments.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/net/cadence_gem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 7d13e7c..4e49f07 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -50,7 +50,7 @@
 #define GEM_IER           (0x00000028/4) /* Interrupt Enable reg */
 #define GEM_IDR           (0x0000002C/4) /* Interrupt Disable reg */
 #define GEM_IMR           (0x00000030/4) /* Interrupt Mask reg */
-#define GEM_PHYMNTNC      (0x00000034/4) /* Phy Maintaince reg */
+#define GEM_PHYMNTNC      (0x00000034/4) /* Phy Maintenance reg */
 #define GEM_RXPAUSE       (0x00000038/4) /* RX Pause Time reg */
 #define GEM_TXPAUSE       (0x0000003C/4) /* TX Pause Time reg */
 #define GEM_TXPARTIALSF   (0x00000040/4) /* TX Partial Store and Forward */
@@ -150,7 +150,7 @@
 #define GEM_NWCTRL_LOCALLOOP   0x00000002 /* Local Loopback */
 
 #define GEM_NWCFG_STRIP_FCS    0x00020000 /* Strip FCS field */
-#define GEM_NWCFG_LERR_DISC    0x00010000 /* Discard RX frames with lenth err */
+#define GEM_NWCFG_LERR_DISC    0x00010000 /* Discard RX frames with len err */
 #define GEM_NWCFG_BUFF_OFST_M  0x0000C000 /* Receive buffer offset mask */
 #define GEM_NWCFG_BUFF_OFST_S  14         /* Receive buffer offset shift */
 #define GEM_NWCFG_UCAST_HASH   0x00000080 /* accept unicast if hash match */
@@ -397,7 +397,7 @@ const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
  */
 static void gem_init_register_masks(GemState *s)
 {
-    /* Mask of register bits which are read only*/
+    /* Mask of register bits which are read only */
     memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
     s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
     s->regs_ro[GEM_NWSTATUS] = 0xFFFFFFFF;
@@ -720,7 +720,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         int      crc_offset;
 
         /* The application wants the FCS field, which QEMU does not provide.
-         * We must try and caclculate one.
+         * We must try and calculate one.
          */
 
         memcpy(rxbuf, buf, size);
@@ -877,7 +877,7 @@ static void gem_transmit(GemState *s)
 
     DB_PRINT("\n");
 
-    /* The packet we will hand off to qemu.
+    /* The packet we will hand off to QEMU.
      * Packets scattered across multiple descriptors are gathered to this
      * one contiguous buffer first.
      */
@@ -929,7 +929,7 @@ static void gem_transmit(GemState *s)
             tx_desc_set_used(desc_first);
             cpu_physical_memory_write(s->tx_desc_addr,
                                       (uint8_t *)&desc_first[0], sizeof(desc));
-            /* Advance the hardare current descriptor past this packet */
+            /* Advance the hardware current descriptor past this packet */
             if (tx_desc_get_wrap(desc)) {
                 s->tx_desc_addr = s->regs[GEM_TXQBASE];
             } else {
-- 
1.9.2.1.g06c4abd

Re: [Qemu-devel] [PATCH net v1 0/4] Cadence GEM patches

[Peter Crosthwaite]

Ping!

On Wed, May 7, 2014 at 9:15 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
>
> Hi Stefan, Peter,
>
> Found a bug in the Cadence GEM. Fixed in P1. Have some follow us trivials
> as well (P2-4).
>
> Regards,
> Peter
>
>
> Peter Crosthwaite (4):
>   net: cadence_gem: Fix Tx descriptor update
>   net: cadence_gem: Add Tx descriptor fetch printf
>   net: cadence_gem: Fix top comment
>   net: cadence_gem: Comment spelling sweep
>
>  hw/net/cadence_gem.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> --
> 1.9.2.1.g06c4abd
>

Re: [Qemu-devel] [PATCH net v1 0/4] Cadence GEM patches

[Peter Crosthwaite]

Ping^2!

I'll try trivial queue too :)

Regards,
Peter

On Wed, May 14, 2014 at 11:17 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Ping!
>
> On Wed, May 7, 2014 at 9:15 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>>
>> Hi Stefan, Peter,
>>
>> Found a bug in the Cadence GEM. Fixed in P1. Have some follow us trivials
>> as well (P2-4).
>>
>> Regards,
>> Peter
>>
>>
>> Peter Crosthwaite (4):
>>   net: cadence_gem: Fix Tx descriptor update
>>   net: cadence_gem: Add Tx descriptor fetch printf
>>   net: cadence_gem: Fix top comment
>>   net: cadence_gem: Comment spelling sweep
>>
>>  hw/net/cadence_gem.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> --
>> 1.9.2.1.g06c4abd
>>

Re: [Qemu-devel] [Qemu-trivial] [PATCH net v1 0/4] Cadence GEM patches

[Michael Tokarev]

24.05.2014 03:06, Peter Crosthwaite wrote:
> Ping^2!
> 
> I'll try trivial queue too :)

Actually this looks like trivial material.

I'll comment in one place, for all.


>>> Peter Crosthwaite (4):
>>>   net: cadence_gem: Fix Tx descriptor update

This appears to be a bugfix, but with an interesting "incomplete"
update:

...
+            unsigned    desc_first[2];
...
             cpu_physical_memory_read(s->tx_desc_addr,
-                                     (uint8_t *)&desc[0], sizeof(desc));
+                                     (uint8_t *)&desc_first[0], sizeof(desc));

sizeof(desc_first), not sizeof(desc).  Also can drop extra
"readdressing".

>>>   net: cadence_gem: Add Tx descriptor fetch printf

+    DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);

packet_desc_addr is hwaddr, which is uint64_t, here it is being cast
to unsigned,  Is it right?  Other code in this file does the same,
but it still does not mean it's right.  Maybe it is because it uses
only the lower 32 bits of it, or that the high bits just aren't
useful for debugging?

>>>   net: cadence_gem: Fix top comment

I applied this one.

>>>   net: cadence_gem: Comment spelling sweep

This look okay, except that it clashes a bit with the first.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH net v1 0/4] Cadence GEM patches

[Peter Crosthwaite]

On Sat, May 24, 2014 at 5:17 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 24.05.2014 03:06, Peter Crosthwaite wrote:
>> Ping^2!
>>
>> I'll try trivial queue too :)
>
> Actually this looks like trivial material.
>
> I'll comment in one place, for all.
>
>
>>>> Peter Crosthwaite (4):
>>>>   net: cadence_gem: Fix Tx descriptor update
>
> This appears to be a bugfix, but with an interesting "incomplete"
> update:
>
> ...
> +            unsigned    desc_first[2];
> ...
>              cpu_physical_memory_read(s->tx_desc_addr,
> -                                     (uint8_t *)&desc[0], sizeof(desc));
> +                                     (uint8_t *)&desc_first[0], sizeof(desc));
>
> sizeof(desc_first), not sizeof(desc).  Also can drop extra
> "readdressing".
>

Fixed.

>>>>   net: cadence_gem: Add Tx descriptor fetch printf
>
> +    DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
>
> packet_desc_addr is hwaddr, which is uint64_t, here it is being cast
> to unsigned,  Is it right?

Probably not.

  Other code in this file does the same,
> but it still does not mean it's right.  Maybe it is because it uses
> only the lower 32 bits of it, or that the high bits just aren't
> useful for debugging?
>

Well they are not useful because the IP is only 32-bit but converting
to HWADDR_PRIx because its probably the right thing to do anyway.

>>>>   net: cadence_gem: Fix top comment
>
> I applied this one.
>

Thanks.

Spinning V2. Thanks for review.

Regards,
Peter

>>>>   net: cadence_gem: Comment spelling sweep
>
> This look okay, except that it clashes a bit with the first.
>
> /mjt
>