[PATCH net-next v2 0/4] net: stmmac: fix setting RE and TE inappropriately

[PATCH net-next v2 0/4] net: stmmac: fix setting RE and TE inappropriately

[Russell King (Oracle)]

Hi,

This series addresses inappropriate setting of the receive and transmit
enables in the GMAC control register identified back in
https://lore.kernel.org/r/Z8BboX9RxZBSXRr1@shell.armlinux.org.uk

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. This is fixed in patch 1.

We remove manipulation of the RE and TE bits from the start_tx(),
stop_tx(), start_rx() and stop_rx() methods for each core.

Finally, we remove the stmmac_mac_set() call from stmmac_hw_setup().

v2: add phylink_carrier_*() functions and use these instead to avoid
calling phy_stop()/phy_start(), which may bounce the physical link.

 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  | 23 +++++-----
 drivers/net/phy/phylink.c                          | 50 ++++++++++++++++++++++
 include/linux/phylink.h                            |  3 ++
 5 files changed, 65 insertions(+), 31 deletions(-)

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

[PATCH net-next v2 1/4] net: phylink: add ability to block carrier up

[Russell King (Oracle)]

Network drivers such as stmmac need to quiesce the network device prior
to changing the DMA configuration. Currently, they do this by calling
netif_carrier_off() to stop the network stack pushing packets, but this
is incompatible with phylink.

Provide a pair of functions to allow the software link state to be
blocked and brought down if necessary, and restored afterwards.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 50 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 53 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0faa3d97e06b..d522e12f89e8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -26,6 +26,7 @@
 
 enum {
 	PHYLINK_DISABLE_STOPPED,
+	PHYLINK_DISABLE_CARRIER,
 	PHYLINK_DISABLE_LINK,
 	PHYLINK_DISABLE_MAC_WOL,
 
@@ -83,6 +84,7 @@ struct phylink {
 	bool mac_tx_clk_stop;
 	u32 mac_tx_lpi_timer;
 	u8 mac_rx_clk_stop_blocked;
+	u8 mac_carrier_blocked;
 
 	struct sfp_bus *sfp_bus;
 	bool sfp_may_have_phy;
@@ -2456,6 +2458,54 @@ void phylink_stop(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_stop);
 
+/**
+ * phylink_carrier_block() - unblock carrier state
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Disable the software link, which will call mac_link_down(). This is to
+ * allow network drivers to safely adjust e.g. DMA settings with the
+ * device idle. All calls to phylink_carrier_block() must be balanced by
+ * the appropriate number of calls to phylink_carrier_unblock().
+ */
+void phylink_carrier_block(struct phylink *pl)
+{
+	ASSERT_RTNL();
+
+	if (pl->mac_carrier_blocked == U8_MAX) {
+		phylink_warn(pl, "%s called too many times - ignoring\n",
+			     __func__);
+		dump_stack();
+		return;
+	}
+
+	if (pl->mac_carrier_blocked++ == 0)
+		phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_CARRIER);
+}
+EXPORT_SYMBOL_GPL(phylink_carrier_block);
+
+/**
+ * phylink_carrier_unblock() - unblock carrier state
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * All calls to phylink_carrier_block() must be balanced with a corresponding
+ * call to phylink_carrier_unblock() to restore the carrier state.
+ */
+void phylink_carrier_unblock(struct phylink *pl)
+{
+	ASSERT_RTNL();
+
+	if (pl->mac_carrier_blocked == 0) {
+		phylink_warn(pl, "%s called too many times - ignoring\n",
+			     __func__);
+		dump_stack();
+		return;
+	}
+
+	if (--pl->mac_carrier_blocked == 0)
+		phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_CARRIER);
+}
+EXPORT_SYMBOL_GPL(phylink_carrier_unblock);
+
 /**
  * phylink_rx_clk_stop_block() - block PHY ability to stop receive clock in LPI
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 30659b615fca..a48032561183 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -715,6 +715,9 @@ int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
 void phylink_start(struct phylink *);
 void phylink_stop(struct phylink *);
 
+void phylink_carrier_block(struct phylink *);
+void phylink_carrier_unblock(struct phylink *);
+
 void phylink_rx_clk_stop_block(struct phylink *);
 void phylink_rx_clk_stop_unblock(struct phylink *);
 
-- 
2.30.2

Re: [PATCH net-next v2 1/4] net: phylink: add ability to block carrier up

[Andrew Lunn]

> +/**
> + * phylink_carrier_block() - unblock carrier state

It seems like a cut/paste error, unlock should be block ?

    Andrew

---
pw-bot: cr

[PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() 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_carrier_block() and
phylink_carrier_unblock() calls, thereby allowing phylink to manage the
netif carrier and TE/RE bits through the .mac_link_up() and
.mac_link_down() methods.

This change will have the side effect of printing link messages to
the kernel log, even though the physical link hasn't changed state.
This matches the carrier state that userspace sees, which has always
"bounced".

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>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f59a2363f150..ac27ea679b23 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
 	/* Ensure tx function is not running */
 	netif_tx_disable(dev);
 
+	/* Take down the software link. stmmac_xdp_open() must be called after
+	 * this function to release this block.
+	 */
+	phylink_carrier_block(priv->phylink);
+
 	/* Disable NAPI process */
 	stmmac_disable_all_queues(priv);
 
@@ -6937,14 +6942,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)
@@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
 		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	}
 
-	/* Enable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, true);
-
 	/* Start Rx & Tx DMA Channels */
 	stmmac_start_all_dma(priv);
 
+	/* Allow phylink to bring the software link back up.
+	 * stmmac_xdp_release() must have been called prior to this.
+	 */
+	phylink_carrier_unblock(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 net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions

[Andrew Lunn]

On Fri, May 02, 2025 at 02:35:36PM +0100, 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_carrier_block() and
> phylink_carrier_unblock() calls, thereby allowing phylink to manage the
> netif carrier and TE/RE bits through the .mac_link_up() and
> .mac_link_down() methods.
> 
> This change will have the side effect of printing link messages to
> the kernel log, even though the physical link hasn't changed state.
> This matches the carrier state that userspace sees, which has always
> "bounced".
> 
> 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>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f59a2363f150..ac27ea679b23 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
>  	/* Ensure tx function is not running */
>  	netif_tx_disable(dev);
>  
> +	/* Take down the software link. stmmac_xdp_open() must be called after
> +	 * this function to release this block.
> +	 */
> +	phylink_carrier_block(priv->phylink);
> +
>  	/* Disable NAPI process */
>  	stmmac_disable_all_queues(priv);
>  
> @@ -6937,14 +6942,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)
> @@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
>  		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	}
>  
> -	/* Enable the MAC Rx/Tx */
> -	stmmac_mac_set(priv, priv->ioaddr, true);
> -
>  	/* Start Rx & Tx DMA Channels */
>  	stmmac_start_all_dma(priv);
>  
> +	/* Allow phylink to bring the software link back up.
> +	 * stmmac_xdp_release() must have been called prior to this.
> +	 */

This is counter intuitive. Why is release called before open?

Looking into stmmac_xdp_set_prog() i think i get it. Even if there is
not a running XDP prog, stmmac_xdp_release() is called, and then
stmmac_xdp_open().

Maybe these two functions need better names? prepare and commit?

      Andrew

Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions

[Russell King (Oracle)]

On Fri, May 02, 2025 at 05:29:21PM +0200, Andrew Lunn wrote:
> On Fri, May 02, 2025 at 02:35:36PM +0100, 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_carrier_block() and
> > phylink_carrier_unblock() calls, thereby allowing phylink to manage the
> > netif carrier and TE/RE bits through the .mac_link_up() and
> > .mac_link_down() methods.
> > 
> > This change will have the side effect of printing link messages to
> > the kernel log, even though the physical link hasn't changed state.
> > This matches the carrier state that userspace sees, which has always
> > "bounced".
> > 
> > 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>
> > ---
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f59a2363f150..ac27ea679b23 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
> >  	/* Ensure tx function is not running */
> >  	netif_tx_disable(dev);
> >  
> > +	/* Take down the software link. stmmac_xdp_open() must be called after
> > +	 * this function to release this block.
> > +	 */
> > +	phylink_carrier_block(priv->phylink);
> > +
> >  	/* Disable NAPI process */
> >  	stmmac_disable_all_queues(priv);
> >  
> > @@ -6937,14 +6942,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)
> > @@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
> >  		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >  	}
> >  
> > -	/* Enable the MAC Rx/Tx */
> > -	stmmac_mac_set(priv, priv->ioaddr, true);
> > -
> >  	/* Start Rx & Tx DMA Channels */
> >  	stmmac_start_all_dma(priv);
> >  
> > +	/* Allow phylink to bring the software link back up.
> > +	 * stmmac_xdp_release() must have been called prior to this.
> > +	 */
> 
> This is counter intuitive. Why is release called before open?

Indeed - and that should've been caught in the review where XDP was
being added.

> Looking into stmmac_xdp_set_prog() i think i get it. Even if there is
> not a running XDP prog, stmmac_xdp_release() is called, and then
> stmmac_xdp_open().

If there is a change of "do we have an XDP prog" state, then
stmmac_xdp_release() is called to free all the current contexts to
do with queue/descriptor management, and then stmmac_xdp_open() is
called thereafter. These are doing a subset of .ndo_open/.ndo_release
and I think that's where they're getting their naming from.

The only possible sequence is:

	stmmac_open()
then, on each XDP prog addition or removal, but not replacement:
		stmmac_xdp_release()
		stmmac_xdp_open()
finally,
	stmmac_release()

> Maybe these two functions need better names? prepare and commit?

Yes, it's all counter intuitive, and there are various things about the
XDP code that make it hard to follow.

For example, stmmac_xdp_set_prog() leads you to think, because of the
way the need_update variable is set, that looking for references to
xdp_prog would show one where all the dependents are, but no, there's
stmmac_xdp_is_enabled(), which is nice and readable, but could've
been used in stmmac_xdp_set_prog() to make it more obvious what to
grep for.

Incidentally, if stmmac_xdp_open() fails to re-grab the interrupts,
then it calls phylink_stop(), stmmac_hw_teardown(), and
free_dma_desc_resources().

If one then set the interface administratively down, stmmac_release()
gets called, which again calls phylink_stop(), free_dma_desc_resources()
and stmmac_release_ptp().

stmmac_release_ptp() disables/unprepares clk_ptp_ref, and unregisters
the PTP stuff. stmmac_hw_teardown() also disables/unprepares
clk_ptp_ref, so we probably unbalance the clk API in this case...
and probably much other stuff.

Calling free_dma_desc_resources() twice calls functios such as 
free_dma_tx_desc_resources() twice, and it looks like that's not going
to be healthy, calling dma_free_coherent() with the same arguments,
double-releasing memory. Same for kfree(). Probably same for the RX
stuff.

Basically, if one messes with XDP in this driver, expect things to go
bang and kill the kernel if something goes wrong with the whole
xdp_release+xdp_open dance.

Honestly, this needs a rewrite, but I currently know nowt about XDP.

So, I'd suggest that the names of these functions is the least of the
problems here.

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

Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions

[Russell King (Oracle)]

On Fri, May 02, 2025 at 06:54:49PM +0100, Russell King (Oracle) wrote:
> On Fri, May 02, 2025 at 05:29:21PM +0200, Andrew Lunn wrote:
> > On Fri, May 02, 2025 at 02:35:36PM +0100, 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_carrier_block() and
> > > phylink_carrier_unblock() calls, thereby allowing phylink to manage the
> > > netif carrier and TE/RE bits through the .mac_link_up() and
> > > .mac_link_down() methods.
> > > 
> > > This change will have the side effect of printing link messages to
> > > the kernel log, even though the physical link hasn't changed state.
> > > This matches the carrier state that userspace sees, which has always
> > > "bounced".
> > > 
> > > 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>
> > > ---
> > >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index f59a2363f150..ac27ea679b23 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
> > >  	/* Ensure tx function is not running */
> > >  	netif_tx_disable(dev);
> > >  
> > > +	/* Take down the software link. stmmac_xdp_open() must be called after
> > > +	 * this function to release this block.
> > > +	 */
> > > +	phylink_carrier_block(priv->phylink);
> > > +
> > >  	/* Disable NAPI process */
> > >  	stmmac_disable_all_queues(priv);
> > >  
> > > @@ -6937,14 +6942,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)
> > > @@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
> > >  		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > >  	}
> > >  
> > > -	/* Enable the MAC Rx/Tx */
> > > -	stmmac_mac_set(priv, priv->ioaddr, true);
> > > -
> > >  	/* Start Rx & Tx DMA Channels */
> > >  	stmmac_start_all_dma(priv);
> > >  
> > > +	/* Allow phylink to bring the software link back up.
> > > +	 * stmmac_xdp_release() must have been called prior to this.
> > > +	 */
> > 
> > This is counter intuitive. Why is release called before open?
> 
> Indeed - and that should've been caught in the review where XDP was
> being added.
> 
> > Looking into stmmac_xdp_set_prog() i think i get it. Even if there is
> > not a running XDP prog, stmmac_xdp_release() is called, and then
> > stmmac_xdp_open().
> 
> If there is a change of "do we have an XDP prog" state, then
> stmmac_xdp_release() is called to free all the current contexts to
> do with queue/descriptor management, and then stmmac_xdp_open() is
> called thereafter. These are doing a subset of .ndo_open/.ndo_release
> and I think that's where they're getting their naming from.
> 
> The only possible sequence is:
> 
> 	stmmac_open()
> then, on each XDP prog addition or removal, but not replacement:
> 		stmmac_xdp_release()
> 		stmmac_xdp_open()
> finally,
> 	stmmac_release()
> 
> > Maybe these two functions need better names? prepare and commit?
> 
> Yes, it's all counter intuitive, and there are various things about the
> XDP code that make it hard to follow.
> 
> For example, stmmac_xdp_set_prog() leads you to think, because of the
> way the need_update variable is set, that looking for references to
> xdp_prog would show one where all the dependents are, but no, there's
> stmmac_xdp_is_enabled(), which is nice and readable, but could've
> been used in stmmac_xdp_set_prog() to make it more obvious what to
> grep for.
> 
> Incidentally, if stmmac_xdp_open() fails to re-grab the interrupts,
> then it calls phylink_stop(), stmmac_hw_teardown(), and
> free_dma_desc_resources().
> 
> If one then set the interface administratively down, stmmac_release()
> gets called, which again calls phylink_stop(), free_dma_desc_resources()
> and stmmac_release_ptp().
> 
> stmmac_release_ptp() disables/unprepares clk_ptp_ref, and unregisters
> the PTP stuff. stmmac_hw_teardown() also disables/unprepares
> clk_ptp_ref, so we probably unbalance the clk API in this case...
> and probably much other stuff.
> 
> Calling free_dma_desc_resources() twice calls functios such as 
> free_dma_tx_desc_resources() twice, and it looks like that's not going
> to be healthy, calling dma_free_coherent() with the same arguments,
> double-releasing memory. Same for kfree(). Probably same for the RX
> stuff.
> 
> Basically, if one messes with XDP in this driver, expect things to go
> bang and kill the kernel if something goes wrong with the whole
> xdp_release+xdp_open dance.
> 
> Honestly, this needs a rewrite, but I currently know nowt about XDP.
> 
> So, I'd suggest that the names of these functions is the least of the
> problems here.

Well, this series has been discarded from patchwork. Shrug. I won't be
posting another version, stmmac can remain broken. I don't have a
suggestion on better names for these functions.

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

Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions

[Russell King (Oracle)]

On Tue, May 06, 2025 at 09:36:57AM +0100, Russell King (Oracle) wrote:
> On Fri, May 02, 2025 at 06:54:49PM +0100, Russell King (Oracle) wrote:
> > On Fri, May 02, 2025 at 05:29:21PM +0200, Andrew Lunn wrote:
> > > On Fri, May 02, 2025 at 02:35:36PM +0100, 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_carrier_block() and
> > > > phylink_carrier_unblock() calls, thereby allowing phylink to manage the
> > > > netif carrier and TE/RE bits through the .mac_link_up() and
> > > > .mac_link_down() methods.
> > > > 
> > > > This change will have the side effect of printing link messages to
> > > > the kernel log, even though the physical link hasn't changed state.
> > > > This matches the carrier state that userspace sees, which has always
> > > > "bounced".
> > > > 
> > > > 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>
> > > > ---
> > > >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
> > > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index f59a2363f150..ac27ea679b23 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
> > > >  	/* Ensure tx function is not running */
> > > >  	netif_tx_disable(dev);
> > > >  
> > > > +	/* Take down the software link. stmmac_xdp_open() must be called after
> > > > +	 * this function to release this block.
> > > > +	 */
> > > > +	phylink_carrier_block(priv->phylink);
> > > > +
> > > >  	/* Disable NAPI process */
> > > >  	stmmac_disable_all_queues(priv);
> > > >  
> > > > @@ -6937,14 +6942,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)
> > > > @@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
> > > >  		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > > >  	}
> > > >  
> > > > -	/* Enable the MAC Rx/Tx */
> > > > -	stmmac_mac_set(priv, priv->ioaddr, true);
> > > > -
> > > >  	/* Start Rx & Tx DMA Channels */
> > > >  	stmmac_start_all_dma(priv);
> > > >  
> > > > +	/* Allow phylink to bring the software link back up.
> > > > +	 * stmmac_xdp_release() must have been called prior to this.
> > > > +	 */
> > > 
> > > This is counter intuitive. Why is release called before open?
> > 
> > Indeed - and that should've been caught in the review where XDP was
> > being added.
> > 
> > > Looking into stmmac_xdp_set_prog() i think i get it. Even if there is
> > > not a running XDP prog, stmmac_xdp_release() is called, and then
> > > stmmac_xdp_open().
> > 
> > If there is a change of "do we have an XDP prog" state, then
> > stmmac_xdp_release() is called to free all the current contexts to
> > do with queue/descriptor management, and then stmmac_xdp_open() is
> > called thereafter. These are doing a subset of .ndo_open/.ndo_release
> > and I think that's where they're getting their naming from.
> > 
> > The only possible sequence is:
> > 
> > 	stmmac_open()
> > then, on each XDP prog addition or removal, but not replacement:
> > 		stmmac_xdp_release()
> > 		stmmac_xdp_open()
> > finally,
> > 	stmmac_release()
> > 
> > > Maybe these two functions need better names? prepare and commit?
> > 
> > Yes, it's all counter intuitive, and there are various things about the
> > XDP code that make it hard to follow.
> > 
> > For example, stmmac_xdp_set_prog() leads you to think, because of the
> > way the need_update variable is set, that looking for references to
> > xdp_prog would show one where all the dependents are, but no, there's
> > stmmac_xdp_is_enabled(), which is nice and readable, but could've
> > been used in stmmac_xdp_set_prog() to make it more obvious what to
> > grep for.
> > 
> > Incidentally, if stmmac_xdp_open() fails to re-grab the interrupts,
> > then it calls phylink_stop(), stmmac_hw_teardown(), and
> > free_dma_desc_resources().
> > 
> > If one then set the interface administratively down, stmmac_release()
> > gets called, which again calls phylink_stop(), free_dma_desc_resources()
> > and stmmac_release_ptp().
> > 
> > stmmac_release_ptp() disables/unprepares clk_ptp_ref, and unregisters
> > the PTP stuff. stmmac_hw_teardown() also disables/unprepares
> > clk_ptp_ref, so we probably unbalance the clk API in this case...
> > and probably much other stuff.
> > 
> > Calling free_dma_desc_resources() twice calls functios such as 
> > free_dma_tx_desc_resources() twice, and it looks like that's not going
> > to be healthy, calling dma_free_coherent() with the same arguments,
> > double-releasing memory. Same for kfree(). Probably same for the RX
> > stuff.
> > 
> > Basically, if one messes with XDP in this driver, expect things to go
> > bang and kill the kernel if something goes wrong with the whole
> > xdp_release+xdp_open dance.
> > 
> > Honestly, this needs a rewrite, but I currently know nowt about XDP.
> > 
> > So, I'd suggest that the names of these functions is the least of the
> > problems here.
> 
> Well, this series has been discarded from patchwork. Shrug. I won't be
> posting another version, stmmac can remain broken. I don't have a
> suggestion on better names for these functions.

... and in any case, Andrew's comment would be a *separate* change to
the subject of this series, so is not appropriate to be part of this
series.

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

Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions

[Jakub Kicinski]

On Tue, 6 May 2025 09:40:02 +0100 Russell King (Oracle) wrote:
> > Well, this series has been discarded from patchwork. Shrug. I won't be
> > posting another version, stmmac can remain broken. I don't have a
> > suggestion on better names for these functions.  
> 
> ... and in any case, Andrew's comment would be a *separate* change to
> the subject of this series, so is not appropriate to be part of this
> series.

Right, I think the state is purely due to the typo in kdoc of patch 1.

[PATCH net-next v2 3/4] 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 stmmac_mac_link_up() and stmmac_mac_link_down() and not during
initialisation.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Furong Xu <0x1207@gmail.com>
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

[PATCH net-next v2 4/4] 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().

Tested-by: Furong Xu <0x1207@gmail.com>
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 ac27ea679b23..ef2a08342b25 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3533,9 +3533,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