[PATCH RFC 0/5] net: stmmac: fix setting RE and TE inappropriately

[PATCH RFC 0/5] net: stmmac: fix setting RE and TE inappropriately

[Russell King (Oracle)]

Hi,

Note: this series depends on "net: stmmac: fix resume failures due to
RX clock".

This series addresses inappropriate setting of the receive and transmit
enables in the GMAC control register.

The databook is clear for the receive enable, that this should not be
set until the initialisation of the MAC and DMA has been completed.
The previous RFC patch series ("net: stmmac: fix resume failures due to
RX clock") which moves phylink_resume() solves that, but we are left
with these enables being set when the link is down. This is not correct.

Sadly, when XDP support was added, new calls to netif_carrier_on() and
netif_carrier_off() were added, which are incorrect in drivers that
make use of phylink - by doing so, the driver has no guarantee that
the .mac_link_up() and .mac_link_down() methods will be called in
sequence anymore.

Solving this requires a number of changes. First, fixing XDP to call
into phylink to bring the link down. This removes a source of
phylink method de-sync mentioned above.

Then we remove the racy teardown in the driver's remove method -
nothing should be torn down while userspace still has access to the
netdev.

Then we remove an unnecessary call to disable the enable bits in
the .ndo_stop() method.

Finally, we remove manipulation of the RE and TE bits from the
start_tx(), stop_tx(), start_rx() and stop_rx() methods.

 drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c   |  8 --------
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 12 ------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 22 ++++++----------------
 3 files changed, 6 insertions(+), 36 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Russell King (Oracle)]

Phylink does not permit drivers to mess with the netif carrier, as
this will de-synchronise phylink with the MAC driver. Moreover,
setting and clearing the TE and RE bits via stmmac_mac_set() in this
path is also wrong as the link may not be up.

Replace the netif_carrier_on(), netif_carrier_off() and
stmmac_mac_set() calls with the appropriate phylink_start() and
phylink_stop() calls, thereby allowing phylink to manage the netif
carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
methods.

Note that RE should only be set after the DMA is ready to avoid the
receive FIFO between the MAC and DMA blocks overflowing, so
phylink_start() needs to be placed after DMA has been started.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 13796b1c8d7c..84d8b1c9f6d4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6887,6 +6887,8 @@ void stmmac_xdp_release(struct net_device *dev)
 	/* Ensure tx function is not running */
 	netif_tx_disable(dev);
 
+	phylink_stop(priv->phylink);
+
 	/* Disable NAPI process */
 	stmmac_disable_all_queues(priv);
 
@@ -6902,14 +6904,10 @@ void stmmac_xdp_release(struct net_device *dev)
 	/* Release and free the Rx/Tx resources */
 	free_dma_desc_resources(priv, &priv->dma_conf);
 
-	/* Disable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, false);
-
 	/* set trans_start so we don't get spurious
 	 * watchdogs during reset
 	 */
 	netif_trans_update(dev);
-	netif_carrier_off(dev);
 }
 
 int stmmac_xdp_open(struct net_device *dev)
@@ -6992,25 +6990,25 @@ int stmmac_xdp_open(struct net_device *dev)
 		tx_q->txtimer.function = stmmac_tx_timer;
 	}
 
-	/* Enable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, true);
-
 	/* Start Rx & Tx DMA Channels */
 	stmmac_start_all_dma(priv);
 
+	phylink_start(priv->phylink);
+
 	ret = stmmac_request_irq(dev);
 	if (ret)
 		goto irq_error;
 
 	/* Enable NAPI process*/
 	stmmac_enable_all_queues(priv);
-	netif_carrier_on(dev);
 	netif_tx_start_all_queues(dev);
 	stmmac_enable_all_dma_irq(priv);
 
 	return 0;
 
 irq_error:
+	phylink_stop(priv->phylink);
+
 	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
 		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
 
-- 
2.30.2

Re: [PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Andrew Lunn]

On Thu, Feb 27, 2025 at 03:05:02PM +0000, Russell King (Oracle) wrote:
> Phylink does not permit drivers to mess with the netif carrier, as
> this will de-synchronise phylink with the MAC driver. Moreover,
> setting and clearing the TE and RE bits via stmmac_mac_set() in this
> path is also wrong as the link may not be up.
> 
> Replace the netif_carrier_on(), netif_carrier_off() and
> stmmac_mac_set() calls with the appropriate phylink_start() and
> phylink_stop() calls, thereby allowing phylink to manage the netif
> carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
> methods.
> 
> Note that RE should only be set after the DMA is ready to avoid the
> receive FIFO between the MAC and DMA blocks overflowing, so
> phylink_start() needs to be placed after DMA has been started.

Sorry, i don't know enough about XDP to review this :-(

	Andrew

Re: [PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Russell King (Oracle)]

On Thu, Feb 27, 2025 at 11:27:57PM +0100, Andrew Lunn wrote:
> On Thu, Feb 27, 2025 at 03:05:02PM +0000, Russell King (Oracle) wrote:
> > Phylink does not permit drivers to mess with the netif carrier, as
> > this will de-synchronise phylink with the MAC driver. Moreover,
> > setting and clearing the TE and RE bits via stmmac_mac_set() in this
> > path is also wrong as the link may not be up.
> > 
> > Replace the netif_carrier_on(), netif_carrier_off() and
> > stmmac_mac_set() calls with the appropriate phylink_start() and
> > phylink_stop() calls, thereby allowing phylink to manage the netif
> > carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
> > methods.
> > 
> > Note that RE should only be set after the DMA is ready to avoid the
> > receive FIFO between the MAC and DMA blocks overflowing, so
> > phylink_start() needs to be placed after DMA has been started.
> 
> Sorry, i don't know enough about XDP to review this :-(

I suspect there aren't many people who could review it.

However, delving into the history, it seems that this commit was
responsible for introducing stmmac_xdp_{open,release}, thus
intrducing the fidding of the netif carrier which is prohibited
by phylink:

commit ac746c8520d9d056b6963ecca8ff1da9929d02f1
Author: Ong Boon Leong <boon.leong.ong@intel.com>
Date:   Thu Nov 11 22:39:49 2021 +0800

    net: stmmac: enhance XDP ZC driver level switching performance

but that commit was wrong for this very reason.

Didn't phylib used to not renegotiate the link if nothing changed
across a phy_stop()..phy_start() ?

I'm wondering whether my commit is in essence reverting this commit.


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Furong Xu]

On Thu, 27 Feb 2025 15:05:02 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Phylink does not permit drivers to mess with the netif carrier, as
> this will de-synchronise phylink with the MAC driver. Moreover,
> setting and clearing the TE and RE bits via stmmac_mac_set() in this
> path is also wrong as the link may not be up.
> 
> Replace the netif_carrier_on(), netif_carrier_off() and
> stmmac_mac_set() calls with the appropriate phylink_start() and
> phylink_stop() calls, thereby allowing phylink to manage the netif
> carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
> methods.
> 
> Note that RE should only be set after the DMA is ready to avoid the
> receive FIFO between the MAC and DMA blocks overflowing, so
> phylink_start() needs to be placed after DMA has been started.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 13796b1c8d7c..84d8b1c9f6d4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6887,6 +6887,8 @@ void stmmac_xdp_release(struct net_device *dev)
>  	/* Ensure tx function is not running */
>  	netif_tx_disable(dev);
>  
> +	phylink_stop(priv->phylink);
> +
>  	/* Disable NAPI process */
>  	stmmac_disable_all_queues(priv);
>  
> @@ -6902,14 +6904,10 @@ void stmmac_xdp_release(struct net_device *dev)
>  	/* Release and free the Rx/Tx resources */
>  	free_dma_desc_resources(priv, &priv->dma_conf);
>  
> -	/* Disable the MAC Rx/Tx */
> -	stmmac_mac_set(priv, priv->ioaddr, false);
> -
>  	/* set trans_start so we don't get spurious
>  	 * watchdogs during reset
>  	 */
>  	netif_trans_update(dev);
> -	netif_carrier_off(dev);
>  }
>  
>  int stmmac_xdp_open(struct net_device *dev)
> @@ -6992,25 +6990,25 @@ int stmmac_xdp_open(struct net_device *dev)
>  		tx_q->txtimer.function = stmmac_tx_timer;
>  	}
>  
> -	/* Enable the MAC Rx/Tx */
> -	stmmac_mac_set(priv, priv->ioaddr, true);
> -
>  	/* Start Rx & Tx DMA Channels */
>  	stmmac_start_all_dma(priv);
>  
> +	phylink_start(priv->phylink);
> +
>  	ret = stmmac_request_irq(dev);
>  	if (ret)
>  		goto irq_error;
>  
>  	/* Enable NAPI process*/
>  	stmmac_enable_all_queues(priv);
> -	netif_carrier_on(dev);
>  	netif_tx_start_all_queues(dev);
>  	stmmac_enable_all_dma_irq(priv);
>  
>  	return 0;
>  
>  irq_error:
> +	phylink_stop(priv->phylink);
> +
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
>  

XDP programs work like a charm both before and after this patch.

Tested-by: Furong Xu <0x1207@gmail.com>

Re: [PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Andrew Lunn]

On Fri, Feb 28, 2025 at 03:31:22PM +0800, Furong Xu wrote:
> On Thu, 27 Feb 2025 15:05:02 +0000
> "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> 
> > Phylink does not permit drivers to mess with the netif carrier, as
> > this will de-synchronise phylink with the MAC driver. Moreover,
> > setting and clearing the TE and RE bits via stmmac_mac_set() in this
> > path is also wrong as the link may not be up.
> > 
> > Replace the netif_carrier_on(), netif_carrier_off() and
> > stmmac_mac_set() calls with the appropriate phylink_start() and
> > phylink_stop() calls, thereby allowing phylink to manage the netif
> > carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
> > methods.
> > 
> > Note that RE should only be set after the DMA is ready to avoid the
> > receive FIFO between the MAC and DMA blocks overflowing, so
> > phylink_start() needs to be placed after DMA has been started.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
 
> XDP programs work like a charm both before and after this patch.
> 
> Tested-by: Furong Xu <0x1207@gmail.com>

Thanks for testing this.

Could you give a little details of how you actually tested it?

The issues here is, when is the link set admin up, requiring that
phylink triggers an autoneg etc.

For plain old TCP/IP, you at some point use:

ip link set eth42 up

which will cause the core to call the drivers open() method, which
then triggers phylnk.

The carrier manipulation which this code replaces seems to suggest you
can load an XDP program while the interface is admin down, and that
action of loading the program will implicitly set the carrier up.

Did you test loading an XDP program on an interface which is admin
down?

	Andrew

Re: [PATCH RFC net-next 1/5] net: stmmac: call phylink_start() and phylink_stop() in XDP functions

[Russell King (Oracle)]

On Fri, Feb 28, 2025 at 02:14:29PM +0100, Andrew Lunn wrote:
> On Fri, Feb 28, 2025 at 03:31:22PM +0800, Furong Xu wrote:
> > On Thu, 27 Feb 2025 15:05:02 +0000
> > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > 
> > > Phylink does not permit drivers to mess with the netif carrier, as
> > > this will de-synchronise phylink with the MAC driver. Moreover,
> > > setting and clearing the TE and RE bits via stmmac_mac_set() in this
> > > path is also wrong as the link may not be up.
> > > 
> > > Replace the netif_carrier_on(), netif_carrier_off() and
> > > stmmac_mac_set() calls with the appropriate phylink_start() and
> > > phylink_stop() calls, thereby allowing phylink to manage the netif
> > > carrier and TE/RE bits through the .mac_link_up() and .mac_link_down()
> > > methods.
> > > 
> > > Note that RE should only be set after the DMA is ready to avoid the
> > > receive FIFO between the MAC and DMA blocks overflowing, so
> > > phylink_start() needs to be placed after DMA has been started.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>  
> > XDP programs work like a charm both before and after this patch.
> > 
> > Tested-by: Furong Xu <0x1207@gmail.com>
> 
> Thanks for testing this.
> 
> Could you give a little details of how you actually tested it?
> 
> The issues here is, when is the link set admin up, requiring that
> phylink triggers an autoneg etc.
> 
> For plain old TCP/IP, you at some point use:
> 
> ip link set eth42 up
> 
> which will cause the core to call the drivers open() method, which
> then triggers phylnk.
> 
> The carrier manipulation which this code replaces seems to suggest you
> can load an XDP program while the interface is admin down, and that
> action of loading the program will implicitly set the carrier up.

It won't. See
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c::stmmac_xdp_set_prog():

        if_running = netif_running(dev);

        need_update = !!priv->xdp_prog != !!prog;

        if (if_running && need_update)
                stmmac_xdp_release(dev);

	...

        if (if_running && need_update)
                stmmac_xdp_open(dev);

stmmac_xdp_open() and stmmac_xdp_release() will only be called if
netif_running() returns true as explained yesterday (but maybe
without enough detail). This tests __LINK_STATE_START in dev->state,
which is set just before calling .ndo_open(), and cleared if it fails
or before calling .ndo_stop().

Thus, netif_running() indicates whether the net core thinks the
device is adminsitratively "up".

Therefore, in the old code, if the programme is changed while the
device is administratively down, netif_running() will return false,
if_running will be false, and neither stmmac_xdp_release() nor
stmmac_xdp_open() will be called.

Hope that addresses your concern.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 2/5] net: stmmac: remove redundant racy tear-down in stmmac_dvr_remove()

[Russell King (Oracle)]

While the network device is registered, it is published to userspace,
and thus userspace can change its state. This means calling
functions such as stmmac_stop_all_dma() and stmmac_mac_set() are
racy.

Moreover, unregister_netdev() will unpublish the network device, and
then if appropriate call the .ndo_stop() method, which is
stmmac_release(). This will first call phylink_stop() which will
synchronously take the link down, resulting in stmmac_mac_link_down()
and stmmac_mac_set(, false) being called.

stmmac_release() will also call stmmac_stop_all_dma().

Consequently, neither of these two functions need to called prior
to unregister_netdev() as that will safely call paths that will
result in this work being done if necessary.

Remove these redundant racy calls.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 84d8b1c9f6d4..9462d05c40c8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7757,8 +7757,6 @@ void stmmac_dvr_remove(struct device *dev)
 
 	pm_runtime_get_sync(dev);
 
-	stmmac_stop_all_dma(priv);
-	stmmac_mac_set(priv, priv->ioaddr, false);
 	unregister_netdev(ndev);
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.30.2

Re: [PATCH RFC net-next 2/5] net: stmmac: remove redundant racy tear-down in stmmac_dvr_remove()

[Andrew Lunn]

On Thu, Feb 27, 2025 at 03:05:07PM +0000, Russell King (Oracle) wrote:
> While the network device is registered, it is published to userspace,
> and thus userspace can change its state. This means calling
> functions such as stmmac_stop_all_dma() and stmmac_mac_set() are
> racy.
> 
> Moreover, unregister_netdev() will unpublish the network device, and
> then if appropriate call the .ndo_stop() method, which is
> stmmac_release(). This will first call phylink_stop() which will
> synchronously take the link down, resulting in stmmac_mac_link_down()
> and stmmac_mac_set(, false) being called.
> 
> stmmac_release() will also call stmmac_stop_all_dma().
> 
> Consequently, neither of these two functions need to called prior
> to unregister_netdev() as that will safely call paths that will
> result in this work being done if necessary.
> 
> Remove these redundant racy calls.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 2/5] net: stmmac: remove redundant racy tear-down in stmmac_dvr_remove()

[Furong Xu]

On Thu, 27 Feb 2025 15:05:07 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> While the network device is registered, it is published to userspace,
> and thus userspace can change its state. This means calling
> functions such as stmmac_stop_all_dma() and stmmac_mac_set() are
> racy.
> 
> Moreover, unregister_netdev() will unpublish the network device, and
> then if appropriate call the .ndo_stop() method, which is
> stmmac_release(). This will first call phylink_stop() which will
> synchronously take the link down, resulting in stmmac_mac_link_down()
> and stmmac_mac_set(, false) being called.
> 
> stmmac_release() will also call stmmac_stop_all_dma().
> 
> Consequently, neither of these two functions need to called prior
> to unregister_netdev() as that will safely call paths that will
> result in this work being done if necessary.
> 
> Remove these redundant racy calls.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 84d8b1c9f6d4..9462d05c40c8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7757,8 +7757,6 @@ void stmmac_dvr_remove(struct device *dev)
>  
>  	pm_runtime_get_sync(dev);
>  
> -	stmmac_stop_all_dma(priv);
> -	stmmac_mac_set(priv, priv->ioaddr, false);
>  	unregister_netdev(ndev);
>  
>  #ifdef CONFIG_DEBUG_FS

We always build stmmac driver as built-in.
Tried to build stmmac driver as a module, but some complicated dependencies were
reported in our down-stream kernel :(
I can not test this patch, so:

Reviewed-by: Furong Xu <0x1207@gmail.com>

[PATCH RFC net-next 3/5] net: stmmac: remove unnecessary stmmac_mac_set() in stmmac_release()

[Russell King (Oracle)]

stmmac_release() calls phylink_stop() and then goes on to call
stmmac_mac_set(, false). However, phylink_stop() will call
stmmac_mac_link_down() before returning, which will do this work.
Remove this unnecessary call.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9462d05c40c8..e47b702fb9f9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4120,9 +4120,6 @@ static int stmmac_release(struct net_device *dev)
 	/* Release and free the Rx/Tx resources */
 	free_dma_desc_resources(priv, &priv->dma_conf);
 
-	/* Disable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, false);
-
 	/* Powerdown Serdes if there is */
 	if (priv->plat->serdes_powerdown)
 		priv->plat->serdes_powerdown(dev, priv->plat->bsp_priv);
-- 
2.30.2

Re: [PATCH RFC net-next 3/5] net: stmmac: remove unnecessary stmmac_mac_set() in stmmac_release()

[Andrew Lunn]

On Thu, Feb 27, 2025 at 03:05:12PM +0000, Russell King (Oracle) wrote:
> stmmac_release() calls phylink_stop() and then goes on to call
> stmmac_mac_set(, false). However, phylink_stop() will call
> stmmac_mac_link_down() before returning, which will do this work.
> Remove this unnecessary call.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 3/5] net: stmmac: remove unnecessary stmmac_mac_set() in stmmac_release()

[Furong Xu]

On Thu, 27 Feb 2025 15:05:12 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> stmmac_release() calls phylink_stop() and then goes on to call
> stmmac_mac_set(, false). However, phylink_stop() will call
> stmmac_mac_link_down() before returning, which will do this work.
> Remove this unnecessary call.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9462d05c40c8..e47b702fb9f9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4120,9 +4120,6 @@ static int stmmac_release(struct net_device *dev)
>  	/* Release and free the Rx/Tx resources */
>  	free_dma_desc_resources(priv, &priv->dma_conf);
>  
> -	/* Disable the MAC Rx/Tx */
> -	stmmac_mac_set(priv, priv->ioaddr, false);
> -
>  	/* Powerdown Serdes if there is */
>  	if (priv->plat->serdes_powerdown)
>  		priv->plat->serdes_powerdown(dev, priv->plat->bsp_priv);

Tested-by: Furong Xu <0x1207@gmail.com>

[PATCH RFC net-next 4/5] net: stmmac: remove _RE and _TE in (start|stop)_(tx|rx)() methods

[Russell King (Oracle)]

Remove fiddling with _TE and _RE in the GMAC control register in the
start_tx/stop_tx/start_rx/stop_rx() methods as this should be handled
by phylink and not during initialisation.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c   |  8 --------
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 12 ------------
 2 files changed, 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
index 57c03d491774..61584b569be7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
@@ -50,10 +50,6 @@ void dwmac4_dma_start_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
 
 	value |= DMA_CONTROL_ST;
 	writel(value, ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));
-
-	value = readl(ioaddr + GMAC_CONFIG);
-	value |= GMAC_CONFIG_TE;
-	writel(value, ioaddr + GMAC_CONFIG);
 }
 
 void dwmac4_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
@@ -77,10 +73,6 @@ void dwmac4_dma_start_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
 	value |= DMA_CONTROL_SR;
 
 	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
-
-	value = readl(ioaddr + GMAC_CONFIG);
-	value |= GMAC_CONFIG_RE;
-	writel(value, ioaddr + GMAC_CONFIG);
 }
 
 void dwmac4_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 7840bc403788..cba12edc1477 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -288,10 +288,6 @@ static void dwxgmac2_dma_start_tx(struct stmmac_priv *priv,
 	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
 	value |= XGMAC_TXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_TX_CONFIG);
-	value |= XGMAC_CONFIG_TE;
-	writel(value, ioaddr + XGMAC_TX_CONFIG);
 }
 
 static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
@@ -302,10 +298,6 @@ static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
 	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
 	value &= ~XGMAC_TXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_TX_CONFIG);
-	value &= ~XGMAC_CONFIG_TE;
-	writel(value, ioaddr + XGMAC_TX_CONFIG);
 }
 
 static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
@@ -316,10 +308,6 @@ static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
 	value = readl(ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
 	value |= XGMAC_RXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_RX_CONFIG);
-	value |= XGMAC_CONFIG_RE;
-	writel(value, ioaddr + XGMAC_RX_CONFIG);
 }
 
 static void dwxgmac2_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
-- 
2.30.2

Re: [PATCH RFC net-next 4/5] net: stmmac: remove _RE and _TE in (start|stop)_(tx|rx)() methods

[Andrew Lunn]

On Thu, Feb 27, 2025 at 03:05:17PM +0000, Russell King (Oracle) wrote:
> Remove fiddling with _TE and _RE in the GMAC control register in the
> start_tx/stop_tx/start_rx/stop_rx() methods as this should be handled
> by phylink and not during initialisation.

Maybe the commit message could be extended. 'by phylink', i think you
actually mean via the stmmac_mac_link_up() and stmmac_mac_link_down()
callbacks which call stmmac_mac_set().

Bit of a nitpick, so:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 4/5] net: stmmac: remove _RE and _TE in (start|stop)_(tx|rx)() methods

[Furong Xu]

On Thu, 27 Feb 2025 15:05:17 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Remove fiddling with _TE and _RE in the GMAC control register in the
> start_tx/stop_tx/start_rx/stop_rx() methods as this should be handled
> by phylink and not during initialisation.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c   |  8 --------
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 12 ------------
>  2 files changed, 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
> index 57c03d491774..61584b569be7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
> @@ -50,10 +50,6 @@ void dwmac4_dma_start_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
>  
>  	value |= DMA_CONTROL_ST;
>  	writel(value, ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));
> -
> -	value = readl(ioaddr + GMAC_CONFIG);
> -	value |= GMAC_CONFIG_TE;
> -	writel(value, ioaddr + GMAC_CONFIG);
>  }
>  
>  void dwmac4_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
> @@ -77,10 +73,6 @@ void dwmac4_dma_start_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
>  	value |= DMA_CONTROL_SR;
>  
>  	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
> -
> -	value = readl(ioaddr + GMAC_CONFIG);
> -	value |= GMAC_CONFIG_RE;
> -	writel(value, ioaddr + GMAC_CONFIG);
>  }
>  
>  void dwmac4_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index 7840bc403788..cba12edc1477 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> @@ -288,10 +288,6 @@ static void dwxgmac2_dma_start_tx(struct stmmac_priv *priv,
>  	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
>  	value |= XGMAC_TXST;
>  	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> -
> -	value = readl(ioaddr + XGMAC_TX_CONFIG);
> -	value |= XGMAC_CONFIG_TE;
> -	writel(value, ioaddr + XGMAC_TX_CONFIG);
>  }
>  
>  static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
> @@ -302,10 +298,6 @@ static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
>  	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
>  	value &= ~XGMAC_TXST;
>  	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> -
> -	value = readl(ioaddr + XGMAC_TX_CONFIG);
> -	value &= ~XGMAC_CONFIG_TE;
> -	writel(value, ioaddr + XGMAC_TX_CONFIG);
>  }
>  
>  static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
> @@ -316,10 +308,6 @@ static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
>  	value = readl(ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
>  	value |= XGMAC_RXST;
>  	writel(value, ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
> -
> -	value = readl(ioaddr + XGMAC_RX_CONFIG);
> -	value |= XGMAC_CONFIG_RE;
> -	writel(value, ioaddr + XGMAC_RX_CONFIG);
>  }
>  
>  static void dwxgmac2_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,

Tested-by: Furong Xu <0x1207@gmail.com>

[PATCH RFC net-next 5/5] net: stmmac: leave enabling RE and TE to stmmac_mac_link_up()

[Russell King (Oracle)]

We only need to enable the MAC receiver and transmiter only when the
link has come up.

With commit "net: stmmac: move phylink_resume() after resume setup
is complete" we move the race between stmmac_mac_link_up() and
stmmac_hw_setup(), ensuring that stmmac_mac_link_up() happens
afterwards. This patch is a pre-requisit of this change.

Remove the unnecessary call to stmmac_mac_set(, true) in
stmmac_hw_setup().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e47b702fb9f9..c80952349eb7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3506,9 +3506,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 		priv->hw->rx_csum = 0;
 	}
 
-	/* Enable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, true);
-
 	/* Set the HW DMA mode and the COE */
 	stmmac_dma_operation_mode(priv);
 
-- 
2.30.2

Re: [PATCH RFC net-next 5/5] net: stmmac: leave enabling RE and TE to stmmac_mac_link_up()

[Furong Xu]

On Thu, 27 Feb 2025 15:05:22 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> We only need to enable the MAC receiver and transmiter only when the
> link has come up.
> 
> With commit "net: stmmac: move phylink_resume() after resume setup
> is complete" we move the race between stmmac_mac_link_up() and
> stmmac_hw_setup(), ensuring that stmmac_mac_link_up() happens
> afterwards. This patch is a pre-requisit of this change.
> 
> Remove the unnecessary call to stmmac_mac_set(, true) in
> stmmac_hw_setup().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e47b702fb9f9..c80952349eb7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3506,9 +3506,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
>  		priv->hw->rx_csum = 0;
>  	}
>  
> -	/* Enable the MAC Rx/Tx */
> -	stmmac_mac_set(priv, priv->ioaddr, true);
> -
>  	/* Set the HW DMA mode and the COE */
>  	stmmac_dma_operation_mode(priv);
>  

Tested-by: Furong Xu <0x1207@gmail.com>