| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DM4PR17MB5969CC2A8D074890B695AF2ADFE62@DM4PR17MB5969.namprd17.prod.outlook.com>
Date: Tue, 21 Jan 2025 16:09:23 +0000
From: "Badel, Laurent" <LaurentBadel@...on.com>
To: Csókás, Bence <csokas.bence@...lan.hu>, 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 Bence and thanks for the patch.
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.
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.
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.
Best regards,
Laurent
On Tue, Jan 21, 2025 at 11:38:58AM +0100, Csókás, Bence wrote:
> The core is reset both in `fec_restart()` (called on link-up) and
> `fec_stop()` (going to sleep, driver remove etc.).
> These two functions had their separate implementations, which was at
> first only a register write and a `udelay()` (and the accompanying
> block comment).
> However, since then we got soft-reset
> (MAC disable) and Wake-on-LAN support, which meant that these
> implementations diverged, often causing bugs. For instance, as of now,
> `fec_stop()` does not check for `FEC_QUIRK_NO_HARD_RESET`. To
> eliminate this bug-source, refactor implementation to a common
> function.
>
> Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link
> up")
> Signed-off-by: Csókás, Bence <csokas.bence@...lan.hu>
> ---
>
> Notes:
> Recommended options for this patch:
> `--color-moved --color-moved-ws=allow-indentation-change`
>
> drivers/net/ethernet/freescale/fec_main.c | 50
> +++++++++++------------
> 1 file changed, 23 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 68725506a095..850ef3de74ec 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev)
> }
> }
>
> +/* Whack a reset. We should wait for this.
> + * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
> + * instead of reset MAC itself.
> + */
> +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) {
> + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) {
> + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES ||
> + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
> + writel(0, fep->hwp + FEC_ECNTRL);
> + } else {
> + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL);
> + udelay(10);
> + }
> + } else {
> + val = readl(fep->hwp + FEC_ECNTRL);
> + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
> + writel(val, fep->hwp + FEC_ECNTRL);
> + }
> +}
> +
> /*
> * This function is called to start or restart the FEC during a link
> * change, transmit timeout, or to reconfigure the FEC. The network
> @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev)
> if (fep->bufdesc_ex)
> fec_ptp_save_state(fep);
>
> - /* Whack a reset. We should wait for this.
> - * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
> - * instead of reset MAC itself.
> - */
> - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES ||
> - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
> - writel(0, fep->hwp + FEC_ECNTRL);
> - } else {
> - writel(1, fep->hwp + FEC_ECNTRL);
Nit, in case of a need for v2 you can mark in commit message that FEC_ECR_RESET == 1, so define can be use instead of 1.
> - udelay(10);
> - }
> + fec_ctrl_reset(fep, false);
>
> /*
> * enet-mac reset will reset mac address registers too, @@
> -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev)
> if (fep->bufdesc_ex)
> fec_ptp_save_state(fep);
>
> - /* Whack a reset. We should wait for this.
> - * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
> - * instead of reset MAC itself.
> - */
> - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) {
> - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
> - writel(0, fep->hwp + FEC_ECNTRL);
> - } else {
> - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL);
> - udelay(10);
> - }
> - } else {
> - val = readl(fep->hwp + FEC_ECNTRL);
> - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
> - writel(val, fep->hwp + FEC_ECNTRL);
> - }
> + fec_ctrl_reset(fep, true);
> writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>
> --
> 2.48.1
Powered by blists - more mailing lists