Re: [PATCH iwl-next v5 1/9] net: ethtool: mm: extract stmmac verification logic into common library

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Feb 21, 2025 at 09:30:09PM +0800, Abdul Rahim, Faizal wrote:
> On 21/2/2025 6:43 pm, Vladimir Oltean wrote:
> > On Fri, Feb 21, 2025 at 06:24:09PM +0800, Furong Xu wrote:
> > > Your fix is better when link is up/down, so I vote verify_enabled.
> > 
> > Hmmm... I thought this was a bug in stmmac that was carried over to
> > ethtool_mmsv, but it looks like it isn't.
> > 
> > In fact, looking at the original refactoring patch I had attached in
> > this email:
> > https://lore.kernel.org/netdev/20241217002254.lyakuia32jbnva46@skbuf/
> > 
> > these 2 lines in ethtool_mmsv_link_state_handle() didn't exist at all.
> > 
> > 	} else {
> > > > > > 		mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
> > > > > > 		mmsv->verify_retries = ETHTOOL_MM_MAX_VERIFY_RETRIES;
> > 
> > 		/* No link or pMAC not enabled */
> > 		ethtool_mmsv_configure_pmac(mmsv, false);
> > 		ethtool_mmsv_configure_tx(mmsv, false);
> > 	}
> > 
> > Faizal, could you remind me why they were added? I don't see this
> > explained in change logs.
> > 
> 
> Hi Vladimir,
> 
> Yeah, it wasn’t there originally. I added that change because it failed the
> link down/link up test.
> After a successful verification, if the link partner goes down, the status
> still shows ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED, which isn’t correct—so
> that’s why I added it.
> 
> Sorry for not mentioning it earlier. I assumed you’d check the delta between
> the original patch and the upstream one, my bad, should have mentioned this
> logic change.
> 
> Should I update it to the latest suggestion?

Never, ever modify logic in the same commit as you are moving code.
I was wondering what's with the Co-developed-by: tags, but I had just
assumed fixups were made to code I had improperly moved because I
didn't have hardware to test. Always structure patches to be one single
logical change per patch, well justified and trivially correct.

I had assumed, in good faith, changes like this wouldn't sneak in, but I
guess thanks for letting me know I should check next time :)

I think it's a slightly open question which state should the verification
be in when the link fails, but in any case, your argument could be made
that the state of the previous verification should be lost.

If I look at figure 99-8 in the Verify state diagram, I see that
whenever the condition "begin || link_fail || disableVerify || !pEnable"
is true, we transition to the state INIT_VERIFICATION. From there, there
is a UCT (unconditional transition) to VERIFICATION_IDLE, and from there,
a transition to state SEND_VERIFY based on "pEnable && !disableVerify".
In principle what this is telling me is that as long as management
software doesn't set pEnable (tx_enable in Linux) to false, verification
would be attempted even with link down, and should eventually fail.

But the mmsv state machine does call ethtool_mmsv_configure_tx(mmsv, false),
and in that case, if I were to interpret the standard state machine very
strictly, it would remain blocked in state VERIFICATION_IDLE until a
link up (thus, we should report the state as "verifying").

But, to be honest, I think the existence of the VERIFICATION_IDLE state
doesn't make a lot of sense. The state machine should just transition on
"!link_fail && !disable_verify && pEnable" to SEND_VERIFY directly, and
from state WAIT_FOR_RESPONSE it should cycle back to SEND_VERIFY if the
verify timer expired but we still have retries, or to INIT_VERIFICATION
if link_fail, disableVerify or pEnable change. One more reason why I
believe the VERIFICATION_IDLE state is redundant and under-specified is
because it gives the user no chance to even _see_ the "initial" state
being reported ever, given the unconditional transition to VERIFICATION_IDLE.

So in that sense, I agree with your proposal, and in terms of code,
I would recommend just this:

 } else {
+	/* Reset the reported verification state while the link is down */
+	if (mmsv->verify_enabled)
+		mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
 
 	/* No link or pMAC not enabled */
 	ethtool_mmsv_configure_pmac(mmsv, false);
 	ethtool_mmsv_configure_tx(mmsv, false);
 }

Because this is just for reporting to user space, resetting
"mmsv->verify_retries = ETHTOOL_MM_MAX_VERIFY_RETRIES;" doesn't matter,
we'll do it on link up anyway.

Also note that there's no ternary operator like in the discussion with
Furong. If mmsv->verify_enabled is false, the mmsv->status should
already be DISABLED, no need for us to re-assign it.