[net v4 PATCH] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters

[net v4 PATCH] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters

[Xiaolei Wang]

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;
 
 	/* Queue 0 is not AVB capable */
 	if (queue <= 0 || queue >= tx_queues_count)
@@ -355,27 +356,20 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
 	if (!priv->dma_cap.av)
 		return -EOPNOTSUPP;
 
+	port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
+
 	/* Port Transmit Rate and Speed Divider */
-	switch (priv->speed) {
+	switch (div_s64(port_transmit_rate_kbps, 1000)) {
 	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:
 		return -EOPNOTSUPP;
@@ -398,10 +392,10 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
 	}
 
 	/* Final adjustments for HW */
-	value = div_s64(qopt->idleslope * 1024ll * ptr, speed_div);
+	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, speed_div);
+	value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
 	priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
 
 	value = qopt->hicredit * 1024ll * 8;
-- 
2.25.1

Re: [net v4 PATCH] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters

[Vladimir Oltean]

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

Re: [net v4 PATCH] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters

[xiaolei wang]

On 6/8/24 19:16, Vladimir Oltean wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> 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.

I will send a new version

thanks

xiaolei

>
> pw-bot: changes-requested

Re: [net v4 PATCH] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters

[Vladimir Oltean]

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;
>  
>  	/* Queue 0 is not AVB capable */
>  	if (queue <= 0 || queue >= tx_queues_count)
> @@ -355,27 +356,20 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  	if (!priv->dma_cap.av)
>  		return -EOPNOTSUPP;
>  
> +	port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
> +
>  	/* Port Transmit Rate and Speed Divider */
> -	switch (priv->speed) {
> +	switch (div_s64(port_transmit_rate_kbps, 1000)) {
>  	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:
>  		return -EOPNOTSUPP;

I have one more request.

It is very discouraging for a user to give invalid parameters and
receive -EOPNOTSUPP, because this is indicative of other "usual
suspects": "did I enable CONFIG_NET_SCH_CBS?"

It is unfortunate that struct tc_cbs_qopt_offload does not carry a
netlink extack, but I'm also not requesting you to add one here.
Instead, please at least change the return code to something like
-EINVAL, and print something to the console along the lines of:
"Invalid portTransmitRate %lld (idleSlope - sendSlope)\n".