Received-SPF: pass (google.com: domain of linux-kernel+bounces-195388-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 30 May 2024 14:50:52 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Xiaolei Wang <xiaolei.wang@windriver.com>
Cc: linux@armlinux.org.uk, alexandre.torgue@foss.st.com,
	joabreu@synopsys.com, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed
 changes after linking up
Message-ID: <f8b0843f-7900-4ad0-9e70-c16175e893d9@lunn.ch>
References: <20240530061453.561708-1-xiaolei.wang@windriver.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240530061453.561708-1-xiaolei.wang@windriver.com>

>  static void stmmac_configure_cbs(struct stmmac_priv *priv)
>  {
>  	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>  	u32 mode_to_use;
>  	u32 queue;
> +	u32 ptr, speed_div;
> +	u64 value;
> +
> +	/* Port Transmit Rate and Speed Divider */
> +	switch (priv->speed) {
> +	case SPEED_10000:
> +		ptr = 32;
> +		speed_div = 10000000;
> +		break;
> +	case SPEED_5000:
> +		ptr = 32;
> +		speed_div = 5000000;
> +		break;
> +	case SPEED_2500:
> +		ptr = 8;
> +		speed_div = 2500000;
> +		break;
> +	case SPEED_1000:
> +		ptr = 8;
> +		speed_div = 1000000;
> +		break;
> +	case SPEED_100:
> +		ptr = 4;
> +		speed_div = 100000;
> +		break;
> +	default:

No SPEED_10 ?

> +		netdev_dbg(priv->dev, "link speed is not known\n");
> +	}
>  
>  	/* queue 0 is reserved for legacy traffic */
>  	for (queue = 1; queue < tx_queues_count; queue++) {
> @@ -3196,6 +3231,12 @@ static void stmmac_configure_cbs(struct stmmac_priv *priv)
>  		if (mode_to_use == MTL_QUEUE_DCB)
>  			continue;
>  
> +		value = div_s64(priv->old_idleslope[queue] * 1024ll * ptr, speed_div);
> +		priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);

Rather than masking off the top bits, shouldn't you be looking for
overflow? that indicates the configuration is not possible. You don't
have a good way to report the problem, since there is no user action
on link up, so you cannot return -EINVAL or -EOPNOTSUPP. So you
probably want to set the hardware as close as possible.

Also, what happens if the result of the div is 0? Does 0 have a
special meaning?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 222540b55480..d3526ad91aff 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -355,6 +355,9 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  	if (!priv->dma_cap.av)
>  		return -EOPNOTSUPP;
>  
> +	if (!netif_carrier_ok(priv->dev))
> +		return -ENETDOWN;
> +

Now that you are configuring the hardware on link up, does that
matter?

	Andrew