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

["Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com>]

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@intel.com>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>> ---
> 
> 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.