Re: [net-next PATCH] taprio: don't reject same mqprio settings

[Vinicius Costa Gomes]

Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxxxxxx> writes:

> The taprio qdisc allows to set mqprio setting but only once. In case
> if mqprio settings are provided next time the error is returned as
> it's not allowed to change traffic class mapping in-flignt and that
> is normal. But if configuration is absolutely the same - no need to
> return error. It allows to provide same command couple times,
> changing only base time for instance, or changing only scheds maps,
> but leaving mqprio setting w/o modification. It more corresponds the
> message: "Changing the traffic mapping of a running schedule is not
> supported", so reject mqprio if it's really changed.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxxxxxx>
> ---
>  net/sched/sch_taprio.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 7cd68628c637..bd844f2cbf7a 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1347,6 +1347,26 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
>  	return err;
>  }
>  
> +static int taprio_mqprio_cmp(struct net_device *dev,
> +			     struct tc_mqprio_qopt *mqprio)

Nitpick: for these kinds of functions I like to add a 'const' to the parameters
at least as documentation that it doesn't modify its arguments.

> +{
> +	int i;
> +
> +	if (mqprio->num_tc != dev->num_tc)
> +		return -1;

Optional: you could move the check for a NULL mqprio inside this
function. Perhaps, for that to make sense you would need to change the
function name to taprio_mqprio_check() or something.

These are all optional.

Acked-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>