Skip to contentSkip to conversation

Cadence GEM patches

PATCH Superseded Peter Crosthwaite 2 participants 11 messages May 7, 2014 – May 26, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Peter Crosthwaite

author
#0 lore raw

cover Cadence GEM patches

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(-)
Signature · 1 line
1.9.2.1.g06c4abd
author
#1 lore raw

PATCH 1/4 net: cadence_gem: Fix Tx descriptor update

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 <[email protected]>

---

 hw/net/cadence_gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
hw/net/cadence_gem.c +5 -3
index e34b25e..1c36e48 100644
@@ -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];
Signature · 1 line
1.9.2.1.g06c4abd
#2 lore raw

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

[...]

         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

#3 lore raw

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

Quoted 17 lines · Michael Tokarev

[...]

         if (tx_desc_get_last(desc)) {
hw/net/cadence_gem.c +5 -3
+            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 */
Quoted 6 lines

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.. ;)

Quoted 9 lines

and, more interesting,

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

    ? :)

Thanks,

/mjt

author
#4 lore raw

PATCH 2/4 net: cadence_gem: Add Tx descriptor fetch printf

In reply to 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 <[email protected]>

---

 hw/net/cadence_gem.c | 3 +++
 1 file changed, 3 insertions(+)
hw/net/cadence_gem.c +3 -0
index 1c36e48..f999886 100644
@@ -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));
     }
Signature · 1 line
1.9.2.1.g06c4abd
author
#5 lore raw

PATCH 3/4 net: cadence_gem: Fix top comment

In reply to Peter Crosthwaite

To indicate Cadence GEM not Xilinx.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/net/cadence_gem.c +1 -1
index f999886..7d13e7c 100644
@@ -1,5 +1,5 @@
 /*
- * QEMU Xilinx GEM emulation
+ * QEMU Cadence GEM emulation
  *
  * Copyright (c) 2011 Xilinx, Inc.
  *
Signature · 1 line
1.9.2.1.g06c4abd
author
#6 lore raw

PATCH 4/4 net: cadence_gem: Comment spelling sweep

In reply to Peter Crosthwaite

Fix some typos in comments.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/net/cadence_gem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
hw/net/cadence_gem.c +6 -6
index 7d13e7c..4e49f07 100644
@@ -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 {
Signature · 1 line
1.9.2.1.g06c4abd
author
#7 lore raw
In reply to Peter Crosthwaite

Ping!

Quoted 22 lines · 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

author
#8 lore raw

Ping^2!

I'll try trivial queue too :)

Regards,
Peter

Quoted 26 lines · Peter Crosthwaite

Ping!

Quoted 22 lines · 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

#9 lore raw
Quoted 3 lines · Peter Crosthwaite

Ping^2!

I'll try trivial queue too :)

Actually this looks like trivial material.

I'll comment in one place, for all.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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".

Quoted 1 line
Quoted 1 line
Quoted 1 line

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?

Quoted 1 line
Quoted 1 line
Quoted 1 line

net: cadence_gem: Fix top comment

I applied this one.

Quoted 1 line
Quoted 1 line
Quoted 1 line

net: cadence_gem: Comment spelling sweep

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

/mjt

author
#10 lore raw
Quoted 23 lines · Michael Tokarev
Quoted 3 lines · Peter Crosthwaite

Ping^2!

I'll try trivial queue too :)

Actually this looks like trivial material.

I'll comment in one place, for all.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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

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

...

(quoted diff) +2 -1
+            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));
Quoted 3 lines

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

Fixed.

Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

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,

Quoted 4 lines

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.

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

net: cadence_gem: Fix top comment

I applied this one.

Thanks.

Spinning V2. Thanks for review.

Regards,
Peter

Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

net: cadence_gem: Comment spelling sweep

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

/mjt