linux-kernel - Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aaf014b1-d0c0-491e-99fb-9d1eb5abafb3@lunn.ch>
Date: Sat, 26 Jul 2025 17:32:43 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "claudiu beznea (tuxon)" <claudiu.beznea@...on.dev>
Cc: Vineeth Karumanchi <vineeth.karumanchi@....com>,
	nicolas.ferre@...rochip.com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, git@....com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE
 command offload support

> > +	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
> 
> To simplify the error path you can use something like:
> 
>         struct queue_enst_configs *enst_queue __free(kfree) = kcalloc(...);
> 
> and drop the "goto cleanup" below.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

1.6.5. Using device-managed and cleanup.h constructs

Netdev remains skeptical about promises of all “auto-cleanup” APIs,
including even devm_ helpers, historically. They are not the preferred
style of implementation, merely an acceptable one.

Use of guard() is discouraged within any function longer than 20
lines, scoped_guard() is considered more readable. Using normal
lock/unlock is still (weakly) preferred.

Low level cleanup constructs (such as __free()) can be used when
building APIs and helpers, especially scoped iterators. However,
direct use of __free() within networking core and drivers is
discouraged. Similar guidance applies to declaring variables
mid-function.

> 
> You can use guard(spinlock_irqsave)(&bp->lock) or
> scoped_guard(spinlock_irqsave, &bp->lock)

scoped_guard() if anything.

> 
> > +
> > +	/* Disable ENST queues if running before configuring */
> > +	if (gem_readl(bp, ENST_CONTROL))
> 
> Is this read necessary?
> 
> > +		gem_writel(bp, ENST_CONTROL,
> > +			   GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET);
> 
> This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if you
> define GEM_ENST_DISABLE_QUEUE_SIZE along with GEM_ENST_DISABLE_QUEUE_OFFSET.
> 
> > +
> > +	for (i = 0; i < conf->num_entries; i++) {
> > +		queue = &bp->queues[enst_queue[i].queue_id];
> > +		/* Configure queue timing registers */
> > +		queue_writel(queue, ENST_START_TIME, enst_queue[i].start_time_mask);
> > +		queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
> > +		queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
> > +	}
> > +
> > +	/* Enable ENST for all configured queues in one write */
> > +	gem_writel(bp, ENST_CONTROL, configured_queues);
> 
> Can this function be executed while other queues are configured? If so,
> would the configured_queues contains it (as well as conf)?
> 
> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
> > +		    conf->num_entries, hweight32(configured_queues));

guard() would put that netdev_info() print inside the guard. Do you
really want to be doing a print, inside a spinlock, with interrupts
potentially disabled? This is one of the reasons i don't like this
magical guard() construct, it is easy to forget how the magic works
and end up with sub optimal code. A scoped_guard() would avoid this
issue. You have to think about where you want to lock released in
order to place the } .

	Andrew