Message-ID: <8395976e-c867-4788-82e6-6d606599bf6c@prolan.hu>
Date: Tue, 21 Jan 2025 21:41:36 +0100
From: Csókás Bence <csokas.bence@...lan.hu>
To: "Badel, Laurent" <LaurentBadel@...on.com>, Jakub Kicinski
	<kuba@...nel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: Wei Fang <wei.fang@....com>, Shenwei Wang <shenwei.wang@....com>, "Clark
 Wang" <xiaoning.wang@....com>, Andrew Lunn <andrew+netdev@...n.ch>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Paolo
 Abeni" <pabeni@...hat.com>
Subject: Re: [EXTERNAL] [PATCH] net: fec: Refactor MAC reset to function

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

Powered by blists - more mailing lists