Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Abdul Rahim, Faizal]

On 17/12/2024 8:22 am, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 01:47:17AM -0500, Faizal Rahim wrote:
> > The i226 hardware doesn't implement the process of verification
> > internally, this is left to the driver.
> >
> > Add a simple implementation of the state machine defined in IEEE
> > 802.3-2018, Section 99.4.7. The state machine is started manually by
> > user after "verify-enabled" command is enabled.
> >
> > Implementation includes:
> > 1. Send and receive verify frame
> > 2. Verification state handling
> > 3. Send and receive response frame
> >
> > Tested by triggering verification handshake:
> > $ sudo ethtool --set-mm enp1s0 pmac-enabled on
> > $ sudo ethtool --set-mm enp1s0 tx-enabled on
> > $ sudo ethtool --set-mm enp1s0 verify-enabled on
> >
> > Note that Ethtool API requires enabling "pmac-enabled on" and
> > "tx-enabled on" before "verify-enabled on" can be issued.
> >
> > After the upcoming patch ("igc: Add support to get MAC Merge data via
> > ethtool") is implemented, verification status can be checked using:
> > $ ethtool --show-mm enp1s0
> >    MAC Merge layer state for enp1s0:
> >    pMAC enabled: on
> >    TX enabled: on
> >    TX active: on
> >    TX minimum fragment size: 252
> >    RX minimum fragment size: 252
> >    Verify enabled: on
> >    Verify time: 128
> >    Max verify time: 128
> >    Verification status: SUCCEEDED
> >
> > Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
> > Signed-off-by: Faizal Rahim <faizal.abdul.rahim@xxxxxxxxxxxxxxx>
> > ---
>
> Am I missing something, or this does not handle link state changes,
> where the verification should restart on each link up? (maybe the old
> link partner didn't support FPE and the new one does, or vice versa)
>
> Either I don't follow the link between igc_watchdog_task() and any
> verification related task, or it doesn't exist.

The latter. I missed this "link state changes" interaction, will rework, 
thanks.

> Anyway, while browsing through this software implementation of a
> verification process, I cannot help but think we'd be making a huge
> mistake to allow each driver to reimplement it on its own. We just
> recently got stmmac to do something fairly clean, with the help and
> great perseverence of Furong Xu (now copied).
>
> I spent a bit of time extracting stmmac's core logic and putting it in
> ethtool. If Furong had such good will so as to regression-test the
> attached patch, do you think you could use this as a starting place
> instead, and implement some ops and call some library methods, instead
> of writing the entire logic yourself?

Totally agree with moving it to a layer reusable by any driver. Thank you 
so much for the skeleton patch implementing it in ethtool — I’ll expand on 
it from here.