From: Wei Fang <[email protected]>
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. It may get wrong indexes of
the two TCs and make the CBS unconfigurable. 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.
^^^^^
Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for hardware offload")
Signed-off-by: Wei Fang <[email protected]>
---
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
On Tue, Jun 06, 2023 at 04:46:18PM +0800, [email protected] wrote:
> From: Wei Fang <[email protected]>
>
> 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. It may get wrong indexes of
> the two TCs and make the CBS unconfigurable. e.g.
Well, you need to consider that prior to commit 1a353111b6d4 ("net:
enetc: act upon the requested mqprio queue configuration"), the driver
would always set up an identity mapping between priorities, traffic
classes, rings and netdev queues.
So, yes, giving a "tc" argument to netdev_get_prio_tc_map() is
semantically incorrect, but it only started being a problem when the
identity mapping started being configurable.
> $ 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.
> ^^^^^
ok.
>
> Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for hardware offload")
In principle, there shouldn't be an issue with backporting the fix that
far (v5.5), even if it is unnecessary beyond commit 1a353111b6d4 (v6.3).
If you want to respin the patch to clarify the situation, fine. If not,
also fine.
> Signed-off-by: Wei Fang <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Tue, Jun 06, 2023 at 04:46:18PM +0800, [email protected] wrote:
> From: Wei Fang <[email protected]>
if you are a sender then you could skip line above.
>
> 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. It may get wrong indexes of
> the two TCs and make the CBS unconfigurable. 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.
> ^^^^^
newlines between commit message and example output would improve
readability. tc commands are awful to read by themselves :P
Please describe in the commit message what is the actual fix.
>
> Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for hardware offload")
> Signed-off-by: Wei Fang <[email protected]>
> ---
> 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
>
>
On Tue, 6 Jun 2023 21:41:57 +0200 Maciej Fijalkowski wrote:
> On Tue, Jun 06, 2023 at 04:46:18PM +0800, [email protected] wrote:
> > From: Wei Fang <[email protected]>
>
> if you are a sender then you could skip line above.
Git generates it because the From in the headers does not have the
names, just the email addr. It's better to keep it.
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: 2023??6??6?? 17:17
> To: Wei Fang <[email protected]>
> Cc: Claudiu Manoil <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: enetc: correct the indexes of highest and 2nd
> highest TCs
>
> On Tue, Jun 06, 2023 at 04:46:18PM +0800, [email protected] wrote:
> > From: Wei Fang <[email protected]>
> >
> > 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. It may get wrong indexes of
> > the two TCs and make the CBS unconfigurable. e.g.
>
> Well, you need to consider that prior to commit 1a353111b6d4 ("net:
> enetc: act upon the requested mqprio queue configuration"), the driver
> would always set up an identity mapping between priorities, traffic
> classes, rings and netdev queues.
>
I also considered the situation prior to the commit 1a353111b6d4. The
problem also existed.
e.g.
$ tc qdisc add dev eno0 root handle 1: mqprio num_tc 8 \
map 0 1 2 3 6 7 4 5 queues 1@0 1@1 \
^ ^ ^ ^
1@2 1@3 1@4 1@5 1@6 1@7 hw 1
The driver would deem the indexes of the two highest TCs are 5 and 4,
rather than 7 and 6.
> So, yes, giving a "tc" argument to netdev_get_prio_tc_map() is
> semantically incorrect, but it only started being a problem when the
> identity mapping started being configurable.
>
In my opinion, "unconfigurable" is also a problem.
> > $ 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.
> > ^^^^^
>
> ok.
>
> >
> > Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for
> hardware offload")
>
> In principle, there shouldn't be an issue with backporting the fix that
> far (v5.5), even if it is unnecessary beyond commit 1a353111b6d4 (v6.3).
> If you want to respin the patch to clarify the situation, fine. If not,
> also fine.
>
> > Signed-off-by: Wei Fang <[email protected]>
> > ---
>
> Reviewed-by: Vladimir Oltean <[email protected]>