1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] etsec: fix IRQ (un)masking

Michael Davidsaver posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/ec95e71d-3d96-489d-6939-cdbef4d8c34e@gmail.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
hw/net/fsl_etsec/etsec.h     |  2 ++
hw/net/fsl_etsec/registers.h | 10 +++++++
hw/net/fsl_etsec/rings.c     | 12 +-------
4 files changed, 49 insertions(+), 43 deletions(-)
[Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
Posted by Michael Davidsaver 6 years, 4 months ago 
Interrupt conditions occurring while masked are not being
signaled when later unmasked.
The fix is to raise/lower IRQs when IMASK is changed.

To avoid problems like this in future, consolidate
IRQ pin update logic in one function.

Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
and update IRQ pins on reset.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
 hw/net/fsl_etsec/etsec.h     |  2 ++
 hw/net/fsl_etsec/registers.h | 10 +++++++
 hw/net/fsl_etsec/rings.c     | 12 +-------
 4 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 9da1932970..0b66274ce3 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -49,6 +49,28 @@ static const int debug_etsec;
     }                                          \
     } while (0)
 
+/* call after any change to IEVENT or IMASK */
+void etsec_update_irq(eTSEC *etsec)
+{
+    uint32_t ievent = etsec->regs[IEVENT].value;
+    uint32_t imask  = etsec->regs[IMASK].value;
+    uint32_t active = ievent & imask;
+
+    int tx  = !!(active & IEVENT_TX_MASK);
+    int rx  = !!(active & IEVENT_RX_MASK);
+    int err = !!(active & IEVENT_ERR_MASK);
+
+    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
+            __func__, ievent, imask,
+            tx  ? 'T' : '_',
+            rx  ? 'R' : '_',
+            err ? 'E' : '_');
+
+    qemu_set_irq(etsec->tx_irq, tx);
+    qemu_set_irq(etsec->rx_irq, rx);
+    qemu_set_irq(etsec->err_irq, err);
+}
+
 static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
 {
     eTSEC          *etsec     = opaque;
@@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
     etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
 }
 
-static void write_ievent(eTSEC          *etsec,
-                         eTSEC_Register *reg,
-                         uint32_t        reg_index,
-                         uint32_t        value)
-{
-    /* Write 1 to clear */
-    reg->value &= ~value;
-
-    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
-        qemu_irq_lower(etsec->tx_irq);
-    }
-    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
-        qemu_irq_lower(etsec->rx_irq);
-    }
-
-    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
-                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
-                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
-                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
-                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
-                        IEVENT_MMRW))) {
-        qemu_irq_lower(etsec->err_irq);
-    }
-}
-
 static void write_dmactrl(eTSEC          *etsec,
                           eTSEC_Register *reg,
                           uint32_t        reg_index,
@@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
         } else {
             /* Graceful receive stop now */
             etsec->regs[IEVENT].value |= IEVENT_GRSC;
-            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
-                qemu_irq_raise(etsec->err_irq);
-            }
+            etsec_update_irq(etsec);
         }
     }
 
@@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
         } else {
             /* Graceful transmit stop now */
             etsec->regs[IEVENT].value |= IEVENT_GTSC;
-            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
-                qemu_irq_raise(etsec->err_irq);
-            }
+            etsec_update_irq(etsec);
         }
     }
 
@@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
 
     switch (reg_index) {
     case IEVENT:
-        write_ievent(etsec, reg, reg_index, value);
+        /* Write 1 to clear */
+        reg->value &= ~value;
+
+        etsec_update_irq(etsec);
+        break;
+
+    case IMASK:
+        reg->value = value;
+
+        etsec_update_irq(etsec);
         break;
 
     case DMACTRL:
@@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
         MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
         MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
         MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
+
+    etsec_update_irq(etsec);
 }
 
 static ssize_t etsec_receive(NetClientState *nc,
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 30c828e241..877988572e 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
                           qemu_irq      rx_irq,
                           qemu_irq      err_irq);
 
+void etsec_update_irq(eTSEC *etsec);
+
 void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
 void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
 ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
index c4ed2b9d62..f085537ecd 100644
--- a/hw/net/fsl_etsec/registers.h
+++ b/hw/net/fsl_etsec/registers.h
@@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
 #define IEVENT_RXC   (1 << 30)
 #define IEVENT_BABR  (1 << 31)
 
+/* Mapping between interrupt pin and interrupt flags */
+#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
+#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
+#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
+    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
+    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
+    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
+    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
+    IEVENT_MMRW)
+
 #define IMASK_RXFEN  (1 <<  7)
 #define IMASK_GRSCEN (1 <<  8)
 #define IMASK_RXBEN  (1 << 15)
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d0f93eebfc..337a55fc95 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
 {
     etsec->regs[IEVENT].value |= flags;
 
-    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
-        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
-        qemu_irq_raise(etsec->tx_irq);
-        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
-    }
-
-    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
-        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
-        qemu_irq_raise(etsec->rx_irq);
-        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
-    }
+    etsec_update_irq(etsec);
 }
 
 static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
-- 
2.11.0
Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
Posted by David Gibson 6 years, 4 months ago 
On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
> 
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.
> 
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

Looks sane, but I would like to get an R-b from someone who knows FSL
better than me before applying.

> ---
>  hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
>  hw/net/fsl_etsec/etsec.h     |  2 ++
>  hw/net/fsl_etsec/registers.h | 10 +++++++
>  hw/net/fsl_etsec/rings.c     | 12 +-------
>  4 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
>      }                                          \
>      } while (0)
>  
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> +    uint32_t ievent = etsec->regs[IEVENT].value;
> +    uint32_t imask  = etsec->regs[IMASK].value;
> +    uint32_t active = ievent & imask;
> +
> +    int tx  = !!(active & IEVENT_TX_MASK);
> +    int rx  = !!(active & IEVENT_RX_MASK);
> +    int err = !!(active & IEVENT_ERR_MASK);
> +
> +    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> +            __func__, ievent, imask,
> +            tx  ? 'T' : '_',
> +            rx  ? 'R' : '_',
> +            err ? 'E' : '_');
> +
> +    qemu_set_irq(etsec->tx_irq, tx);
> +    qemu_set_irq(etsec->rx_irq, rx);
> +    qemu_set_irq(etsec->err_irq, err);
> +}
> +
>  static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      eTSEC          *etsec     = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
>      etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
>  }
>  
> -static void write_ievent(eTSEC          *etsec,
> -                         eTSEC_Register *reg,
> -                         uint32_t        reg_index,
> -                         uint32_t        value)
> -{
> -    /* Write 1 to clear */
> -    reg->value &= ~value;
> -
> -    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> -        qemu_irq_lower(etsec->tx_irq);
> -    }
> -    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> -        qemu_irq_lower(etsec->rx_irq);
> -    }
> -
> -    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> -                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> -                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> -                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> -                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> -                        IEVENT_MMRW))) {
> -        qemu_irq_lower(etsec->err_irq);
> -    }
> -}
> -
>  static void write_dmactrl(eTSEC          *etsec,
>                            eTSEC_Register *reg,
>                            uint32_t        reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful receive stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GRSC;
> -            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful transmit stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GTSC;
> -            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
>  
>      switch (reg_index) {
>      case IEVENT:
> -        write_ievent(etsec, reg, reg_index, value);
> +        /* Write 1 to clear */
> +        reg->value &= ~value;
> +
> +        etsec_update_irq(etsec);
> +        break;
> +
> +    case IMASK:
> +        reg->value = value;
> +
> +        etsec_update_irq(etsec);
>          break;
>  
>      case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
>          MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
>          MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
> +
> +    etsec_update_irq(etsec);
>  }
>  
>  static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
>                            qemu_irq      rx_irq,
>                            qemu_irq      err_irq);
>  
> +void etsec_update_irq(eTSEC *etsec);
> +
>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
>  ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
>  #define IEVENT_RXC   (1 << 30)
>  #define IEVENT_BABR  (1 << 31)
>  
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> +    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> +    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> +    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> +    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> +    IEVENT_MMRW)
> +
>  #define IMASK_RXFEN  (1 <<  7)
>  #define IMASK_GRSCEN (1 <<  8)
>  #define IMASK_RXBEN  (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
>  {
>      etsec->regs[IEVENT].value |= flags;
>  
> -    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> -        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> -        qemu_irq_raise(etsec->tx_irq);
> -        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> -    }
> -
> -    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> -        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> -        qemu_irq_raise(etsec->rx_irq);
> -        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> -    }
> +    etsec_update_irq(etsec);
>  }
>  
>  static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
Posted by Michael Davidsaver 5 years, 9 months ago 
On 11/28/2017 05:35 PM, David Gibson wrote:
> On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
>> Interrupt conditions occurring while masked are not being
>> signaled when later unmasked.
>> The fix is to raise/lower IRQs when IMASK is changed.
>>
>> To avoid problems like this in future, consolidate
>> IRQ pin update logic in one function.
>>
>> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
>> and update IRQ pins on reset.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> 
> Looks sane, but I would like to get an R-b from someone who knows FSL
> better than me before applying.

Any progress on finding a candidate to comment?


>> ---
>>  hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
>>  hw/net/fsl_etsec/etsec.h     |  2 ++
>>  hw/net/fsl_etsec/registers.h | 10 +++++++
>>  hw/net/fsl_etsec/rings.c     | 12 +-------
>>  4 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
>> index 9da1932970..0b66274ce3 100644
>> --- a/hw/net/fsl_etsec/etsec.c
>> +++ b/hw/net/fsl_etsec/etsec.c
>> @@ -49,6 +49,28 @@ static const int debug_etsec;
>>      }                                          \
>>      } while (0)
>>  
>> +/* call after any change to IEVENT or IMASK */
>> +void etsec_update_irq(eTSEC *etsec)
>> +{
>> +    uint32_t ievent = etsec->regs[IEVENT].value;
>> +    uint32_t imask  = etsec->regs[IMASK].value;
>> +    uint32_t active = ievent & imask;
>> +
>> +    int tx  = !!(active & IEVENT_TX_MASK);
>> +    int rx  = !!(active & IEVENT_RX_MASK);
>> +    int err = !!(active & IEVENT_ERR_MASK);
>> +
>> +    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
>> +            __func__, ievent, imask,
>> +            tx  ? 'T' : '_',
>> +            rx  ? 'R' : '_',
>> +            err ? 'E' : '_');
>> +
>> +    qemu_set_irq(etsec->tx_irq, tx);
>> +    qemu_set_irq(etsec->rx_irq, rx);
>> +    qemu_set_irq(etsec->err_irq, err);
>> +}
>> +
>>  static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
>>  {
>>      eTSEC          *etsec     = opaque;
>> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
>>      etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
>>  }
>>  
>> -static void write_ievent(eTSEC          *etsec,
>> -                         eTSEC_Register *reg,
>> -                         uint32_t        reg_index,
>> -                         uint32_t        value)
>> -{
>> -    /* Write 1 to clear */
>> -    reg->value &= ~value;
>> -
>> -    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
>> -        qemu_irq_lower(etsec->tx_irq);
>> -    }
>> -    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
>> -        qemu_irq_lower(etsec->rx_irq);
>> -    }
>> -
>> -    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
>> -                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
>> -                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
>> -                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
>> -                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
>> -                        IEVENT_MMRW))) {
>> -        qemu_irq_lower(etsec->err_irq);
>> -    }
>> -}
>> -
>>  static void write_dmactrl(eTSEC          *etsec,
>>                            eTSEC_Register *reg,
>>                            uint32_t        reg_index,
>> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
>>          } else {
>>              /* Graceful receive stop now */
>>              etsec->regs[IEVENT].value |= IEVENT_GRSC;
>> -            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
>> -                qemu_irq_raise(etsec->err_irq);
>> -            }
>> +            etsec_update_irq(etsec);
>>          }
>>      }
>>  
>> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
>>          } else {
>>              /* Graceful transmit stop now */
>>              etsec->regs[IEVENT].value |= IEVENT_GTSC;
>> -            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
>> -                qemu_irq_raise(etsec->err_irq);
>> -            }
>> +            etsec_update_irq(etsec);
>>          }
>>      }
>>  
>> @@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
>>  
>>      switch (reg_index) {
>>      case IEVENT:
>> -        write_ievent(etsec, reg, reg_index, value);
>> +        /* Write 1 to clear */
>> +        reg->value &= ~value;
>> +
>> +        etsec_update_irq(etsec);
>> +        break;
>> +
>> +    case IMASK:
>> +        reg->value = value;
>> +
>> +        etsec_update_irq(etsec);
>>          break;
>>  
>>      case DMACTRL:
>> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
>>          MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
>>          MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
>>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
>> +
>> +    etsec_update_irq(etsec);
>>  }
>>  
>>  static ssize_t etsec_receive(NetClientState *nc,
>> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
>> index 30c828e241..877988572e 100644
>> --- a/hw/net/fsl_etsec/etsec.h
>> +++ b/hw/net/fsl_etsec/etsec.h
>> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
>>                            qemu_irq      rx_irq,
>>                            qemu_irq      err_irq);
>>  
>> +void etsec_update_irq(eTSEC *etsec);
>> +
>>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
>>  ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
>> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
>> index c4ed2b9d62..f085537ecd 100644
>> --- a/hw/net/fsl_etsec/registers.h
>> +++ b/hw/net/fsl_etsec/registers.h
>> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
>>  #define IEVENT_RXC   (1 << 30)
>>  #define IEVENT_BABR  (1 << 31)
>>  
>> +/* Mapping between interrupt pin and interrupt flags */
>> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
>> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
>> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
>> +    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
>> +    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
>> +    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
>> +    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
>> +    IEVENT_MMRW)
>> +
>>  #define IMASK_RXFEN  (1 <<  7)
>>  #define IMASK_GRSCEN (1 <<  8)
>>  #define IMASK_RXBEN  (1 << 15)
>> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
>> index d0f93eebfc..337a55fc95 100644
>> --- a/hw/net/fsl_etsec/rings.c
>> +++ b/hw/net/fsl_etsec/rings.c
>> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
>>  {
>>      etsec->regs[IEVENT].value |= flags;
>>  
>> -    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
>> -        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
>> -        qemu_irq_raise(etsec->tx_irq);
>> -        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
>> -    }
>> -
>> -    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
>> -        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
>> -        qemu_irq_raise(etsec->rx_irq);
>> -        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
>> -    }
>> +    etsec_update_irq(etsec);
>>  }
>>  
>>  static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
>
Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
Posted by David Gibson 5 years, 9 months ago 
On Sat, Jul 07, 2018 at 11:06:05AM -0700, Michael Davidsaver wrote:
> On 11/28/2017 05:35 PM, David Gibson wrote:
> > On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
> >> Interrupt conditions occurring while masked are not being
> >> signaled when later unmasked.
> >> The fix is to raise/lower IRQs when IMASK is changed.
> >>
> >> To avoid problems like this in future, consolidate
> >> IRQ pin update logic in one function.
> >>
> >> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> >> and update IRQ pins on reset.
> >>
> >> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> > 
> > Looks sane, but I would like to get an R-b from someone who knows FSL
> > better than me before applying.
> 
> Any progress on finding a candidate to comment?

I'm afraid not, I was kind of hoping someone would respond on list.

> 
> 
> >> ---
> >>  hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
> >>  hw/net/fsl_etsec/etsec.h     |  2 ++
> >>  hw/net/fsl_etsec/registers.h | 10 +++++++
> >>  hw/net/fsl_etsec/rings.c     | 12 +-------
> >>  4 files changed, 49 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> >> index 9da1932970..0b66274ce3 100644
> >> --- a/hw/net/fsl_etsec/etsec.c
> >> +++ b/hw/net/fsl_etsec/etsec.c
> >> @@ -49,6 +49,28 @@ static const int debug_etsec;
> >>      }                                          \
> >>      } while (0)
> >>  
> >> +/* call after any change to IEVENT or IMASK */
> >> +void etsec_update_irq(eTSEC *etsec)
> >> +{
> >> +    uint32_t ievent = etsec->regs[IEVENT].value;
> >> +    uint32_t imask  = etsec->regs[IMASK].value;
> >> +    uint32_t active = ievent & imask;
> >> +
> >> +    int tx  = !!(active & IEVENT_TX_MASK);
> >> +    int rx  = !!(active & IEVENT_RX_MASK);
> >> +    int err = !!(active & IEVENT_ERR_MASK);
> >> +
> >> +    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> >> +            __func__, ievent, imask,
> >> +            tx  ? 'T' : '_',
> >> +            rx  ? 'R' : '_',
> >> +            err ? 'E' : '_');
> >> +
> >> +    qemu_set_irq(etsec->tx_irq, tx);
> >> +    qemu_set_irq(etsec->rx_irq, rx);
> >> +    qemu_set_irq(etsec->err_irq, err);
> >> +}
> >> +
> >>  static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> >>  {
> >>      eTSEC          *etsec     = opaque;
> >> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
> >>      etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
> >>  }
> >>  
> >> -static void write_ievent(eTSEC          *etsec,
> >> -                         eTSEC_Register *reg,
> >> -                         uint32_t        reg_index,
> >> -                         uint32_t        value)
> >> -{
> >> -    /* Write 1 to clear */
> >> -    reg->value &= ~value;
> >> -
> >> -    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> >> -        qemu_irq_lower(etsec->tx_irq);
> >> -    }
> >> -    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> >> -        qemu_irq_lower(etsec->rx_irq);
> >> -    }
> >> -
> >> -    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> >> -                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> >> -                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> >> -                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> >> -                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> >> -                        IEVENT_MMRW))) {
> >> -        qemu_irq_lower(etsec->err_irq);
> >> -    }
> >> -}
> >> -
> >>  static void write_dmactrl(eTSEC          *etsec,
> >>                            eTSEC_Register *reg,
> >>                            uint32_t        reg_index,
> >> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
> >>          } else {
> >>              /* Graceful receive stop now */
> >>              etsec->regs[IEVENT].value |= IEVENT_GRSC;
> >> -            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> >> -                qemu_irq_raise(etsec->err_irq);
> >> -            }
> >> +            etsec_update_irq(etsec);
> >>          }
> >>      }
> >>  
> >> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
> >>          } else {
> >>              /* Graceful transmit stop now */
> >>              etsec->regs[IEVENT].value |= IEVENT_GTSC;
> >> -            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> >> -                qemu_irq_raise(etsec->err_irq);
> >> -            }
> >> +            etsec_update_irq(etsec);
> >>          }
> >>      }
> >>  
> >> @@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
> >>  
> >>      switch (reg_index) {
> >>      case IEVENT:
> >> -        write_ievent(etsec, reg, reg_index, value);
> >> +        /* Write 1 to clear */
> >> +        reg->value &= ~value;
> >> +
> >> +        etsec_update_irq(etsec);
> >> +        break;
> >> +
> >> +    case IMASK:
> >> +        reg->value = value;
> >> +
> >> +        etsec_update_irq(etsec);
> >>          break;
> >>  
> >>      case DMACTRL:
> >> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
> >>          MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
> >>          MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
> >>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
> >> +
> >> +    etsec_update_irq(etsec);
> >>  }
> >>  
> >>  static ssize_t etsec_receive(NetClientState *nc,
> >> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> >> index 30c828e241..877988572e 100644
> >> --- a/hw/net/fsl_etsec/etsec.h
> >> +++ b/hw/net/fsl_etsec/etsec.h
> >> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
> >>                            qemu_irq      rx_irq,
> >>                            qemu_irq      err_irq);
> >>  
> >> +void etsec_update_irq(eTSEC *etsec);
> >> +
> >>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> >>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> >>  ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> >> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> >> index c4ed2b9d62..f085537ecd 100644
> >> --- a/hw/net/fsl_etsec/registers.h
> >> +++ b/hw/net/fsl_etsec/registers.h
> >> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
> >>  #define IEVENT_RXC   (1 << 30)
> >>  #define IEVENT_BABR  (1 << 31)
> >>  
> >> +/* Mapping between interrupt pin and interrupt flags */
> >> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> >> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> >> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> >> +    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> >> +    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> >> +    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> >> +    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> >> +    IEVENT_MMRW)
> >> +
> >>  #define IMASK_RXFEN  (1 <<  7)
> >>  #define IMASK_GRSCEN (1 <<  8)
> >>  #define IMASK_RXBEN  (1 << 15)
> >> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> >> index d0f93eebfc..337a55fc95 100644
> >> --- a/hw/net/fsl_etsec/rings.c
> >> +++ b/hw/net/fsl_etsec/rings.c
> >> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
> >>  {
> >>      etsec->regs[IEVENT].value |= flags;
> >>  
> >> -    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> >> -        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> >> -        qemu_irq_raise(etsec->tx_irq);
> >> -        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> >> -    }
> >> -
> >> -    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> >> -        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> >> -        qemu_irq_raise(etsec->rx_irq);
> >> -        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> >> -    }
> >> +    etsec_update_irq(etsec);
> >>  }
> >>  
> >>  static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
> > 
> 
> 




-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [Qemu-ppc] [PATCH] etsec: fix IRQ (un)masking
Posted by Cédric Le Goater 5 years, 9 months ago 
On 11/28/2017 03:42 AM, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
> 
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.

it makes sense.
 
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
>  hw/net/fsl_etsec/etsec.h     |  2 ++
>  hw/net/fsl_etsec/registers.h | 10 +++++++
>  hw/net/fsl_etsec/rings.c     | 12 +-------
>  4 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
>      }                                          \
>      } while (0)
>  
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> +    uint32_t ievent = etsec->regs[IEVENT].value;
> +    uint32_t imask  = etsec->regs[IMASK].value;
> +    uint32_t active = ievent & imask;
> +
> +    int tx  = !!(active & IEVENT_TX_MASK);
> +    int rx  = !!(active & IEVENT_RX_MASK);
> +    int err = !!(active & IEVENT_ERR_MASK);

you could make these bools. this is minor. 

> +    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> +            __func__, ievent, imask,
> +            tx  ? 'T' : '_',
> +            rx  ? 'R' : '_',
> +            err ? 'E' : '_');
> +
> +    qemu_set_irq(etsec->tx_irq, tx);
> +    qemu_set_irq(etsec->rx_irq, rx);
> +    qemu_set_irq(etsec->err_irq, err);
> +}
> +
>  static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      eTSEC          *etsec     = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
>      etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
>  }
>  
> -static void write_ievent(eTSEC          *etsec,
> -                         eTSEC_Register *reg,
> -                         uint32_t        reg_index,
> -                         uint32_t        value)
> -{
> -    /* Write 1 to clear */
> -    reg->value &= ~value;
> -
> -    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> -        qemu_irq_lower(etsec->tx_irq);
> -    }
> -    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> -        qemu_irq_lower(etsec->rx_irq);
> -    }
> -
> -    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> -                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> -                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> -                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> -                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> -                        IEVENT_MMRW))) {
> -        qemu_irq_lower(etsec->err_irq);
> -    }
> -}
> -
>  static void write_dmactrl(eTSEC          *etsec,
>                            eTSEC_Register *reg,
>                            uint32_t        reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful receive stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GRSC;
> -            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful transmit stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GTSC;
> -            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
>  
>      switch (reg_index) {
>      case IEVENT:
> -        write_ievent(etsec, reg, reg_index, value);
> +        /* Write 1 to clear */
> +        reg->value &= ~value;
> +
> +        etsec_update_irq(etsec);
> +        break;
> +
> +    case IMASK:
> +        reg->value = value;
> +
> +        etsec_update_irq(etsec);
>          break;
>  
>      case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
>          MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
>          MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
> +
> +    etsec_update_irq(etsec);>
>  }
>  
>  static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
>                            qemu_irq      rx_irq,
>                            qemu_irq      err_irq);
>  
> +void etsec_update_irq(eTSEC *etsec);
> +
>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
>  ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
>  #define IEVENT_RXC   (1 << 30)
>  #define IEVENT_BABR  (1 << 31)
>  
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> +    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> +    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> +    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> +    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> +    IEVENT_MMRW)
> +
>  #define IMASK_RXFEN  (1 <<  7)
>  #define IMASK_GRSCEN (1 <<  8)
>  #define IMASK_RXBEN  (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
>  {
>      etsec->regs[IEVENT].value |= flags;
>  
> -    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> -        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> -        qemu_irq_raise(etsec->tx_irq);
> -        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> -    }
> -
> -    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> -        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> -        qemu_irq_raise(etsec->rx_irq);
> -        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> -    }
> +    etsec_update_irq(etsec);
>  }
>  
>  static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
>
Re: [Qemu-devel] [Qemu-ppc] [PATCH] etsec: fix IRQ (un)masking
Posted by David Gibson 5 years, 9 months ago 
On Mon, Jul 09, 2018 at 08:20:03AM +0200, Cédric Le Goater wrote:
11;rgb:ffff/ffff/ffff> On 11/28/2017 03:42 AM, Michael Davidsaver wrote:
> > Interrupt conditions occurring while masked are not being
> > signaled when later unmasked.
> > The fix is to raise/lower IRQs when IMASK is changed.
> > 
> > To avoid problems like this in future, consolidate
> > IRQ pin update logic in one function.
> 
> it makes sense.
>  
> > Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> > and update IRQ pins on reset.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks for the review Cédric.  Unfortunately, I've mislaid the
original patch.

Michael, can you resend with Cédric's R-b included, please.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
[Qemu-devel] [PATCH 1/1] etsec: fix IRQ (un)masking
Posted by Michael Davidsaver 5 years, 9 months ago 
Interrupt conditions occurring while masked are not being
signaled when later unmasked.
The fix is to raise/lower IRQs when IMASK is changed.

To avoid problems like this in future, consolidate
IRQ pin update logic in one function.

Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
and update IRQ pins on reset.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
 hw/net/fsl_etsec/etsec.h     |  2 ++
 hw/net/fsl_etsec/registers.h | 10 +++++++
 hw/net/fsl_etsec/rings.c     | 12 +-------
 4 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 9da1932970..0b66274ce3 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -49,6 +49,28 @@ static const int debug_etsec;
     }                                          \
     } while (0)
 
+/* call after any change to IEVENT or IMASK */
+void etsec_update_irq(eTSEC *etsec)
+{
+    uint32_t ievent = etsec->regs[IEVENT].value;
+    uint32_t imask  = etsec->regs[IMASK].value;
+    uint32_t active = ievent & imask;
+
+    int tx  = !!(active & IEVENT_TX_MASK);
+    int rx  = !!(active & IEVENT_RX_MASK);
+    int err = !!(active & IEVENT_ERR_MASK);
+
+    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c",
+            __func__, ievent, imask,
+            tx  ? 'T' : '_',
+            rx  ? 'R' : '_',
+            err ? 'E' : '_');
+
+    qemu_set_irq(etsec->tx_irq, tx);
+    qemu_set_irq(etsec->rx_irq, rx);
+    qemu_set_irq(etsec->err_irq, err);
+}
+
 static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
 {
     eTSEC          *etsec     = opaque;
@@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
     etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
 }
 
-static void write_ievent(eTSEC          *etsec,
-                         eTSEC_Register *reg,
-                         uint32_t        reg_index,
-                         uint32_t        value)
-{
-    /* Write 1 to clear */
-    reg->value &= ~value;
-
-    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
-        qemu_irq_lower(etsec->tx_irq);
-    }
-    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
-        qemu_irq_lower(etsec->rx_irq);
-    }
-
-    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
-                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
-                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
-                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
-                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
-                        IEVENT_MMRW))) {
-        qemu_irq_lower(etsec->err_irq);
-    }
-}
-
 static void write_dmactrl(eTSEC          *etsec,
                           eTSEC_Register *reg,
                           uint32_t        reg_index,
@@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
         } else {
             /* Graceful receive stop now */
             etsec->regs[IEVENT].value |= IEVENT_GRSC;
-            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
-                qemu_irq_raise(etsec->err_irq);
-            }
+            etsec_update_irq(etsec);
         }
     }
 
@@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
         } else {
             /* Graceful transmit stop now */
             etsec->regs[IEVENT].value |= IEVENT_GTSC;
-            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
-                qemu_irq_raise(etsec->err_irq);
-            }
+            etsec_update_irq(etsec);
         }
     }
 
@@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
 
     switch (reg_index) {
     case IEVENT:
-        write_ievent(etsec, reg, reg_index, value);
+        /* Write 1 to clear */
+        reg->value &= ~value;
+
+        etsec_update_irq(etsec);
+        break;
+
+    case IMASK:
+        reg->value = value;
+
+        etsec_update_irq(etsec);
         break;
 
     case DMACTRL:
@@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
         MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
         MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
         MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
+
+    etsec_update_irq(etsec);
 }
 
 static ssize_t etsec_receive(NetClientState *nc,
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 30c828e241..877988572e 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
                           qemu_irq      rx_irq,
                           qemu_irq      err_irq);
 
+void etsec_update_irq(eTSEC *etsec);
+
 void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
 void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
 ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
index c4ed2b9d62..f085537ecd 100644
--- a/hw/net/fsl_etsec/registers.h
+++ b/hw/net/fsl_etsec/registers.h
@@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
 #define IEVENT_RXC   (1 << 30)
 #define IEVENT_BABR  (1 << 31)
 
+/* Mapping between interrupt pin and interrupt flags */
+#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
+#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
+#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
+    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
+    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
+    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
+    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
+    IEVENT_MMRW)
+
 #define IMASK_RXFEN  (1 <<  7)
 #define IMASK_GRSCEN (1 <<  8)
 #define IMASK_RXBEN  (1 << 15)
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d0f93eebfc..337a55fc95 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
 {
     etsec->regs[IEVENT].value |= flags;
 
-    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
-        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
-        qemu_irq_raise(etsec->tx_irq);
-        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
-    }
-
-    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
-        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
-        qemu_irq_raise(etsec->rx_irq);
-        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
-    }
+    etsec_update_irq(etsec);
 }
 
 static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
-- 
2.11.0
Re: [Qemu-devel] [PATCH 1/1] etsec: fix IRQ (un)masking
Posted by David Gibson 5 years, 9 months ago 
On Thu, Jul 12, 2018 at 02:00:52PM -0700, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
> 
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.
> 
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-3.0.

> ---
>  hw/net/fsl_etsec/etsec.c     | 68 +++++++++++++++++++++++---------------------
>  hw/net/fsl_etsec/etsec.h     |  2 ++
>  hw/net/fsl_etsec/registers.h | 10 +++++++
>  hw/net/fsl_etsec/rings.c     | 12 +-------
>  4 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
>      }                                          \
>      } while (0)
>  
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> +    uint32_t ievent = etsec->regs[IEVENT].value;
> +    uint32_t imask  = etsec->regs[IMASK].value;
> +    uint32_t active = ievent & imask;
> +
> +    int tx  = !!(active & IEVENT_TX_MASK);
> +    int rx  = !!(active & IEVENT_RX_MASK);
> +    int err = !!(active & IEVENT_ERR_MASK);
> +
> +    DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c",
> +            __func__, ievent, imask,
> +            tx  ? 'T' : '_',
> +            rx  ? 'R' : '_',
> +            err ? 'E' : '_');
> +
> +    qemu_set_irq(etsec->tx_irq, tx);
> +    qemu_set_irq(etsec->rx_irq, rx);
> +    qemu_set_irq(etsec->err_irq, err);
> +}
> +
>  static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      eTSEC          *etsec     = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC          *etsec,
>      etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
>  }
>  
> -static void write_ievent(eTSEC          *etsec,
> -                         eTSEC_Register *reg,
> -                         uint32_t        reg_index,
> -                         uint32_t        value)
> -{
> -    /* Write 1 to clear */
> -    reg->value &= ~value;
> -
> -    if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> -        qemu_irq_lower(etsec->tx_irq);
> -    }
> -    if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> -        qemu_irq_lower(etsec->rx_irq);
> -    }
> -
> -    if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> -                        IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> -                        IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> -                        IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> -                        IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> -                        IEVENT_MMRW))) {
> -        qemu_irq_lower(etsec->err_irq);
> -    }
> -}
> -
>  static void write_dmactrl(eTSEC          *etsec,
>                            eTSEC_Register *reg,
>                            uint32_t        reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful receive stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GRSC;
> -            if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC          *etsec,
>          } else {
>              /* Graceful transmit stop now */
>              etsec->regs[IEVENT].value |= IEVENT_GTSC;
> -            if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> -                qemu_irq_raise(etsec->err_irq);
> -            }
> +            etsec_update_irq(etsec);
>          }
>      }
>  
> @@ -222,7 +215,16 @@ static void etsec_write(void     *opaque,
>  
>      switch (reg_index) {
>      case IEVENT:
> -        write_ievent(etsec, reg, reg_index, value);
> +        /* Write 1 to clear */
> +        reg->value &= ~value;
> +
> +        etsec_update_irq(etsec);
> +        break;
> +
> +    case IMASK:
> +        reg->value = value;
> +
> +        etsec_update_irq(etsec);
>          break;
>  
>      case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
>          MII_SR_EXTENDED_STATUS  | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
>          MII_SR_10T_HD_CAPS      | MII_SR_10T_FD_CAPS   | MII_SR_100X_HD_CAPS  |
>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
> +
> +    etsec_update_irq(etsec);
>  }
>  
>  static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr        base,
>                            qemu_irq      rx_irq,
>                            qemu_irq      err_irq);
>  
> +void etsec_update_irq(eTSEC *etsec);
> +
>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
>  ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
>  #define IEVENT_RXC   (1 << 30)
>  #define IEVENT_BABR  (1 << 31)
>  
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> +    IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> +    IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> +    IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> +    IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> +    IEVENT_MMRW)
> +
>  #define IMASK_RXFEN  (1 <<  7)
>  #define IMASK_GRSCEN (1 <<  8)
>  #define IMASK_RXBEN  (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC    *etsec,
>  {
>      etsec->regs[IEVENT].value |= flags;
>  
> -    if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> -        || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> -        qemu_irq_raise(etsec->tx_irq);
> -        RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> -    }
> -
> -    if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> -        || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> -        qemu_irq_raise(etsec->rx_irq);
> -        RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> -    }
> +    etsec_update_irq(etsec);
>  }
>  
>  static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.