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

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

[Csókás, Bence]

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`, and `fec_restart()` missed the refactor in
commit ff049886671c ("net: fec: Refactor: #define magic constants")
renaming the "magic" constant `1` to `FEC_ECR_RESET`.

To eliminate this bug-source, refactor implementation to a common function.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up")
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---

Notes:
    Recommended options for this patch:
    `--color-moved --color-moved-ws=allow-indentation-change`
    Changes in v2:
    * collect Michal's tag
    * reformat message to 75 cols
    * fix missing `u32 val`

 drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 68725506a095..520fe638ea00 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1064,6 +1064,29 @@ 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)
+{
+	u32 val;
+
+	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 +1103,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 +1357,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

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

[Jacob Keller]

On 1/22/2025 8:39 AM, 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`, and `fec_restart()` missed the refactor in
> commit ff049886671c ("net: fec: Refactor: #define magic constants")
> renaming the "magic" constant `1` to `FEC_ECR_RESET`.
> 
> To eliminate this bug-source, refactor implementation to a common function.
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up")
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---
> 

You didn't mention the tree this targets. This is a bug fix, so this
should be targeted to net.

> Notes:
>     Recommended options for this patch:
>     `--color-moved --color-moved-ws=allow-indentation-change`
>     Changes in v2:
>     * collect Michal's tag
>     * reformat message to 75 cols
>     * fix missing `u32 val`
> 
>  drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 68725506a095..520fe638ea00 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1064,6 +1064,29 @@ 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)
> +{
> +	u32 val;
> +
> +	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);
> +	}
> +}
> +

Typically this kind of refactor to functions is kept separate from bug
fixes. However, I think in this case, the diff size is small enough it
is easy to review, so I think its worth taking as-is to net.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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

[Wei Fang]

> 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`, and `fec_restart()` missed the refactor in
> commit ff049886671c ("net: fec: Refactor: #define magic constants")
> renaming the "magic" constant `1` to `FEC_ECR_RESET`.
> 
> To eliminate this bug-source, refactor implementation to a common function.
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up")
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---

You seem to have missed Andrew's comment in the v1. If this patch is just
a refactor, then the Fixes tag should be removed, and the target tree should
be net-next. If not, please describe the bug in more detail in the commit
message.

> 
> Notes:
>     Recommended options for this patch:
>     `--color-moved --color-moved-ws=allow-indentation-change`
>     Changes in v2:
>     * collect Michal's tag
>     * reformat message to 75 cols
>     * fix missing `u32 val`
> 
>  drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 68725506a095..520fe638ea00 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1064,6 +1064,29 @@ 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)
> +{
> +	u32 val;
> +
> +	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 +1103,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 +1357,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
> 

[PATCH net v3] net: fec: Refactor MAC reset to function

[Csókás, Bence]

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`, meaning the MII/RMII mode is cleared on eg.
a PM power-down event; and `fec_restart()` missed the refactor renaming
the "magic" constant `1` to `FEC_ECR_RESET`.

To harmonize current implementations, and eliminate this source of
potential future bugs, refactor implementation to a common function.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up")
Fixes: ff049886671c ("net: fec: Refactor: #define magic constants")
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---

Notes:
    Recommended options for this patch:
    `--color-moved --color-moved-ws=allow-indentation-change`
    Changes in v2:
    * collect Michal's tag
    * reformat message to 75 cols
    * fix missing `u32 val`
    Changes in v3:
    * rename parameter to `allow_wol`
    Changes in v3:
    * clarify message
    * collect Jacob's tag
    * rebased onto c2933b2befe2

 drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f7c4ce8e9a26..a86cfebedaa8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1093,6 +1093,29 @@ 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 allow_wol)
+{
+	u32 val;
+
+	if (!allow_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
@@ -1109,17 +1132,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,
@@ -1373,22 +1386,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

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

[Jakub Kicinski]

On Tue, 4 Feb 2025 10:37:54 +0100 Csókás, Bence wrote:
> For instance, as of now, `fec_stop()` does not check for
> `FEC_QUIRK_NO_HARD_RESET`, meaning the MII/RMII mode is cleared on eg.
> a PM power-down event; and `fec_restart()` missed the refactor renaming
> the "magic" constant `1` to `FEC_ECR_RESET`.

Laurent responded to v1 saying this was intentional. Please give more
details on how problem you're seeing and on what platforms. Otherwise
this is not a fix but refactoring.

Please don't post new versions in-reply-to, and add lore links to
the previous version in the changelog.
-- 
pw-bot: cr

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

[Csókás Bence]

Hi,

On 2025. 02. 04. 16:45, Jakub Kicinski wrote:
 > Please don't post new versions in-reply-to, and add lore links to
 > the previous version in the changelog.

Will do. Is it okay to only include the last version, or should I 
collect them going back to v1?

> On Tue, 4 Feb 2025 10:37:54 +0100 Csókás, Bence wrote:
>> For instance, as of now, `fec_stop()` does not check for
>> `FEC_QUIRK_NO_HARD_RESET`, meaning the MII/RMII mode is cleared on eg.
>> a PM power-down event; and `fec_restart()` missed the refactor renaming
>> the "magic" constant `1` to `FEC_ECR_RESET`.
> 
> Laurent responded to v1 saying this was intentional. Please give more
> details on how problem you're seeing and on what platforms. Otherwise
> this is not a fix but refactoring.

True, but he also said:
On 2025. 01. 21. 17:09, Badel, Laurent wrote:
 > 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.

I understand he is cautious, but I'd argue that the fact that two people 
already posted Reviewed-by: (not counting Simon, who since withdrew it), 
means that others also agree that we should err on the OTHER side of 
caution, and do the check in both cases. He also mentions that the 
reason he didn't do the check in `fec_stop()` was that he believed that 
the only time that gets called is on driver/interface remove, but that 
is not the case, as I outlined in the message already.

Bence

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

[Jakub Kicinski]

On Wed, 5 Feb 2025 14:53:50 +0100 Csókás Bence wrote:
> On 2025. 02. 04. 16:45, Jakub Kicinski wrote:
>  > Please don't post new versions in-reply-to, and add lore links to
>  > the previous version in the changelog.  
> 
> Will do. Is it okay to only include the last version, or should I 
> collect them going back to v1?

All.

> > On Tue, 4 Feb 2025 10:37:54 +0100 Csókás, Bence wrote:  
> >> For instance, as of now, `fec_stop()` does not check for
> >> `FEC_QUIRK_NO_HARD_RESET`, meaning the MII/RMII mode is cleared on eg.
> >> a PM power-down event; and `fec_restart()` missed the refactor renaming
> >> the "magic" constant `1` to `FEC_ECR_RESET`.  
> > 
> > Laurent responded to v1 saying this was intentional. Please give more
> > details on what problem you're seeing and on what platforms. Otherwise
> > this is not a fix but refactoring.  
> 
> True, but he also said:
> On 2025. 01. 21. 17:09, Badel, Laurent wrote:
>  > 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.  
> 
> I understand he is cautious, but I'd argue that the fact that two people 
> already posted Reviewed-by: (not counting Simon, who since withdrew it), 
> means that others also agree that we should err on the OTHER side of 
> caution, and do the check in both cases. He also mentions that the 
> reason he didn't do the check in `fec_stop()` was that he believed that 
> the only time that gets called is on driver/interface remove, but that 
> is not the case, as I outlined in the message already.

That's a bit of a he said, she said. Either you're see a problem 
and you can describe clearly the behavior you see, and on what
platform. Or you're just improving the code speculatively, and 
it's not a fix. The patch as is would end up in stable.

To be clear, nobody is against the patch itself, the question is
whether its a fix or refactoring.