[v1] e1000e: Add ICR clearing by corresponding IMS bit

[PATCH] e1000e: Add ICR clearing by corresponding IMS bit

Posted by Akihiko Odaki 11 months ago

The datasheet does not say what happens when interrupt was asserted
(ICR.INT_ASSERT=1) and auto mask is *not* active.
However, section of 13.3.27 the PCIe* GbE Controllers Open Source
Software Developer’s Manual, which were written for older devices,
namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
82573E/82573V/82573L, does say:
> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is
> not 0b, but some ICR bit is set where the corresponding IMS bit is not
> set, then a read does not clear the ICR register. For example, if
> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register
> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read
> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b).

Linux does no longer activate auto mask since commit
0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears
ICR even in such a case so we also should do so.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Supersedes: <20201203133236.222207-1-andrew@daynix.com>
("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.")

 hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------
 hw/net/trace-events  |  1 +
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 9785ef279c..338bbbf4f4 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
         e1000e_lower_interrupts(core, ICR, 0xffffffff);
     }
 
-    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
-        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
-        trace_e1000e_irq_icr_clear_iame();
-        e1000e_lower_interrupts(core, ICR, 0xffffffff);
-        trace_e1000e_irq_icr_process_iame();
-        e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
+    if (core->mac[ICR] & E1000_ICR_ASSERTED) {
+        if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) {
+            trace_e1000e_irq_icr_clear_iame();
+            e1000e_lower_interrupts(core, ICR, 0xffffffff);
+            trace_e1000e_irq_icr_process_iame();
+            e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
+        }
+
+        /*
+         * The datasheet does not say what happens when interrupt was asserted
+         * (ICR.INT_ASSERT=1) and auto mask is *not* active.
+         * However, section of 13.3.27 the PCIe* GbE Controllers Open Source
+         * Software Developer’s Manual, which were written for older devices,
+         * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
+         * 82573E/82573V/82573L, does say:
+         * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS
+         * > is not 0b, but some ICR bit is set where the corresponding IMS bit
+         * > is not set, then a read does not clear the ICR register. For
+         * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the
+         * > ICR register does not clear it. If IMS = 10101010b and
+         * > ICR = 0101011b, then a read to the ICR register clears it entirely
+         * > (ICR.INT_ASSERTED = 1b).
+         *
+         * Linux does no longer activate auto mask since commit
+         * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware
+         * clears ICR even in such a case so we also should do so.
+         */
+        if (core->mac[ICR] & core->mac[IMS]) {
+            trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
+                                                   core->mac[IMS]);
+            e1000e_lower_interrupts(core, ICR, 0xffffffff);
+        }
     }
 
     return ret;
diff --git a/hw/net/trace-events b/hw/net/trace-events
index e97e9dc17b..9103488e17 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
 e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int"
 e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
 e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
+e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
 e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
 e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
 e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"
-- 
2.40.1

Re: [PATCH] e1000e: Add ICR clearing by corresponding IMS bit

Posted by Akihiko Odaki 10 months, 1 week ago

On 2023/06/02 16:25, Akihiko Odaki wrote:
> The datasheet does not say what happens when interrupt was asserted
> (ICR.INT_ASSERT=1) and auto mask is *not* active.
> However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> Software Developer’s Manual, which were written for older devices,
> namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> 82573E/82573V/82573L, does say:
>> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is
>> not 0b, but some ICR bit is set where the corresponding IMS bit is not
>> set, then a read does not clear the ICR register. For example, if
>> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register
>> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read
>> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b).
> 
> Linux does no longer activate auto mask since commit
> 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears
> ICR even in such a case so we also should do so.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Supersedes: <20201203133236.222207-1-andrew@daynix.com>
> ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.")
> 
>   hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------
>   hw/net/trace-events  |  1 +
>   2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 9785ef279c..338bbbf4f4 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>           e1000e_lower_interrupts(core, ICR, 0xffffffff);
>       }
>   
> -    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> -        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
> -        trace_e1000e_irq_icr_clear_iame();
> -        e1000e_lower_interrupts(core, ICR, 0xffffffff);
> -        trace_e1000e_irq_icr_process_iame();
> -        e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> +    if (core->mac[ICR] & E1000_ICR_ASSERTED) {
> +        if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) {
> +            trace_e1000e_irq_icr_clear_iame();
> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> +            trace_e1000e_irq_icr_process_iame();
> +            e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> +        }
> +
> +        /*
> +         * The datasheet does not say what happens when interrupt was asserted
> +         * (ICR.INT_ASSERT=1) and auto mask is *not* active.
> +         * However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> +         * Software Developer’s Manual, which were written for older devices,
> +         * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> +         * 82573E/82573V/82573L, does say:
> +         * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS
> +         * > is not 0b, but some ICR bit is set where the corresponding IMS bit
> +         * > is not set, then a read does not clear the ICR register. For
> +         * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the
> +         * > ICR register does not clear it. If IMS = 10101010b and
> +         * > ICR = 0101011b, then a read to the ICR register clears it entirely
> +         * > (ICR.INT_ASSERTED = 1b).
> +         *
> +         * Linux does no longer activate auto mask since commit
> +         * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware
> +         * clears ICR even in such a case so we also should do so.
> +         */
> +        if (core->mac[ICR] & core->mac[IMS]) {
> +            trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> +                                                   core->mac[IMS]);
> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> +        }
>       }
>   
>       return ret;
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index e97e9dc17b..9103488e17 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
>   e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int"
>   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
>   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
>   e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
>   e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
>   e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"

Hi Jason,

Can you have a look at this patch and
"[PATCH] igb: Remove obsolete workaround for Windows":
https://patchew.org/QEMU/20230529023704.9387-1-akihiko.odaki@daynix.com/

Regards,
Akihiko Odaki

Re: [PATCH] e1000e: Add ICR clearing by corresponding IMS bit

Posted by Jason Wang 10 months ago

On Thu, Jun 29, 2023 at 4:53 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/06/02 16:25, Akihiko Odaki wrote:
> > The datasheet does not say what happens when interrupt was asserted
> > (ICR.INT_ASSERT=1) and auto mask is *not* active.
> > However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> > Software Developer’s Manual, which were written for older devices,
> > namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> > 82573E/82573V/82573L, does say:
> >> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is
> >> not 0b, but some ICR bit is set where the corresponding IMS bit is not
> >> set, then a read does not clear the ICR register. For example, if
> >> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register
> >> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read
> >> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b).
> >
> > Linux does no longer activate auto mask since commit
> > 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears
> > ICR even in such a case so we also should do so.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> > Supersedes: <20201203133236.222207-1-andrew@daynix.com>
> > ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.")
> >
> >   hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------
> >   hw/net/trace-events  |  1 +
> >   2 files changed, 33 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index 9785ef279c..338bbbf4f4 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
> >           e1000e_lower_interrupts(core, ICR, 0xffffffff);
> >       }
> >
> > -    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> > -        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
> > -        trace_e1000e_irq_icr_clear_iame();
> > -        e1000e_lower_interrupts(core, ICR, 0xffffffff);
> > -        trace_e1000e_irq_icr_process_iame();
> > -        e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> > +    if (core->mac[ICR] & E1000_ICR_ASSERTED) {
> > +        if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) {
> > +            trace_e1000e_irq_icr_clear_iame();
> > +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> > +            trace_e1000e_irq_icr_process_iame();
> > +            e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> > +        }
> > +
> > +        /*
> > +         * The datasheet does not say what happens when interrupt was asserted
> > +         * (ICR.INT_ASSERT=1) and auto mask is *not* active.
> > +         * However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> > +         * Software Developer’s Manual, which were written for older devices,
> > +         * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> > +         * 82573E/82573V/82573L, does say:
> > +         * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS
> > +         * > is not 0b, but some ICR bit is set where the corresponding IMS bit
> > +         * > is not set, then a read does not clear the ICR register. For
> > +         * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the
> > +         * > ICR register does not clear it. If IMS = 10101010b and
> > +         * > ICR = 0101011b, then a read to the ICR register clears it entirely
> > +         * > (ICR.INT_ASSERTED = 1b).
> > +         *
> > +         * Linux does no longer activate auto mask since commit
> > +         * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware
> > +         * clears ICR even in such a case so we also should do so.
> > +         */
> > +        if (core->mac[ICR] & core->mac[IMS]) {
> > +            trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> > +                                                   core->mac[IMS]);
> > +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> > +        }
> >       }
> >
> >       return ret;
> > diff --git a/hw/net/trace-events b/hw/net/trace-events
> > index e97e9dc17b..9103488e17 100644
> > --- a/hw/net/trace-events
> > +++ b/hw/net/trace-events
> > @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
> >   e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int"
> >   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
> >   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
> > +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
> >   e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
> >   e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
> >   e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"
>
> Hi Jason,
>
> Can you have a look at this patch and
> "[PATCH] igb: Remove obsolete workaround for Windows":
> https://patchew.org/QEMU/20230529023704.9387-1-akihiko.odaki@daynix.com/
>
> Regards,
> Akihiko Odaki

I've queued both of the patches.

Thanks

>

Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit

Posted by Paul Menzel 11 months ago

Dear Akihiko,


Thank you for your patch.

After looking at the diff, it looks like this is a QEMU patch, and not 
one for the Linux kernel. I leave my inline comments anyway.


Am 02.06.23 um 09:25 schrieb Akihiko Odaki:

It’d be nice if you started by summarizing the bug.

> The datasheet does not say what happens when interrupt was asserted
> (ICR.INT_ASSERT=1) and auto mask is *not* active.

Personal nit: For better legibility, I’d separate paragraphs by an empty 
line, or – in this case – I wouldn’t wrap the line, just because a 
sentence ends.

> However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> Software Developer’s Manual, which were written for older devices,
> namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> 82573E/82573V/82573L, does say:
>> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is
>> not 0b, but some ICR bit is set where the corresponding IMS bit is not
>> set, then a read does not clear the ICR register. For example, if
>> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register
>> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read
>> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b).
> 
> Linux does no longer activate auto mask since commit
> 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears
> ICR even in such a case so we also should do so.

… since commit 0a8047ac68e5 (e1000e: Fix msi-x interrupt automask) …

> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Supersedes: <20201203133236.222207-1-andrew@daynix.com>
> ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.")
> 
>   hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------
>   hw/net/trace-events  |  1 +
>   2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 9785ef279c..338bbbf4f4 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>           e1000e_lower_interrupts(core, ICR, 0xffffffff);
>       }
>   
> -    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> -        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
> -        trace_e1000e_irq_icr_clear_iame();
> -        e1000e_lower_interrupts(core, ICR, 0xffffffff);
> -        trace_e1000e_irq_icr_process_iame();
> -        e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> +    if (core->mac[ICR] & E1000_ICR_ASSERTED) {
> +        if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) {
> +            trace_e1000e_irq_icr_clear_iame();
> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> +            trace_e1000e_irq_icr_process_iame();
> +            e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
> +        }
> +
> +        /*
> +         * The datasheet does not say what happens when interrupt was asserted

I believe the network subsystem has a different commenting style than 
the rest of the Linux source code.

> +         * (ICR.INT_ASSERT=1) and auto mask is *not* active.
> +         * However, section of 13.3.27 the PCIe* GbE Controllers Open Source
> +         * Software Developer’s Manual, which were written for older devices,
> +         * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
> +         * 82573E/82573V/82573L, does say:

I’d add a blank line below.

> +         * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS
> +         * > is not 0b, but some ICR bit is set where the corresponding IMS bit
> +         * > is not set, then a read does not clear the ICR register. For
> +         * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the
> +         * > ICR register does not clear it. If IMS = 10101010b and
> +         * > ICR = 0101011b, then a read to the ICR register clears it entirely
> +         * > (ICR.INT_ASSERTED = 1b).
> +         *
> +         * Linux does no longer activate auto mask since commit
> +         * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware
> +         * clears ICR even in such a case so we also should do so.
> +         */
> +        if (core->mac[ICR] & core->mac[IMS]) {
> +            trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> +                                                   core->mac[IMS]);
> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
> +        }
>       }
>   
>       return ret;
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index e97e9dc17b..9103488e17 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
>   e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read due to non MSI-X int"
>   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
>   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
>   e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
>   e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
>   e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit

Posted by Paul Menzel 11 months ago

[Removed bpoirier@suse.com from Cc: again.]

Am 02.06.23 um 11:41 schrieb Paul Menzel:
> Dear Akihiko,
> 
> 
> Thank you for your patch.
> 
> After looking at the diff, it looks like this is a QEMU patch, and not 
> one for the Linux kernel. I leave my inline comments anyway.
> 
> 
> Am 02.06.23 um 09:25 schrieb Akihiko Odaki:
> 
> It’d be nice if you started by summarizing the bug.
> 
>> The datasheet does not say what happens when interrupt was asserted
>> (ICR.INT_ASSERT=1) and auto mask is *not* active.
> 
> Personal nit: For better legibility, I’d separate paragraphs by an empty 
> line, or – in this case – I wouldn’t wrap the line, just because a 
> sentence ends.
> 
>> However, section of 13.3.27 the PCIe* GbE Controllers Open Source
>> Software Developer’s Manual, which were written for older devices,
>> namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
>> 82573E/82573V/82573L, does say:
>>> If IMS = 0b, then the ICR register is always clear-on-read. If IMS is
>>> not 0b, but some ICR bit is set where the corresponding IMS bit is not
>>> set, then a read does not clear the ICR register. For example, if
>>> IMS = 10101010b and ICR = 01010101b, then a read to the ICR register
>>> does not clear it. If IMS = 10101010b and ICR = 0101011b, then a read
>>> to the ICR register clears it entirely (ICR.INT_ASSERTED = 1b).
>>
>> Linux does no longer activate auto mask since commit
>> 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware clears
>> ICR even in such a case so we also should do so.
> 
> … since commit 0a8047ac68e5 (e1000e: Fix msi-x interrupt automask) …
> 
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> Supersedes: <20201203133236.222207-1-andrew@daynix.com>
>> ("[PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.")
>>
>>   hw/net/e1000e_core.c | 38 ++++++++++++++++++++++++++++++++------
>>   hw/net/trace-events  |  1 +
>>   2 files changed, 33 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index 9785ef279c..338bbbf4f4 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -2607,12 +2607,38 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>>           e1000e_lower_interrupts(core, ICR, 0xffffffff);
>>       }
>> -    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>> -        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>> -        trace_e1000e_irq_icr_clear_iame();
>> -        e1000e_lower_interrupts(core, ICR, 0xffffffff);
>> -        trace_e1000e_irq_icr_process_iame();
>> -        e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
>> +    if (core->mac[ICR] & E1000_ICR_ASSERTED) {
>> +        if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME) {
>> +            trace_e1000e_irq_icr_clear_iame();
>> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
>> +            trace_e1000e_irq_icr_process_iame();
>> +            e1000e_lower_interrupts(core, IMS, core->mac[IAM]);
>> +        }
>> +
>> +        /*
>> +         * The datasheet does not say what happens when interrupt was asserted
> 
> I believe the network subsystem has a different commenting style than 
> the rest of the Linux source code.
> 
>> +         * (ICR.INT_ASSERT=1) and auto mask is *not* active.
>> +         * However, section of 13.3.27 the PCIe* GbE Controllers Open Source
>> +         * Software Developer’s Manual, which were written for older devices,
>> +         * namely 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI &
>> +         * 82573E/82573V/82573L, does say:
> 
> I’d add a blank line below.
> 
>> +         * > If IMS = 0b, then the ICR register is always clear-on-read. If IMS
>> +         * > is not 0b, but some ICR bit is set where the corresponding IMS bit
>> +         * > is not set, then a read does not clear the ICR register. For
>> +         * > example, if IMS = 10101010b and ICR = 01010101b, then a read to the
>> +         * > ICR register does not clear it. If IMS = 10101010b and
>> +         * > ICR = 0101011b, then a read to the ICR register clears it entirely
>> +         * > (ICR.INT_ASSERTED = 1b).
>> +         *
>> +         * Linux does no longer activate auto mask since commit
>> +         * 0a8047ac68e50e4ccbadcfc6b6b070805b976885 and the real hardware
>> +         * clears ICR even in such a case so we also should do so.
>> +         */
>> +        if (core->mac[ICR] & core->mac[IMS]) {
>> +            trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
>> +                                                   core->mac[IMS]);
>> +            e1000e_lower_interrupts(core, ICR, 0xffffffff);
>> +        }
>>       }
>>       return ret;
>> diff --git a/hw/net/trace-events b/hw/net/trace-events
>> index e97e9dc17b..9103488e17 100644
>> --- a/hw/net/trace-events
>> +++ b/hw/net/trace-events
>> @@ -217,6 +217,7 @@ e1000e_irq_read_ims(uint32_t ims) "Current IMS: 0x%x"
>>   e1000e_irq_icr_clear_nonmsix_icr_read(void) "Clearing ICR on read 
>> due to non MSI-X int"
>>   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
>>   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
>> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
>>   e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
>>   e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
>>   e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"
> 
> 
> Kind regards,
> 
> Paul