Message-ID: <8f8d6149-d6a6-4fec-bb4d-fa0eb3613cd8@linux.intel.com>
Date: Tue, 17 Dec 2024 17:47:02 +0800
From: "Abdul Rahim, Faizal" <faizal.abdul.rahim@...ux.intel.com>
To: Vladimir Oltean <olteanv@...il.com>, Furong Xu <0x1207@...il.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S . Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 Vinicius Costa Gomes <vinicius.gomes@...el.com>,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption
 verification



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

Powered by blists - more mailing lists