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

[Jakub Raczynski <j.raczynski@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4735 bytes --]

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.

> > @@ -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.

BR
Jakub Raczynski

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]