linux-kernel - RE: [PATCH] net: fec: Refactor MAC reset to function

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <PAXPR04MB8510A8D8807A4EB3188E935788E12@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Wed, 22 Jan 2025 02:50:39 +0000
From: Wei Fang <wei.fang@....com>
To: Csókás, Bence <csokas.bence@...lan.hu>, Jakub Kicinski
	<kuba@...nel.org>, Laurent Badel <laurentbadel@...on.com>,
	"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: 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: [PATCH] net: fec: Refactor MAC reset to function

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

'wol' gives me the feeling that it indicates whether the WOL function
is enabled. And the else branch will be taken if 'wol' is true. But in fact,
even if 'wol' is true, it may go to the reset branch. This is a bit confusing.

How about the following changes?

static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol)
{
	if (wol) {
		u32 val;

		val = readl(fep->hwp + FEC_ECNTRL);
		val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
		writel(val, fep->hwp + FEC_ECNTRL);
	} else 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);
	}
}

> +{
> +	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);
> -		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
>