Re: [PATCH net-next v2 1/3] net/stmmac/dwxgmac: Modify DMA functions for future hardware

[Larysa Zaremba]

Overall looks fine, but some adjustments won't hurt.

Reviewed-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>

On Wed, Jun 03, 2026 at 01:46:53PM +0200, Jakub Raczynski wrote:
> Gonna respond to Sashiko comments.
> 
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > @@ -97,11 +97,19 @@ static void dwxgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
> > >  static void dwxgmac2_rx_queue_prio(struct mac_device_info *hw, u32 prio,
> > >  				   u32 queue)
> > >  {
> > > +	const unsigned int numtc = hw->priv_data->dma_cap.numtc;
> > > +	struct net_device *ndev = hw->priv_data->dev;
> > >  	void __iomem *ioaddr = hw->pcsr;
> > >  	u32 clear_mask = 0;
> > >  	u32 ctrl2, ctrl3;
> > >  	int i;
> > >  
> > > +	if (queue >= numtc) {
> > > +		netdev_err(ndev, "%s: invalid TC queue %d, supported %d\n",
> > > +			   __func__, queue, numtc);
> > > +		return;
> > > +	}
> > Does this artificially restrict the configuration of valid RX queue priorities?
> > It looks like the queue index is being compared against the number of
> > traffic classes (numtc), which could prevent mapping priorities on valid
> > queues greater than the number of traffic classes.
> > [ ... ]
> 
> No. These registers just don't exist to support more.
> Synopsys seems to have done this this way because there there is no free
> space after XGMAC_RXQ_CTRL3.
> So if this becomes a thing, we will have big new driver dwxgmac3.
>
> > > @@ -246,8 +263,15 @@ static void dwxgmac2_set_mtl_tx_queue_weight(struct stmmac_priv *priv,
> > >  					     struct mac_device_info *hw,
> > >  					     u32 weight, u32 queue)
> > >  {
> > > +	const unsigned int numtc = priv->dma_cap.numtc;
> > >  	void __iomem *ioaddr = hw->pcsr;
> > >  
> > > +	if (queue >= numtc) {
> > > +		netdev_err(priv->dev, "%s: invalid TC queue %d, supported %d\n",
> > > +			   __func__, queue, numtc);
> > > +		return;
> > > +	}
> > > +
> > Because dwxgmac2_set_mtl_tx_queue_weight() returns void and is invoked via
> > the stmmac_do_void_callback() macro (which unconditionally returns 0), will
> > this early return cause ETS qdisc queue weighting on higher queue indices
> > to silently fail? 
> > It appears userspace will receive a success code while the hardware is left
> > unconfigured.
> > [ ... ]
> 
> As talked in previous mailing, answer is yes, there will be error in dmesg,
> but no fail, for very simple reason that we would need to rewrite A LOT to add
> proper error handling.
> I just hope that if user misconfigured this stuff they will read this error
> and fix it rather than complain.
>

But previously those callbacks did not have any failure conditions, so I would 
say that adding a message-only failure does constitute a minor regression.
 
> > > @@ -357,6 +388,8 @@ static void dwxgmac2_flow_ctrl(struct mac_device_info *hw, unsigned int duplex,
> > >  			       unsigned int fc, unsigned int pause_time,
> > >  			       u8 tx_cnt)
> > >  {
> > > +	const unsigned int numtc = hw->priv_data->dma_cap.numtc;
> > > +	struct net_device *ndev = hw->priv_data->dev;
> > >  	void __iomem *ioaddr = hw->pcsr;
> > >  	u8 i;
> > >  
> > > @@ -366,6 +399,17 @@ static void dwxgmac2_flow_ctrl(struct mac_device_info *hw, unsigned int duplex,
> > >  		for (i = 0; i < tx_cnt; i++) {
> > >  			u32 value = XGMAC_TFE;
> > >  
> > > +			if (i >= numtc) {
> > > +				netdev_err(ndev,
> > > +					   "%s: invalid TC queue %d, supported %d\n",
> > > +					   __func__, i, numtc);
> > > +				/* This will skip all other wrong channels,
> > > +				 * but triggering this is preceded by warnings
> > > +				 * from other functions, so limit the spam.
> > > +				 */
> > > +				break;
> > > +			}
> > > +
> > If the hardware supports up to 16 transmit queues but only 8 traffic classes,
> > does breaking this loop at numtc skip TX flow control initialization for
> > valid higher queues? 
> > The XGMAC_Qx_TX_FLOW_CTRL registers appear to be per-queue rather than
> > per-traffic-class.
> > [ ... ]
> 
> No, queues are in order. If dynamic queues get implemented then maybe?
> Depends how this will get implemenented.
> 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 3591755ea30be..5917bf47c7de6 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -4140,7 +4140,7 @@ static int __stmmac_open(struct net_device *dev,
> > >  	u8 chan;
> > >  	int ret;
> > >  
> > > -	for (int i = 0; i < MTL_MAX_TX_QUEUES; i++)
> > > +	for (int i = 0; i < priv->plat->tx_queues_to_use; i++)
> > >  		if (priv->dma_conf.tx_queue[i].tbs & STMMAC_TBS_EN)
> > >  			dma_conf->tx_queue[i].tbs = priv->dma_conf.tx_queue[i].tbs;
> > The tx_queue array inside struct stmmac_dma_conf is statically sized to
> > MTL_MAX_TX_QUEUES (8). If priv->plat->tx_queues_to_use is dynamically set
> > up to 16, does this cause an out-of-bounds access that corrupts adjacent
> > memory?
> > I see this is resolved later in the series by commit 9cc40baf0739 which
> > increases the macro to 16. Could this intermediate step in the series break
> > bisectability due to the buffer overflow?
> 
> Nah.

Maybe worth adding a WARN_ON to indicate that such case can only come from a 
programming error?

> 
> BR
> Jakub Raczynski