[Intel-wired-lan] [PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.

[Alexander Duyck]

Okay. That sounds reasonable. You should repost this with your more
thorough explanation of the problem and how this solves it in the
patch description.

Thanks.

- Alex

On Mon, Dec 14, 2020 at 3:09 AM Andrew Melnichenko <andrew at daynix.com> wrote:
>
> Hi,
> The issue is in LSC clearing. So, after "link up"(during initialization), the next LSC event is masked and can't be processed.
> Technically, the event should be 'cleared' during ICR read.
> On Windows guest, everything works well, mostly because of different interrupt routines(ICR clears during register write).
> So, I've added ICR clearing during read, according to the note by
> section 13.3.27 of the 8257X developers manual.
>
> On Thu, Dec 3, 2020 at 7:57 PM Alexander Duyck <alexander.duyck at gmail.com> wrote:
>>
>> On Thu, Dec 3, 2020 at 5:00 AM Andrew Melnychenko <andrew at daynix.com> wrote:
>> >
>> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>>
>> So the bugzilla seems to be reporting that the NIC operstate is being
>> misreported when qemu has configured the link down. Based on the
>> description it isn't clear to me how this patch addresses that. Some
>> documentation on how you reproduced the issue, and what was seen
>> before and after this patch would be useful.
>>
>> > Added ICR clearing if there is IMS bit - according to the note by
>>
>> Should probably be "Add" instead of "Added". Same for the title of the patch.
>>
>> > section 13.3.27 of the 8257X developers manual.
>> >
>> > Signed-off-by: Andrew Melnychenko <andrew at daynix.com>
>> > ---
>> >  hw/net/e1000e_core.c | 10 ++++++++++
>> >  hw/net/trace-events  |  1 +
>> >  2 files changed, 11 insertions(+)
>> >
>> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> > index 095c01ebc6..9705f5c52e 100644
>> > --- a/hw/net/e1000e_core.c
>> > +++ b/hw/net/e1000e_core.c
>> > @@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>> >          e1000e_clear_ims_bits(core, core->mac[IAM]);
>> >      }
>> >
>> > +    /*
>> > +     * PCIe* GbE Controllers Open Source Software Developer's Manual
>> > +     * 13.3.27 Interrupt Cause Read Register
>> > +     */
>> > +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>> > +        (core->mac[ICR] & core->mac[IMS])) {
>> > +        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], core->mac[IMS]);
>> > +        core->mac[ICR] = 0;
>> > +    }
>> > +
>> >      trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
>> >      e1000e_update_interrupt_state(core);
>> >      return ret;
>>
>> Changes like this have historically been problematic. I am curious
>> what testing had been done on this and with what drivers? Keep in mind
>> that we have to support several flavors of BSD, Windows, and Linux
>> with this.
>>
>> > diff --git a/hw/net/trace-events b/hw/net/trace-events
>> > index 5db45456d9..2c3521a19c 100644
>> > --- a/hw/net/trace-events
>> > +++ b/hw/net/trace-events
>> > @@ -237,6 +237,7 @@ e1000e_irq_icr_read_entry(uint32_t icr) "Starting ICR read. Current ICR: 0x%x"
>> >  e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read. Current ICR: 0x%x"
>> >  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.29.2
>> >