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.
