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

[Jason Wang]

On 2020/6/2 上午12:47, Andrew Melnichenko wrote:
> As I understand it, the e1000e.c was implemented by 82574L 
> spec(https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf).
> In the same spec there is 10.2.4 paragraph which provides more details 
> when ICR should be cleared.
>
>     • Case 1 - Interrupt Mask register equals 0x0000 (mask all): ICR
>     content is cleared.
>     • Case 2 - Interrupt was asserted (ICR.INT_ASSERT=1) and auto mask
>     is active: ICR
>     content is cleared, and the IAM register is written to the IMC
>     register.
>     • Case 3 - Interrupt was not asserted (ICR.INT_ASSERT=0): Read has
>     no side affect.

Thanks for the pointer, so it looks to me the current implementation is 
fine ?

static uint32_t
e1000e_mac_icr_read(E1000ECore *core, int index)
{
    uint32_t ret = core->mac[ICR];
    trace_e1000e_irq_icr_read_entry(ret);

    if (core->mac[IMS] == 0) {
        trace_e1000e_irq_icr_clear_zero_ims();
        core->mac[ICR] = 0;
    }


// This is the case 1)


    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
        (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
        trace_e1000e_irq_icr_clear_iame();
        core->mac[ICR] = 0;
        trace_e1000e_irq_icr_process_iame();
        e1000e_clear_ims_bits(core, core->mac[IAM]);
    }


// This is the case 2) and case 3)

    trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
    e1000e_update_interrupt_state(core);
    return ret;
}


Thanks


> On Fri, May 29, 2020 at 10:35 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/5/29 下午3:18, Jason Wang wrote:
>     >
>     > On 2020/5/13 下午7:31, andrew@daynix.com
>     <mailto:andrew@daynix.com> wrote:
>     >> From: Andrew Melnychenko <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>>
>     >> ---
>     >>   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 d5676871fa..10212d7932 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;
>     >> +    }
>     >> +
>     >
>     >
>     > Hi Andrew:
>     >
>     > So my comments still. I think we need to implement 82574l
>     behavior (if
>     > you go through e1000e.c all chapters it mentioned is for 82574l
>     > datasheet not the one you pointed to me).
>     >
>     > And actually the 82574l behavior is much more simpler.
>
>
>     To be more specific.
>
>     See chapter 7.4.5 which describes the ICR clearing.
>
>     It has three methods for clearing: auto-clear, clear-on-write and
>     clear-on-read.
>
>     And in the part of "Read to clear" it said:
>
>     """
>     All bits in the ICR register are cleared on a read to ICR.
>
>     """
>
>     So there's no need to IMS and other stuffs here.
>
>     Thanks
>
>
>     >
>     > Thanks