netdev - Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed changes after linking up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f8b0843f-7900-4ad0-9e70-c16175e893d9@lunn.ch>
Date: Thu, 30 May 2024 14:50:52 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Xiaolei Wang <xiaolei.wang@...driver.com>
Cc: linux@...linux.org.uk, alexandre.torgue@...s.st.com,
	joabreu@...opsys.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, mcoquelin.stm32@...il.com,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed
 changes after linking up

>  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