diff mbox series

[V2,net] net: enetc: correct the indexes of highest and 2nd highest TCs

Message ID 20230607091048.1152674-1-wei.fang@nxp.com
State New
Headers
Series [V2,net] net: enetc: correct the indexes of highest and 2nd highest TCs |

Commit Message

Wei Fang June 7, 2023, 9:10 a.m. UTC 
  From: Wei Fang <wei.fang@nxp.com>

For ENETC hardware, the TCs are numbered from 0 to N-1, where N
is the number of TCs. Numerically higher TC has higher priority.
It's obvious that the highest priority TC index should be N-1 and
the 2nd highest priority TC index should be N-2.

However, the previous logic uses netdev_get_prio_tc_map() to get
the indexes of highest priority and 2nd highest priority TCs, it
does not make sense and is incorrect to give a "tc" argument to
netdev_get_prio_tc_map(). So the driver may get the wrong indexes
of the two highest priotiry TCs which would lead to failed to set
the CBS for the two highest priotiry TCs.

e.g.
$ tc qdisc add dev eno0 parent root handle 100: mqprio num_tc 6 \
	map 0 0 1 1 2 3 4 5 queues 1@0 1@1 1@2 1@3 2@4 2@6 hw 1
$ tc qdisc replace dev eno0 parent 100:6 cbs idleslope 100000 \
	sendslope -900000 hicredit 12 locredit -113 offload 1
$ Error: Specified device failed to setup cbs hardware offload.
  ^^^^^

In this example, the previous logic deems the indexes of the two
highest priotiry TCs should be 3 and 2. Actually, the indexes are
5 and 4, because the number of TCs is 6. So it would be failed to
configure the CBS for the two highest priority TCs.

Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for hardware offload")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
V2:
Improved the commit message based on Maciej's comments.
---
 drivers/net/ethernet/freescale/enetc/enetc_qos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Maciej Fijalkowski June 7, 2023, 11:07 a.m. UTC | #1 
On Wed, Jun 07, 2023 at 05:10:48PM +0800, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> For ENETC hardware, the TCs are numbered from 0 to N-1, where N
> is the number of TCs. Numerically higher TC has higher priority.
> It's obvious that the highest priority TC index should be N-1 and
> the 2nd highest priority TC index should be N-2.
> 
> However, the previous logic uses netdev_get_prio_tc_map() to get
> the indexes of highest priority and 2nd highest priority TCs, it
> does not make sense and is incorrect to give a "tc" argument to
> netdev_get_prio_tc_map(). So the driver may get the wrong indexes
> of the two highest priotiry TCs which would lead to failed to set
> the CBS for the two highest priotiry TCs.
> 
> e.g.
> $ tc qdisc add dev eno0 parent root handle 100: mqprio num_tc 6 \
> 	map 0 0 1 1 2 3 4 5 queues 1@0 1@1 1@2 1@3 2@4 2@6 hw 1
> $ tc qdisc replace dev eno0 parent 100:6 cbs idleslope 100000 \
> 	sendslope -900000 hicredit 12 locredit -113 offload 1
> $ Error: Specified device failed to setup cbs hardware offload.
>   ^^^^^
> 
> In this example, the previous logic deems the indexes of the two
> highest priotiry TCs should be 3 and 2. Actually, the indexes are
> 5 and 4, because the number of TCs is 6. So it would be failed to
> configure the CBS for the two highest priority TCs.
> 
> Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for hardware offload")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Thanks!

> ---
> V2:
> Improved the commit message based on Maciej's comments.
> ---
>  drivers/net/ethernet/freescale/enetc/enetc_qos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index 83c27bbbc6ed..126007ab70f6 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -181,8 +181,8 @@ int enetc_setup_tc_cbs(struct net_device *ndev, void *type_data)
>  	int bw_sum = 0;
>  	u8 bw;
>  
> -	prio_top = netdev_get_prio_tc_map(ndev, tc_nums - 1);
> -	prio_next = netdev_get_prio_tc_map(ndev, tc_nums - 2);
> +	prio_top = tc_nums - 1;
> +	prio_next = tc_nums - 2;
>  
>  	/* Support highest prio and second prio tc in cbs mode */
>  	if (tc != prio_top && tc != prio_next)
> -- 
> 2.25.1
>
patchwork-bot+netdevbpf@kernel.org June 8, 2023, 8 p.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed,  7 Jun 2023 17:10:48 +0800 you wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> For ENETC hardware, the TCs are numbered from 0 to N-1, where N
> is the number of TCs. Numerically higher TC has higher priority.
> It's obvious that the highest priority TC index should be N-1 and
> the 2nd highest priority TC index should be N-2.
> 
> [...]

Here is the summary with links:
  - [V2,net] net: enetc: correct the indexes of highest and 2nd highest TCs
    https://git.kernel.org/netdev/net/c/21225873be14

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 83c27bbbc6ed..126007ab70f6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -181,8 +181,8 @@  int enetc_setup_tc_cbs(struct net_device *ndev, void *type_data)
 	int bw_sum = 0;
 	u8 bw;
 
-	prio_top = netdev_get_prio_tc_map(ndev, tc_nums - 1);
-	prio_next = netdev_get_prio_tc_map(ndev, tc_nums - 2);
+	prio_top = tc_nums - 1;
+	prio_next = tc_nums - 2;
 
 	/* Support highest prio and second prio tc in cbs mode */
 	if (tc != prio_top && tc != prio_next)