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

[Paul Menzel <pmenzel@molgen.mpg.de>]

[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