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

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

On 21/2/2025 10:44 pm, Vladimir Oltean wrote:
> 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.

Got it, sorry about that.

> 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.
> 

Will update, thanks.