Received-SPF: pass (google.com: domain of linux-kernel+bounces-207016-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 8 Jun 2024 14:16:21 +0300
From: Vladimir Oltean <olteanv@gmail.com>
To: Xiaolei Wang <xiaolei.wang@windriver.com>
Cc: linux@armlinux.org.uk, andrew@lunn.ch, alexandre.torgue@foss.st.com,
	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 v4 PATCH] net: stmmac: replace priv->speed with the
 portTransmitRate from the tc-cbs parameters
Message-ID: <20240608111621.oasttwwkhsmpcl4y@skbuf>
References: <20240608044557.1380550-1-xiaolei.wang@windriver.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240608044557.1380550-1-xiaolei.wang@windriver.com>

On Sat, Jun 08, 2024 at 12:45:57PM +0800, Xiaolei Wang wrote:
> The current cbs parameter depends on speed after uplinking,
> which is not needed and will report a configuration error
> if the port is not initially connected. The UAPI exposed by
> tc-cbs requires userspace to recalculate the send slope anyway,
> because the formula depends on port_transmit_rate (see man tc-cbs),
> which is not an invariant from tc's perspective. Therefore, we
> use offload->sendslope and offload->idleslope to derive the
> original port_transmit_rate from the CBS formula.
> 
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> 
> Change log:
> 
> v1:
>     https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240528092010.439089-1-xiaolei.wang@windriver.com/
> v2:
>     Update CBS parameters when speed changes after linking up
>     https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240530061453.561708-1-xiaolei.wang@windriver.com/
> v3:
>     replace priv->speed with the  portTransmitRate from the tc-cbs parameters suggested by Vladimir Oltean
>     link: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240607103327.438455-1-xiaolei.wang@windriver.com/
> v4:
>     Delete speed_div variable, delete redundant port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; and update commit log
> 
>  .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 20 +++++++------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 222540b55480..87af129a6a1d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -344,10 +344,11 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  {
>  	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>  	u32 queue = qopt->queue;
> -	u32 ptr, speed_div;
> +	u32 ptr;
>  	u32 mode_to_use;
>  	u64 value;
>  	int ret;
> +	s64 port_transmit_rate_kbps;

The feedback that came along with Wojciech's review in v3 was to use
reverse Christmas tree (RCT) variable ordering. That means to sort
variable declarations from longest line to shortest. It is the de facto
coding style standard for kernel networking code.

pw-bot: changes-requested