Received-SPF: pass (google.com: domain of linux-kernel+bounces-215909-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 15 Jun 2024 15:47:47 +0100
From: Simon Horman <horms@kernel.org>
To: Xiaolei Wang <xiaolei.wang@windriver.com>
Cc: olteanv@gmail.com, linux@armlinux.org.uk, alexandre.torgue@foss.st.com,
	andrew@lunn.ch, joabreu@synopsys.com, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	mcoquelin.stm32@gmail.com, wojciech.drewek@intel.com,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [net PATCH] net: stmmac: No need to calculate speed divider when
 offload is disabled
Message-ID: <20240615144747.GE8447@kernel.org>
References: <20240614081916.764761-1-xiaolei.wang@windriver.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240614081916.764761-1-xiaolei.wang@windriver.com>

On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
> commit be27b8965297 ("net: stmmac: replace priv->speed with
> the portTransmitRate from the tc-cbs parameters") introduced
> a problem. When deleting, it prompts "Invalid portTransmitRate
> 0 (idleSlope - sendSlope)" and exits. Add judgment on cbs.enable.
> Only when offload is enabled, speed divider needs to be calculated.
> 
> Fixes: be27b8965297 ("net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters")
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 38 ++++++++++---------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 1562fbdd0a04..b0fd2d6e525e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -358,24 +358,26 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  
>  	port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
>  
> -	/* Port Transmit Rate and Speed Divider */
> -	switch (div_s64(port_transmit_rate_kbps, 1000)) {
> -	case SPEED_10000:
> -	case SPEED_5000:
> -		ptr = 32;
> -		break;
> -	case SPEED_2500:
> -	case SPEED_1000:
> -		ptr = 8;
> -		break;
> -	case SPEED_100:
> -		ptr = 4;
> -		break;
> -	default:
> -		netdev_err(priv->dev,
> -			   "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> -			   port_transmit_rate_kbps);
> -		return -EINVAL;
> +	if (qopt->enable) {
> +		/* Port Transmit Rate and Speed Divider */
> +		switch (div_s64(port_transmit_rate_kbps, 1000)) {
> +		case SPEED_10000:
> +		case SPEED_5000:
> +			ptr = 32;
> +			break;
> +		case SPEED_2500:
> +		case SPEED_1000:
> +			ptr = 8;
> +			break;
> +		case SPEED_100:
> +			ptr = 4;
> +			break;
> +		default:
> +			netdev_err(priv->dev,
> +				   "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> +				   port_transmit_rate_kbps);
> +			return -EINVAL;
> +		}
>  	}
>  	mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;

Hi Xiaolei Wang,

The code following this function looks like this:

	if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
		ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
		if (ret)
			return ret;
		priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
	} else if (!qopt->enable) {
		ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
				       MTL_QUEUE_DCB);
		if (ret)
			return ret;
		priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
	}

	/* Final adjustments for HW */
	value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
	priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);

	value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
	priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);

And the div_s64() lines above appear to use
ptr uninitialised in the !qopt->enable case.

Flagged by Smatch.

-- 
pw-bot: changes-requested