diff mbox series

[PULL,V2,01/23] dp8393x: Mask EOL bit from descriptor addresses

Message ID 1583230242-14597-2-git-send-email-jasowang@redhat.com
State New
Headers show
Series [PULL,V2,01/23] dp8393x: Mask EOL bit from descriptor addresses | expand

Commit Message

Jason Wang March 3, 2020, 10:10 a.m. UTC 
From: Finn Thain <fthain@telegraphics.com.au>

The Least Significant bit of a descriptor address register is used as
an EOL flag. It has to be masked when the register value is to be used
as an actual address for copying memory around. But when the registers
are to be updated the EOL bit should not be masked.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/dp8393x.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Finn Thain March 3, 2020, 10:44 p.m. UTC | #1 
Hi Jason,

The patch in this pull request (since merged) differs from the patch that 
I sent. In particular, the change below is missing from commit 88f632fbb1 
("dp8393x: Mask EOL bit from descriptor addresses") in mainline.

--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -525,8 +525,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                                  * (4 + 3 * s->regs[SONIC_TFC]),
                                MEMTXATTRS_UNSPECIFIED, s->data,
                                size);
-            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-            if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
             }

Please compare with "[PATCH v4 01/14] dp8393x: Mask EOL bit from 
descriptor addresses" in the mailing list archives: 
https://lore.kernel.org/qemu-devel/d6e8d06ad4d02f4a30c4caa6001967f806f21a1a.1580290069.git.fthain@telegraphics.com.au/

It appears that this portion of my patch went missing when merge conflicts 
were resolved. The conflicts were apparently caused by commit 19f7034773 
("Avoid address_space_rw() with a constant is_write argument").

Regards,
Finn

On Tue, 3 Mar 2020, Jason Wang wrote:

> From: Finn Thain <fthain@telegraphics.com.au>
> 
> The Least Significant bit of a descriptor address register is used as
> an EOL flag. It has to be masked when the register value is to be used
> as an actual address for copying memory around. But when the registers
> are to be updated the EOL bit should not be masked.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/dp8393x.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 7045193..216d44b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
>  #define SONIC_ISR_PINT   0x0800
>  #define SONIC_ISR_LCD    0x1000
>  
> +#define SONIC_DESC_EOL   0x0001
> +#define SONIC_DESC_ADDR  0xFFFE
> +
>  #define TYPE_DP8393X "dp8393x"
>  #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
>  
> @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
>  
>  static uint32_t dp8393x_crda(dp8393xState *s)
>  {
> -    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> +    return (s->regs[SONIC_URDA] << 16) |
> +           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
>  }
>  
>  static uint32_t dp8393x_rbwc(dp8393xState *s)
> @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
>  
>  static uint32_t dp8393x_ttda(dp8393xState *s)
>  {
> -    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> +    return (s->regs[SONIC_UTDA] << 16) |
> +           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
>  }
>  
>  static uint32_t dp8393x_wt(dp8393xState *s)
> @@ -509,7 +514,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>                                 MEMTXATTRS_UNSPECIFIED, s->data,
>                                 size);
>              s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> -            if (dp8393x_get(s, width, 0) & 0x1) {
> +            if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>                  /* EOL detected */
>                  break;
>              }
> @@ -765,13 +770,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>      /* XXX: Check byte ordering */
>  
>      /* Check for EOL */
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>          /* Are we still in resource exhaustion? */
>          size = sizeof(uint16_t) * 1 * width;
>          address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
>          address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
>                             s->data, size);
> -        if (dp8393x_get(s, width, 0) & 0x1) {
> +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>              /* Still EOL ; stop reception */
>              return -1;
>          } else {
> @@ -831,7 +836,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>                         dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
>                         MEMTXATTRS_UNSPECIFIED, s->data, size);
>      s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>          /* EOL detected */
>          s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>      } else {
>
Jason Wang March 4, 2020, 2:43 a.m. UTC | #2 
On 2020/3/4 上午6:44, Finn Thain wrote:
> Hi Jason,
>
> The patch in this pull request (since merged) differs from the patch that
> I sent. In particular, the change below is missing from commit 88f632fbb1
> ("dp8393x: Mask EOL bit from descriptor addresses") in mainline.
>
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -525,8 +525,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>                                    * (4 + 3 * s->regs[SONIC_TFC]),
>                                  MEMTXATTRS_UNSPECIFIED, s->data,
>                                  size);
> -            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> -            if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> +            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> +            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
>                   /* EOL detected */
>                   break;
>               }
>
> Please compare with "[PATCH v4 01/14] dp8393x: Mask EOL bit from
> descriptor addresses" in the mailing list archives:
> https://lore.kernel.org/qemu-devel/d6e8d06ad4d02f4a30c4caa6001967f806f21a1a.1580290069.git.fthain@telegraphics.com.au/
>
> It appears that this portion of my patch went missing when merge conflicts
> were resolved. The conflicts were apparently caused by commit 19f7034773
> ("Avoid address_space_rw() with a constant is_write argument").
>
> Regards,
> Finn


Exactly.

Please send a patch to fix this.

Thanks
diff mbox series

Patch

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 7045193..216d44b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -145,6 +145,9 @@  do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
 #define SONIC_ISR_PINT   0x0800
 #define SONIC_ISR_LCD    0x1000
 
+#define SONIC_DESC_EOL   0x0001
+#define SONIC_DESC_ADDR  0xFFFE
+
 #define TYPE_DP8393X "dp8393x"
 #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
 
@@ -197,7 +200,8 @@  static uint32_t dp8393x_crba(dp8393xState *s)
 
 static uint32_t dp8393x_crda(dp8393xState *s)
 {
-    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
+    return (s->regs[SONIC_URDA] << 16) |
+           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_rbwc(dp8393xState *s)
@@ -217,7 +221,8 @@  static uint32_t dp8393x_tsa(dp8393xState *s)
 
 static uint32_t dp8393x_ttda(dp8393xState *s)
 {
-    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
+    return (s->regs[SONIC_UTDA] << 16) |
+           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_wt(dp8393xState *s)
@@ -509,7 +514,7 @@  static void dp8393x_do_transmit_packets(dp8393xState *s)
                                MEMTXATTRS_UNSPECIFIED, s->data,
                                size);
             s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-            if (dp8393x_get(s, width, 0) & 0x1) {
+            if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
             }
@@ -765,13 +770,13 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* XXX: Check byte ordering */
 
     /* Check for EOL */
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* Are we still in resource exhaustion? */
         size = sizeof(uint16_t) * 1 * width;
         address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
         address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
                            s->data, size);
-        if (dp8393x_get(s, width, 0) & 0x1) {
+        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
             /* Still EOL ; stop reception */
             return -1;
         } else {
@@ -831,7 +836,7 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
                        dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
                        MEMTXATTRS_UNSPECIFIED, s->data, size);
     s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {