Re: [EXTERNAL] [PATCH] net: fec: Refactor MAC reset to function

[Csókás Bence]

Hi Laurent,

On 2025. 01. 21. 17:09, Badel, Laurent wrote:
> Hi Bence and thanks for the patch.

thanks for your input.

> Leaving out the check for FEC_QUIRK_NO_HARD_RESET in fec_stop() was, in fact,
> not unintentional. Although a hard reset in fec_restart() caused link issues
> with the iMX28, I had no particular reason to believe that it would also cause
> issues in fec_stop(), since at this point you're turning off the interface, and
> I did not observe any particular problems either, so I did not think the same
> modification was warranted there.

I had a feeling it was intentional, however, `fec_stop()` is called all 
over the place - not just when removing the interface (e.g. unloading 
the driver), but also by the PM subsystem for entering suspend, 
restarting auto-negotiation, for handling Pause frames and changing 
HW-accelerated RX checksum checking...

> If you have reason to believe that this is a bug, then it should be fixed, but
> currently I don't see why this is the case here. I think a refactoring
> duplicated code is a good idea, but since it also includes a modification of
> the behavior (specifically, there is a possible path where
> FEC_QUIRK_NO_HARD_RESET is set and the link is up, where fec_stop() will issue
> a soft reset instead of a hard reset), I would prefer to know that this change
> is indeed necessary.
>
> If others disagree and there's a consensus that this change is ok, I'm happy
> for the patch to get through, but I tend to err on the side of caution in such
> cases.

To me, the name `FEC_QUIRK_NO_HARD_RESET`, and its doc-comment seems to 
suggest that we do *not* want to hard-reset this MAC *ever*; not in the 
codepath of `fec_restart()` and not in `fec_stop()`. Did you observe 
problems on i.MX28 if you soft-reset it in stop()? I _might_ be able to 
get my hands on an i.MX287 and test, but I have no idea if it is 
working; I took it out from the junk bin.

Right now, we're chasing a different bug on the i.MX6, and this was just 
meant to reduce the amount of clutter we have to cut through.

> An additional comment - this is just my personal opinion - but in
> fec_ctrl_reset(), it seems to me that the function of the wol argument really
> is to distinguish if we're using the fec_restart() or the fec_stop()
> implementation, so I think the naming may be a bit misleading in this case.

True, but I would prefer to keep it separate, i.e. the `wol` parameter 
should really only control whether we want to enable WoL. If we decide 
to keep the old behavior of not honoring FEC_QUIRK_NO_HARD_RESET in 
stop(), I'd rather add a new parameter `allow_soft_reset`. That way, if 
ever someone needs to call `fec_ctrl_reset()`, they will be able to give 
it values they make sense at that point-of-call, instead of having to 
"fake" either restart() or stop().

Bence