[PATCH 0/2] lan9118: Fix RX Status FIFO PEEK value

[PATCH 0/2] lan9118: Fix RX Status FIFO PEEK value

[Peter Maydell]

This patchset fixes https://bugs.launchpad.net/bugs/1904954 :
we give a bogus value for the RX Status FIFO peek register,
because of a copy-and-paste error. (This bug has been present since
2009 when the device model was first added.)

Patch 1 fixes the bug; patch 2 does a little bit of tidyup while
I was looking at this bit of the code.

thanks
-- PMM

Peter Maydell (2):
  hw/net/lan9118: Fix RX Status FIFO PEEK value
  hw/net/lan9118: Add symbolic constants for register offsets

 hw/net/lan9118.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/net/lan9118: Fix RX Status FIFO PEEK value

[Peter Maydell]

A copy-and-paste error meant that the return value for register offset 0x44
(the RX Status FIFO PEEK register) returned a byte from a bogus offset in
the rx status FIFO. Fix the typo.

Cc: qemu-stable@nongnu.org
Fixes: https://bugs.launchpad.net/qemu/+bug/1904954
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/lan9118.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index ab57c02c8e1..13d469fe24f 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -1206,7 +1206,7 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
     case 0x40:
         return rx_status_fifo_pop(s);
     case 0x44:
-        return s->rx_status_fifo[s->tx_status_fifo_head];
+        return s->rx_status_fifo[s->rx_status_fifo_head];
     case 0x48:
         return tx_status_fifo_pop(s);
     case 0x4c:
-- 
2.20.1

[PATCH 2/2] hw/net/lan9118: Add symbolic constants for register offsets

[Peter Maydell]

The lan9118 code mostly uses symbolic constants for register offsets;
the exceptions are those which the datasheet doesn't give an official
symbolic name to.

Add some names for the registers which don't already have them, based
on the longer names they are given in the memory map.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/lan9118.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 13d469fe24f..abc796285ab 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -40,6 +40,17 @@ do { hw_error("lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
 do { fprintf(stderr, "lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
 #endif
 
+/* The tx and rx fifo ports are a range of aliased 32-bit registers */
+#define RX_DATA_FIFO_PORT_FIRST 0x00
+#define RX_DATA_FIFO_PORT_LAST 0x1f
+#define TX_DATA_FIFO_PORT_FIRST 0x20
+#define TX_DATA_FIFO_PORT_LAST 0x3f
+
+#define RX_STATUS_FIFO_PORT 0x40
+#define RX_STATUS_FIFO_PEEK 0x44
+#define TX_STATUS_FIFO_PORT 0x48
+#define TX_STATUS_FIFO_PEEK 0x4c
+
 #define CSR_ID_REV      0x50
 #define CSR_IRQ_CFG     0x54
 #define CSR_INT_STS     0x58
@@ -1020,7 +1031,8 @@ static void lan9118_writel(void *opaque, hwaddr offset,
     offset &= 0xff;
 
     //DPRINTF("Write reg 0x%02x = 0x%08x\n", (int)offset, val);
-    if (offset >= 0x20 && offset < 0x40) {
+    if (offset >= TX_DATA_FIFO_PORT_FIRST &&
+        offset <= TX_DATA_FIFO_PORT_LAST) {
         /* TX FIFO */
         tx_fifo_push(s, val);
         return;
@@ -1198,18 +1210,18 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
     lan9118_state *s = (lan9118_state *)opaque;
 
     //DPRINTF("Read reg 0x%02x\n", (int)offset);
-    if (offset < 0x20) {
+    if (offset <= RX_DATA_FIFO_PORT_LAST) {
         /* RX FIFO */
         return rx_fifo_pop(s);
     }
     switch (offset) {
-    case 0x40:
+    case RX_STATUS_FIFO_PORT:
         return rx_status_fifo_pop(s);
-    case 0x44:
+    case RX_STATUS_FIFO_PEEK:
         return s->rx_status_fifo[s->rx_status_fifo_head];
-    case 0x48:
+    case TX_STATUS_FIFO_PORT:
         return tx_status_fifo_pop(s);
-    case 0x4c:
+    case TX_STATUS_FIFO_PEEK:
         return s->tx_status_fifo[s->tx_status_fifo_head];
     case CSR_ID_REV:
         return 0x01180001;
-- 
2.20.1

Re: [PATCH 1/2] hw/net/lan9118: Fix RX Status FIFO PEEK value

[Philippe Mathieu-Daudé]

On 1/8/21 7:04 PM, Peter Maydell wrote:
> A copy-and-paste error meant that the return value for register offset 0x44
> (the RX Status FIFO PEEK register) returned a byte from a bogus offset in
> the rx status FIFO. Fix the typo.

Wow, nice catch :)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Cc: qemu-stable@nongnu.org
> Fixes: https://bugs.launchpad.net/qemu/+bug/1904954
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/lan9118.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index ab57c02c8e1..13d469fe24f 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -1206,7 +1206,7 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
>      case 0x40:
>          return rx_status_fifo_pop(s);
>      case 0x44:
> -        return s->rx_status_fifo[s->tx_status_fifo_head];
> +        return s->rx_status_fifo[s->rx_status_fifo_head];
>      case 0x48:
>          return tx_status_fifo_pop(s);
>      case 0x4c:
> 

Re: [PATCH 2/2] hw/net/lan9118: Add symbolic constants for register offsets

[Philippe Mathieu-Daudé]

On 1/8/21 7:04 PM, Peter Maydell wrote:
> The lan9118 code mostly uses symbolic constants for register offsets;
> the exceptions are those which the datasheet doesn't give an official
> symbolic name to.
> 
> Add some names for the registers which don't already have them, based
> on the longer names they are given in the memory map.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/lan9118.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>