[PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[Vinicius Costa Gomes]

When configuring a taprio instance if "flags" is not specified (or
it's zero), taprio currently replies with an "Invalid argument" error.

So, set the return value to zero after we are done with all the
checks.

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

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 68b543f85a96..6719a65169d4 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1341,6 +1341,10 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
 		NL_SET_ERR_MSG(extack, "Specifying a 'clockid' is mandatory");
 		goto out;
 	}
+
+	/* Everything went ok, return success. */
+	err = 0;
+
 out:
 	return err;
 }
-- 
2.23.0

Re: [PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[Vladimir Oltean]

Hi Vinicius,

On Wed, 9 Oct 2019 at 02:19, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> When configuring a taprio instance if "flags" is not specified (or
> it's zero), taprio currently replies with an "Invalid argument" error.
>
> So, set the return value to zero after we are done with all the
> checks.
>
> Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---

You mean clockid, not flags, right?
Otherwise the patch looks correct, sorry for the bug.
Once you fix the commit message:

Acked-by: Vladimir Oltean <olteanv@gmail.com>

>  net/sched/sch_taprio.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 68b543f85a96..6719a65169d4 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1341,6 +1341,10 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
>                 NL_SET_ERR_MSG(extack, "Specifying a 'clockid' is mandatory");
>                 goto out;
>         }
> +
> +       /* Everything went ok, return success. */
> +       err = 0;
> +
>  out:
>         return err;
>  }
> --
> 2.23.0
>

Re: [PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[Vladimir Oltean]

On Wed, 9 Oct 2019 at 02:58, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Vinicius,
>
> On Wed, 9 Oct 2019 at 02:19, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
> >
> > When configuring a taprio instance if "flags" is not specified (or
> > it's zero), taprio currently replies with an "Invalid argument" error.
> >
> > So, set the return value to zero after we are done with all the
> > checks.
> >
> > Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> > ---
>
> You mean clockid, not flags, right?
> Otherwise the patch looks correct, sorry for the bug.
> Once you fix the commit message:
>
> Acked-by: Vladimir Oltean <olteanv@gmail.com>
>

No, you are actually right to phrase it this way, but the description
is still confusing. When 'flags' is zero, the driver takes a code path
which is buggy and always returns a negative error code. Although I'm
not really sure how much better this can be phrased. I'm fine with the
description now too.

> >  net/sched/sch_taprio.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index 68b543f85a96..6719a65169d4 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -1341,6 +1341,10 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
> >                 NL_SET_ERR_MSG(extack, "Specifying a 'clockid' is mandatory");
> >                 goto out;
> >         }
> > +
> > +       /* Everything went ok, return success. */
> > +       err = 0;
> > +
> >  out:
> >         return err;
> >  }
> > --
> > 2.23.0
> >

Re: [PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[Vinicius Costa Gomes]

Hi,

Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Vinicius,
>
> On Wed, 9 Oct 2019 at 02:19, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> When configuring a taprio instance if "flags" is not specified (or
>> it's zero), taprio currently replies with an "Invalid argument" error.
>>
>> So, set the return value to zero after we are done with all the
>> checks.
>>
>> Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>
> You mean clockid, not flags, right?

I do mean "flags", yeah. The case I was testing was something like this:

$ tc qdisc replace dev $IFACE parent root handle 100 taprio \
      num_tc 3 \
      map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
      queues 1@0 1@1 2@2 \
      base-time $BASE_TIME \
      sched-entry S 01 300000 \
      sched-entry S 02 300000 \
      sched-entry S 04 400000 \
      clockid CLOCK_TAI


Cheers,
--
Vinicius

Re: [PATCH net v1] net: taprio: Fix returning EINVAL when configuring without flags

[Jakub Kicinski]

On Tue,  8 Oct 2019 16:20:07 -0700, Vinicius Costa Gomes wrote:
> When configuring a taprio instance if "flags" is not specified (or
> it's zero), taprio currently replies with an "Invalid argument" error.
> 
> So, set the return value to zero after we are done with all the
> checks.
> 
> Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Applied, thanks.