[PATCH net v1 0/3] taprio: Some fixes

[PATCH net v1 0/3] taprio: Some fixes

[Vinicius Costa Gomes]

Hi,

Some fixes for taprio, mostly (2/3 and 3/3) related to the 'flags'
argument:

Patch 1/3: Reported by Po Liu, is more of a improvement of usability for
drivers implementing offloading features, now they can rely on the
value of dev->num_tc, instead of going through some hops to get this
value.

Patch 2/3: Changing the "flags" parameter during "runtime" was never
supposed to work, but if the user didn't set it at the beginning, we
were still allowing it.

Patch 3/3: Improves the code, so a similar situation fixed by 2/3 is
harder to happen again, and has the intended side effect that users do
not need to specify the "flags" argument when changing schedules when
some kind of offloading is enabled.


Cheers,
--
Vinicius

Vinicius Costa Gomes (3):
  taprio: Fix enabling offload with wrong number of traffic classes
  taprio: Fix still allowing changing the flags during runtime
  taprio: Allow users not to specify "flags" when changing schedules

 net/sched/sch_taprio.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
2.25.0

[PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Vinicius Costa Gomes]

If the driver implementing taprio offloading depends on the value of
the network device number of traffic classes (dev->num_tc) for
whatever reason, it was going to receive the value zero. The value was
only set after the offloading function is called.

So, moving setting the number of traffic classes to before the
offloading function is called fixes this issue. This is safe because
this only happens when taprio is instantiated (we don't allow this
configuration to be changed without first removing taprio).

Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
Reported-by: Po Liu <po.liu@nxp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 net/sched/sch_taprio.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index c609373c8661..ad0dadcfcdba 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 
 	taprio_set_picos_per_byte(dev, q);
 
+	if (mqprio) {
+		netdev_set_num_tc(dev, mqprio->num_tc);
+		for (i = 0; i < mqprio->num_tc; i++)
+			netdev_set_tc_queue(dev, i,
+					    mqprio->count[i],
+					    mqprio->offset[i]);
+
+		/* Always use supplied priority mappings */
+		for (i = 0; i <= TC_BITMASK; i++)
+			netdev_set_prio_tc_map(dev, i,
+					       mqprio->prio_tc_map[i]);
+	}
+
 	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
 		err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
 	else
@@ -1471,19 +1484,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->advance_timer.function = advance_sched;
 	}
 
-	if (mqprio) {
-		netdev_set_num_tc(dev, mqprio->num_tc);
-		for (i = 0; i < mqprio->num_tc; i++)
-			netdev_set_tc_queue(dev, i,
-					    mqprio->count[i],
-					    mqprio->offset[i]);
-
-		/* Always use supplied priority mappings */
-		for (i = 0; i <= TC_BITMASK; i++)
-			netdev_set_prio_tc_map(dev, i,
-					       mqprio->prio_tc_map[i]);
-	}
-
 	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
 		q->dequeue = taprio_dequeue_offload;
 		q->peek = taprio_peek_offload;
-- 
2.25.0

Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Vladimir Oltean]

On Sat, 25 Jan 2020 at 02:53, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> If the driver implementing taprio offloading depends on the value of
> the network device number of traffic classes (dev->num_tc) for
> whatever reason, it was going to receive the value zero. The value was
> only set after the offloading function is called.
>
> So, moving setting the number of traffic classes to before the
> offloading function is called fixes this issue. This is safe because
> this only happens when taprio is instantiated (we don't allow this
> configuration to be changed without first removing taprio).
>
> Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
> Reported-by: Po Liu <po.liu@nxp.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  net/sched/sch_taprio.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index c609373c8661..ad0dadcfcdba 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>
>         taprio_set_picos_per_byte(dev, q);
>
> +       if (mqprio) {
> +               netdev_set_num_tc(dev, mqprio->num_tc);
> +               for (i = 0; i < mqprio->num_tc; i++)
> +                       netdev_set_tc_queue(dev, i,
> +                                           mqprio->count[i],
> +                                           mqprio->offset[i]);
> +
> +               /* Always use supplied priority mappings */
> +               for (i = 0; i <= TC_BITMASK; i++)
> +                       netdev_set_prio_tc_map(dev, i,
> +                                              mqprio->prio_tc_map[i]);
> +       }
> +
>         if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
>                 err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
>         else
> @@ -1471,19 +1484,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                 q->advance_timer.function = advance_sched;
>         }
>
> -       if (mqprio) {
> -               netdev_set_num_tc(dev, mqprio->num_tc);
> -               for (i = 0; i < mqprio->num_tc; i++)
> -                       netdev_set_tc_queue(dev, i,
> -                                           mqprio->count[i],
> -                                           mqprio->offset[i]);
> -
> -               /* Always use supplied priority mappings */
> -               for (i = 0; i <= TC_BITMASK; i++)
> -                       netdev_set_prio_tc_map(dev, i,
> -                                              mqprio->prio_tc_map[i]);
> -       }
> -
>         if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
>                 q->dequeue = taprio_dequeue_offload;
>                 q->peek = taprio_peek_offload;
> --
> 2.25.0
>

Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Andre Guedes]

Hi Vinicius,

Quoting Vinicius Costa Gomes (2020-01-24 16:53:18)
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index c609373c8661..ad0dadcfcdba 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  
>         taprio_set_picos_per_byte(dev, q);
>  
> +       if (mqprio) {
> +               netdev_set_num_tc(dev, mqprio->num_tc);
> +               for (i = 0; i < mqprio->num_tc; i++)
> +                       netdev_set_tc_queue(dev, i,
> +                                           mqprio->count[i],
> +                                           mqprio->offset[i]);
> +
> +               /* Always use supplied priority mappings */
> +               for (i = 0; i <= TC_BITMASK; i++)
> +                       netdev_set_prio_tc_map(dev, i,
> +                                              mqprio->prio_tc_map[i]);
> +       }
> +
>         if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
>                 err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
>         else

If something goes wrong later within this function (e.g.
taprio_enable_offload() returns error), don't we want to roll back these
changes to the netdev object?

- Andre

Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Vinicius Costa Gomes]

Hi Andre,

Andre Guedes <andre.guedes@linux.intel.com> writes:

> Hi Vinicius,
>
> Quoting Vinicius Costa Gomes (2020-01-24 16:53:18)
>> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
>> index c609373c8661..ad0dadcfcdba 100644
>> --- a/net/sched/sch_taprio.c
>> +++ b/net/sched/sch_taprio.c
>> @@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>>  
>>         taprio_set_picos_per_byte(dev, q);
>>  
>> +       if (mqprio) {
>> +               netdev_set_num_tc(dev, mqprio->num_tc);
>> +               for (i = 0; i < mqprio->num_tc; i++)
>> +                       netdev_set_tc_queue(dev, i,
>> +                                           mqprio->count[i],
>> +                                           mqprio->offset[i]);
>> +
>> +               /* Always use supplied priority mappings */
>> +               for (i = 0; i <= TC_BITMASK; i++)
>> +                       netdev_set_prio_tc_map(dev, i,
>> +                                              mqprio->prio_tc_map[i]);
>> +       }
>> +
>>         if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
>>                 err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
>>         else
>
> If something goes wrong later within this function (e.g.
> taprio_enable_offload() returns error), don't we want to roll back these
> changes to the netdev object?

If something goes wrong, and change() returns an error, taprio_destroy()
is called, and the changes are undone.


Cheers,
--
Vinicius

[PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime

[Vinicius Costa Gomes]

The check for 'q->flags' being zero allows that, if the taprio
instance was created with no 'flags' specified, for 'flags' to be
changed. By design this should not be permitted, as it might require
changing the enqueue()/dequeue() functions of taprio, which might
already be running, and if the hrtimer is enabled or not.

The solution is to not support changing flags in any way during
"runtime".

As a result of this problem the following RCU stall can be observed:

[ 1730.558249] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[ 1730.558258] rcu: 	  6-...0: (190 ticks this GP) idle=922/0/0x1 softirq=25580/25582 fqs=16250
[ 1730.558264] 		  (detected by 2, t=65002 jiffies, g=33017, q=81)
[ 1730.558269] Sending NMI from CPU 2 to CPUs 6:
[ 1730.559277] NMI backtrace for cpu 6
[ 1730.559277] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G            E     5.5.0-rc6+ #35
[ 1730.559278] Hardware name: Gigabyte Technology Co., Ltd. Z390 AORUS ULTRA/Z390 AORUS ULTRA-CF, BIOS F7 03/14/2019
[ 1730.559278] RIP: 0010:__hrtimer_run_queues+0xe2/0x440
[ 1730.559278] Code: 48 8b 43 28 4c 89 ff 48 8b 75 c0 48 89 45 c8 e8 f4 bb 7c 00 0f 1f 44 00 00 65 8b 05 40 31 f0 68 89 c0 48 0f a3 05 3e 5c 25 01 <0f> 82 fc 01 00 00 48 8b 45 c8 48 89 df ff d0 89 45 c8 0f 1f 44 00
[ 1730.559279] RSP: 0018:ffff9970802d8f10 EFLAGS: 00000083
[ 1730.559279] RAX: 0000000000000006 RBX: ffff8b31645bff38 RCX: 0000000000000000
[ 1730.559280] RDX: 0000000000000000 RSI: ffffffff9710f2ec RDI: ffffffff978daf0e
[ 1730.559280] RBP: ffff9970802d8f68 R08: 0000000000000000 R09: 0000000000000000
[ 1730.559280] R10: 0000018336d7944e R11: 0000000000000001 R12: ffff8b316e39f9c0
[ 1730.559281] R13: ffff8b316e39f940 R14: ffff8b316e39f998 R15: ffff8b316e39f7c0
[ 1730.559281] FS:  0000000000000000(0000) GS:ffff8b316e380000(0000) knlGS:0000000000000000
[ 1730.559281] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1730.559281] CR2: 00007f1105303760 CR3: 0000000227210005 CR4: 00000000003606e0
[ 1730.559282] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1730.559282] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1730.559282] Call Trace:
[ 1730.559282]  <IRQ>
[ 1730.559283]  ? taprio_dequeue_soft+0x2d0/0x2d0 [sch_taprio]
[ 1730.559283]  hrtimer_interrupt+0x104/0x220
[ 1730.559283]  ? irqtime_account_irq+0x34/0xa0
[ 1730.559283]  smp_apic_timer_interrupt+0x6d/0x230
[ 1730.559284]  apic_timer_interrupt+0xf/0x20
[ 1730.559284]  </IRQ>
[ 1730.559284] RIP: 0010:cpu_idle_poll+0x35/0x1a0
[ 1730.559285] Code: 88 82 ff 65 44 8b 25 12 7d 73 68 0f 1f 44 00 00 e8 90 c3 89 ff fb 65 48 8b 1c 25 c0 7e 01 00 48 8b 03 a8 08 74 0b eb 1c f3 90 <48> 8b 03 a8 08 75 13 8b 05 be a8 a8 00 85 c0 75 ed e8 75 48 84 ff
[ 1730.559285] RSP: 0018:ffff997080137ea8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
[ 1730.559285] RAX: 0000000000000001 RBX: ffff8b316bc3c580 RCX: 0000000000000000
[ 1730.559286] RDX: 0000000000000001 RSI: 000000002819aad9 RDI: ffffffff978da730
[ 1730.559286] RBP: ffff997080137ec0 R08: 0000018324a6d387 R09: 0000000000000000
[ 1730.559286] R10: 0000000000000400 R11: 0000000000000001 R12: 0000000000000006
[ 1730.559286] R13: ffff8b316bc3c580 R14: 0000000000000000 R15: 0000000000000000
[ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
[ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
[ 1730.559287]  do_idle+0x4d/0x1f0
[ 1730.559287]  ? complete+0x44/0x50
[ 1730.559288]  cpu_startup_entry+0x1b/0x20
[ 1730.559288]  start_secondary+0x142/0x180
[ 1730.559288]  secondary_startup_64+0xb6/0xc0
[ 1776.686313] nvme nvme0: I/O 96 QID 1 timeout, completion polled

Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 net/sched/sch_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index ad0dadcfcdba..dfbecb9ee2f4 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1391,7 +1391,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
 		taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
 
-		if (q->flags != 0 && q->flags != taprio_flags) {
+		if (q->flags != taprio_flags) {
 			NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
 			return -EOPNOTSUPP;
 		} else if (!taprio_flags_valid(taprio_flags)) {
-- 
2.25.0

Re: [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime

[Vladimir Oltean]

Hi Vinicius,

On Sat, 25 Jan 2020 at 02:52, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> The check for 'q->flags' being zero allows that, if the taprio
> instance was created with no 'flags' specified, for 'flags' to be
> changed. By design this should not be permitted, as it might require
> changing the enqueue()/dequeue() functions of taprio, which might
> already be running, and if the hrtimer is enabled or not.
>
> The solution is to not support changing flags in any way during
> "runtime".
>
> As a result of this problem the following RCU stall can be observed:
>
> [ 1730.558249] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
> [ 1730.558258] rcu:       6-...0: (190 ticks this GP) idle=922/0/0x1 softirq=25580/25582 fqs=16250
> [ 1730.558264]            (detected by 2, t=65002 jiffies, g=33017, q=81)
> [ 1730.558269] Sending NMI from CPU 2 to CPUs 6:
> [ 1730.559277] NMI backtrace for cpu 6
> [ 1730.559277] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G            E     5.5.0-rc6+ #35
> [ 1730.559278] Hardware name: Gigabyte Technology Co., Ltd. Z390 AORUS ULTRA/Z390 AORUS ULTRA-CF, BIOS F7 03/14/2019
> [ 1730.559278] RIP: 0010:__hrtimer_run_queues+0xe2/0x440
> [ 1730.559278] Code: 48 8b 43 28 4c 89 ff 48 8b 75 c0 48 89 45 c8 e8 f4 bb 7c 00 0f 1f 44 00 00 65 8b 05 40 31 f0 68 89 c0 48 0f a3 05 3e 5c 25 01 <0f> 82 fc 01 00 00 48 8b 45 c8 48 89 df ff d0 89 45 c8 0f 1f 44 00
> [ 1730.559279] RSP: 0018:ffff9970802d8f10 EFLAGS: 00000083
> [ 1730.559279] RAX: 0000000000000006 RBX: ffff8b31645bff38 RCX: 0000000000000000
> [ 1730.559280] RDX: 0000000000000000 RSI: ffffffff9710f2ec RDI: ffffffff978daf0e
> [ 1730.559280] RBP: ffff9970802d8f68 R08: 0000000000000000 R09: 0000000000000000
> [ 1730.559280] R10: 0000018336d7944e R11: 0000000000000001 R12: ffff8b316e39f9c0
> [ 1730.559281] R13: ffff8b316e39f940 R14: ffff8b316e39f998 R15: ffff8b316e39f7c0
> [ 1730.559281] FS:  0000000000000000(0000) GS:ffff8b316e380000(0000) knlGS:0000000000000000
> [ 1730.559281] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1730.559281] CR2: 00007f1105303760 CR3: 0000000227210005 CR4: 00000000003606e0
> [ 1730.559282] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1730.559282] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 1730.559282] Call Trace:
> [ 1730.559282]  <IRQ>
> [ 1730.559283]  ? taprio_dequeue_soft+0x2d0/0x2d0 [sch_taprio]
> [ 1730.559283]  hrtimer_interrupt+0x104/0x220
> [ 1730.559283]  ? irqtime_account_irq+0x34/0xa0
> [ 1730.559283]  smp_apic_timer_interrupt+0x6d/0x230
> [ 1730.559284]  apic_timer_interrupt+0xf/0x20
> [ 1730.559284]  </IRQ>
> [ 1730.559284] RIP: 0010:cpu_idle_poll+0x35/0x1a0
> [ 1730.559285] Code: 88 82 ff 65 44 8b 25 12 7d 73 68 0f 1f 44 00 00 e8 90 c3 89 ff fb 65 48 8b 1c 25 c0 7e 01 00 48 8b 03 a8 08 74 0b eb 1c f3 90 <48> 8b 03 a8 08 75 13 8b 05 be a8 a8 00 85 c0 75 ed e8 75 48 84 ff
> [ 1730.559285] RSP: 0018:ffff997080137ea8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
> [ 1730.559285] RAX: 0000000000000001 RBX: ffff8b316bc3c580 RCX: 0000000000000000
> [ 1730.559286] RDX: 0000000000000001 RSI: 000000002819aad9 RDI: ffffffff978da730
> [ 1730.559286] RBP: ffff997080137ec0 R08: 0000018324a6d387 R09: 0000000000000000
> [ 1730.559286] R10: 0000000000000400 R11: 0000000000000001 R12: 0000000000000006
> [ 1730.559286] R13: ffff8b316bc3c580 R14: 0000000000000000 R15: 0000000000000000
> [ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
> [ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
> [ 1730.559287]  do_idle+0x4d/0x1f0
> [ 1730.559287]  ? complete+0x44/0x50
> [ 1730.559288]  cpu_startup_entry+0x1b/0x20
> [ 1730.559288]  start_secondary+0x142/0x180
> [ 1730.559288]  secondary_startup_64+0xb6/0xc0
> [ 1776.686313] nvme nvme0: I/O 96 QID 1 timeout, completion polled
>
> Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  net/sched/sch_taprio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index ad0dadcfcdba..dfbecb9ee2f4 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1391,7 +1391,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>         if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
>                 taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
>
> -               if (q->flags != 0 && q->flags != taprio_flags) {
> +               if (q->flags != taprio_flags) {

You can't quite do this, since it now breaks plain 'tc qdisc add'
behavior. Can you initialize q->flags to -1 in taprio_init, just below
q->clockid, and keep the "q->flags != -1" check here? You might also
need to validate the taprio_flags to be a positive value.

>                         NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
>                         return -EOPNOTSUPP;
>                 } else if (!taprio_flags_valid(taprio_flags)) {
> --
> 2.25.0
>

Regards,
-Vladimir

Re: [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime

[Vinicius Costa Gomes]

Hi,

Vladimir Oltean <olteanv@gmail.com> writes:

>
> You can't quite do this, since it now breaks plain 'tc qdisc add'
> behavior. Can you initialize q->flags to -1 in taprio_init, just below
> q->clockid, and keep the "q->flags != -1" check here? You might also
> need to validate the taprio_flags to be a positive value.
>

Ugh, thanks for pointing this out. Will re-spin the series fixing this.
Another lesson on not sending patches last thing on a friday.


Cheers,
--
Vinicius

[PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules

[Vinicius Costa Gomes]

When any offload mode is enabled, users had to specify the
"flags" parameter when adding a new "admin" schedule.

This fix allows that parameter to be omitted when adding a new
schedule.

Also move 'taprio_flags' to a smaller scope, so we reduce the region
where we have two variables with the similar concepts ('taprio_flags'
and 'q->flags').

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 net/sched/sch_taprio.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index dfbecb9ee2f4..65357e2ba04e 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1375,7 +1375,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
 	struct tc_mqprio_qopt *mqprio = NULL;
-	u32 taprio_flags = 0;
 	unsigned long flags;
 	ktime_t start;
 	int i, err;
@@ -1389,7 +1388,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
 
 	if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
-		taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
+		u32 taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
 
 		if (q->flags != taprio_flags) {
 			NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
@@ -1402,7 +1401,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->flags = taprio_flags;
 	}
 
-	err = taprio_parse_mqprio_opt(dev, mqprio, extack, taprio_flags);
+	err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
 	if (err < 0)
 		return err;
 
@@ -1457,7 +1456,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 					       mqprio->prio_tc_map[i]);
 	}
 
-	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
+	if (FULL_OFFLOAD_IS_ENABLED(q->flags))
 		err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
 	else
 		err = taprio_disable_offload(dev, q, extack);
@@ -1477,14 +1476,14 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->txtime_delay = nla_get_u32(tb[TCA_TAPRIO_ATTR_TXTIME_DELAY]);
 	}
 
-	if (!TXTIME_ASSIST_IS_ENABLED(taprio_flags) &&
-	    !FULL_OFFLOAD_IS_ENABLED(taprio_flags) &&
+	if (!TXTIME_ASSIST_IS_ENABLED(q->flags) &&
+	    !FULL_OFFLOAD_IS_ENABLED(q->flags) &&
 	    !hrtimer_active(&q->advance_timer)) {
 		hrtimer_init(&q->advance_timer, q->clockid, HRTIMER_MODE_ABS);
 		q->advance_timer.function = advance_sched;
 	}
 
-	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
+	if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
 		q->dequeue = taprio_dequeue_offload;
 		q->peek = taprio_peek_offload;
 	} else {
@@ -1501,7 +1500,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		goto unlock;
 	}
 
-	if (TXTIME_ASSIST_IS_ENABLED(taprio_flags)) {
+	if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
 		setup_txtime(q, new_admin, start);
 
 		if (!oper) {
@@ -1528,7 +1527,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 
 		spin_unlock_irqrestore(&q->current_entry_lock, flags);
 
-		if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
+		if (FULL_OFFLOAD_IS_ENABLED(q->flags))
 			taprio_offload_config_changed(q);
 	}
 
-- 
2.25.0

Re: [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules

[Vladimir Oltean]

On Sat, 25 Jan 2020 at 02:52, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> When any offload mode is enabled, users had to specify the
> "flags" parameter when adding a new "admin" schedule.
>
> This fix allows that parameter to be omitted when adding a new
> schedule.
>
> Also move 'taprio_flags' to a smaller scope, so we reduce the region
> where we have two variables with the similar concepts ('taprio_flags'
> and 'q->flags').
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  net/sched/sch_taprio.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index dfbecb9ee2f4..65357e2ba04e 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1375,7 +1375,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>         struct taprio_sched *q = qdisc_priv(sch);
>         struct net_device *dev = qdisc_dev(sch);
>         struct tc_mqprio_qopt *mqprio = NULL;
> -       u32 taprio_flags = 0;
>         unsigned long flags;
>         ktime_t start;
>         int i, err;
> @@ -1389,7 +1388,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                 mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
>
>         if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
> -               taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
> +               u32 taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
>
>                 if (q->flags != taprio_flags) {
>                         NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
> @@ -1402,7 +1401,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                 q->flags = taprio_flags;
>         }
>
> -       err = taprio_parse_mqprio_opt(dev, mqprio, extack, taprio_flags);
> +       err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
>         if (err < 0)
>                 return err;
>
> @@ -1457,7 +1456,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                                                mqprio->prio_tc_map[i]);
>         }
>
> -       if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
> +       if (FULL_OFFLOAD_IS_ENABLED(q->flags))
>                 err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
>         else
>                 err = taprio_disable_offload(dev, q, extack);
> @@ -1477,14 +1476,14 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                 q->txtime_delay = nla_get_u32(tb[TCA_TAPRIO_ATTR_TXTIME_DELAY]);
>         }
>
> -       if (!TXTIME_ASSIST_IS_ENABLED(taprio_flags) &&
> -           !FULL_OFFLOAD_IS_ENABLED(taprio_flags) &&
> +       if (!TXTIME_ASSIST_IS_ENABLED(q->flags) &&
> +           !FULL_OFFLOAD_IS_ENABLED(q->flags) &&
>             !hrtimer_active(&q->advance_timer)) {
>                 hrtimer_init(&q->advance_timer, q->clockid, HRTIMER_MODE_ABS);
>                 q->advance_timer.function = advance_sched;
>         }
>
> -       if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
> +       if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
>                 q->dequeue = taprio_dequeue_offload;
>                 q->peek = taprio_peek_offload;
>         } else {
> @@ -1501,7 +1500,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>                 goto unlock;
>         }
>
> -       if (TXTIME_ASSIST_IS_ENABLED(taprio_flags)) {
> +       if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
>                 setup_txtime(q, new_admin, start);
>
>                 if (!oper) {
> @@ -1528,7 +1527,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>
>                 spin_unlock_irqrestore(&q->current_entry_lock, flags);
>
> -               if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
> +               if (FULL_OFFLOAD_IS_ENABLED(q->flags))
>                         taprio_offload_config_changed(q);
>         }
>
> --
> 2.25.0
>