* [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
@ 2023-06-02 7:25 ` Akihiko Odaki
0 siblings, 0 replies; 10+ messages in thread
From: Akihiko Odaki @ 2023-06-02 7:25 UTC (permalink / raw)
Cc: intel-wired-lan, qemu-devel, alexander.duyck, Dmitry Fleytman,
Jason Wang, Akihiko Odaki, Andrew Melnychenko
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
2023-06-02 7:25 ` Akihiko Odaki
@ 2023-06-02 9:41 ` Paul Menzel
-1 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2023-06-02 9:41 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Andrew Melnychenko, Dmitry Fleytman, Jason Wang, qemu-devel,
intel-wired-lan, Benjamin Poirier
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
@ 2023-06-02 9:41 ` Paul Menzel
0 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2023-06-02 9:41 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Benjamin Poirier, Andrew Melnychenko, Dmitry Fleytman,
Jason Wang, qemu-devel, intel-wired-lan
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
2023-06-02 9:41 ` Paul Menzel
@ 2023-06-02 9:49 ` Paul Menzel
-1 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2023-06-02 9:49 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Andrew Melnychenko, Dmitry Fleytman, Jason Wang, qemu-devel,
intel-wired-lan
[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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
@ 2023-06-02 9:49 ` Paul Menzel
0 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2023-06-02 9:49 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Andrew Melnychenko, Dmitry Fleytman, intel-wired-lan, Jason Wang,
qemu-devel
[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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
2023-06-02 7:25 ` Akihiko Odaki
@ 2023-06-29 8:53 ` Akihiko Odaki
-1 siblings, 0 replies; 10+ messages in thread
From: Akihiko Odaki @ 2023-06-29 8:53 UTC (permalink / raw)
To: Jason Wang
Cc: Andrew Melnychenko, Dmitry Fleytman, intel-wired-lan, qemu-devel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
@ 2023-06-29 8:53 ` Akihiko Odaki
0 siblings, 0 replies; 10+ messages in thread
From: Akihiko Odaki @ 2023-06-29 8:53 UTC (permalink / raw)
To: Jason Wang
Cc: intel-wired-lan, qemu-devel, alexander.duyck, Dmitry Fleytman,
Andrew Melnychenko
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
2023-06-29 8:53 ` Akihiko Odaki
@ 2023-06-30 6:05 ` Jason Wang
-1 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2023-06-30 6:05 UTC (permalink / raw)
To: Akihiko Odaki
Cc: intel-wired-lan, qemu-devel, alexander.duyck, Dmitry Fleytman,
Andrew Melnychenko
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: Add ICR clearing by corresponding IMS bit
@ 2023-06-30 6:05 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2023-06-30 6:05 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Andrew Melnychenko, Dmitry Fleytman, intel-wired-lan, qemu-devel
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
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 10+ messages in thread