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

[Andrew Lunn <andrew@lunn.ch>]

> > +	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