hello Jakub, and thanks for checking!

On Thu, 2020-12-17 at 11:05 -0800, Jakub Kicinski wrote:

On Wed, 16 Dec 2020 19:33:29 +0100 Davide Caratti wrote:

quoted

+	if (q->qdiscs) {
+		for (i = 0; i < dev->num_tx_queues && q->qdiscs[i]; i++)
+			qdisc_reset(q->qdiscs[i]);

Are you sure that we can't graft a NULL in the middle of the array?

that should not happen, because child qdiscs are checked for being non-
NULL when they are created:

https://elixir.bootlin.com/linux/v5.10/source/net/sched/sch_taprio.c#L1674

and then assigned to q->qdiscs[i]. So, there might be NULL elements of
q->qdiscs[] in the middle of the array when taprio_reset() is called,
but it should be ok to finish the loop when we encounter the first one:
subsequent ones should be NULL as well.

Shouldn't this be:

	for (i = 0; i < dev->num_tx_queues; i++)
		if (q->qdiscs[i])
			qdisc_reset(q->qdiscs[i]);

?

probably the above code is more robust, but - like you noticed - then we
should also change the same 'for' loop in taprio_destroy(), otherwise it
leaks resources. If you and Vinicius agree, I can post a follow-up patch
that makes ->reset() and ->destroy()  more consistent with ->enqueue() 
and ->dequeue(), and send it for net-next when it reopens. Ok?

-- 
davide