Re: [PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.

[Jason Wang <jasowang@redhat.com>]

On 2020/5/11 下午6:08, Andrew Melnichenko wrote:
> Yo,
>
>     So I think we should implement the 82574l behavior?
>
>  Well, as I understand it - its already implemented. I've added ICR 
> clearance if ICR & IMS(also need to add ICR_ASSERTED check, my bad, 
> I'll prepare new patch).


Yes, but it behave more like e.g 82573 not what we claim to emulate like 
82574l.


>
> At first, I had hacks to clear 'msi_causes_pending' at 
> 'e1000e_core_set_link_status()' before link down. It works but it's 
> not a solution.
> Also, on Windows the bug doesn't reproduce. I've traced Windows and 
> Linux - the difference that Windows driver clears pending by writing 
> to ICR, where Linux tries to clear by reading it.
> I had another possible fix - for Linux driver(writing to ICR at 
> interrupt routine).
> I've asked intel guys, does Linux driver works with a device(I don't 
> have real one). Thay said that it works and suggested to check 8257x 
> spec. I'll forward the message to you.


Ok.

Thanks


>
> On Sat, May 9, 2020 at 9:02 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/5/9 上午2:13, Andrew Melnichenko wrote:
>     > Yo, I've used OpenSDM_8257x-18.pdf specification.
>     > This document was recommended by Intel guys(Also, they
>     referenced to
>     > that note).
>     > I've made a fast fix and it works. Before that I had a fix for
>     Linux
>     > e1000e driver.
>     > Overall, the issue was in pending interrupts that can't be
>     cleared by
>     > reading ICR in Linux(Windows driver clears by writing to ICR).
>     >
>     > You can download spec for example from:
>     >
>     http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf
>
>
>     Interesting, this spec doesn't include 82574l which is what e1000e
>     claims to emulate:
>
>          c->vendor_id = PCI_VENDOR_ID_INTEL;
>          c->device_id = E1000_DEV_ID_82574L;
>
>     Looking at 82574l spec (using the link mentioned in
>     e1000e_core.c), it
>     said (7.4.3):
>
>     In MSI-X mode the bits in this register can be configured to
>     auto-clear
>     when the MSI-X
>     interrupt message is sent, in order to minimize driver overhead, and
>     when using MSI-X
>     interrupt signaling.
>     In systems that do not support MSI-X, reading the ICR register clears
>     it's bits or writing
>     1b's clears the corresponding bits in this register.
>
>     So the auto clear is under the control of EIAC (MSIX) or
>     unconditionally
>     (non MSI-X).
>
>     But what has been implemented in e1000e_mac_icr_read() is something
>     similar to the behavior of non 82574l card.
>
>     So I think we should implement the 82574l behavior?
>
>     Thanks
>
>
>     >
>     > On Fri, May 8, 2020 at 5:21 AM Jason Wang <jasowang@redhat.com
>     <mailto:jasowang@redhat.com>
>     > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>> wrote:
>     >
>     >
>     >     On 2020/5/7 上午5:26, andrew@daynix.com
>     <mailto:andrew@daynix.com> <mailto:andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >     wrote:
>     >     > From: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>
>     >     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     >
>     >     > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>     >     > Added ICR clearing if there is IMS bit - according to the
>     note by
>     >     > section 13.3.27 of the 8257X developers manual.
>     >     >
>     >     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>
>     >     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     > ---
>     >     >   hw/net/e1000e_core.c | 9 +++++++++
>     >     >   hw/net/trace-events  | 1 +
>     >     >   2 files changed, 10 insertions(+)
>     >     >
>     >     > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>     >     > index d5676871fa..302e99ff46 100644
>     >     > --- a/hw/net/e1000e_core.c
>     >     > +++ b/hw/net/e1000e_core.c
>     >     > @@ -2624,6 +2624,15 @@ 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
>     >     > +     */
>     >
>     >
>     >     Hi Andrew:
>     >
>     >     Which version of the manual did you use? I try to use the one
>     >     mentioned
>     >     in e1000e.c which is
>     >
>     http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf.
>     >
>     >     But I couldn't find chapter 13.3.27.
>     >
>     >     Thanks
>     >
>     >
>     >     > +    if (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;
>     >     > diff --git a/hw/net/trace-events b/hw/net/trace-events
>     >     > index e18f883cfd..46e40fcfa9 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"
>     >
>