Re: [net-next v3 14/15] net: marvell: Convert tasklet API to new bottom half workqueue mechanism

[Russell King (Oracle)]

On Tue, Jul 30, 2024 at 10:39:51PM +0200, Andrew Lunn wrote:
> > - * Called only from mvpp2_txq_done(), called from mvpp2_tx()
> > - * (migration disabled) and from the TX completion tasklet (migration
> > - * disabled) so using smp_processor_id() is OK.
> > + * Called only from mvpp2_txq_done().
> > + *
> > + * Historically, this function was invoked directly from mvpp2_tx()
> > + * (with migration disabled) and from the bottom half workqueue.
> > + * Verify that the use of smp_processor_id() is still appropriate
> > + * considering the current bottom half workqueue implementation.
> 
> What does this mean? You want somebody else to verify this? You are
> potentially breaking this driver?

I don't see how, the only thing that's changing in mvpp2 seems to be
an outdated comment that happens to mention a tasklet, but the
driver doesn't use tasklets.

Let's look at the original comment which claims what the call sites
are:

static void mvpp2_txq_done(struct mvpp2_port *port, struct mvpp2_tx_queue *txq,
                           struct mvpp2_txq_pcpu *txq_pcpu)
{
...
        tx_done = mvpp2_txq_sent_desc_proc(port, txq);

and that is it. _This_ function is called from several places:

mvpp2_tx_done()
mvpp2_xdp_finish_tx()
mvpp2_tx()

So I suppose that the original comment was referring to the
mvpp2_tx() -> mvpp2_txq_done() -> mvpp2_txq_sent_desc_proc() call path,
and the others were added over time.

mvpp2_tx_done() is called from mvpp2_hr_timer_cb(), and yes, back in
the distant history there was a tasklet here - see:

ecb9f80db23a net/mvpp2: Replace tasklet with softirq hrtimer

So, the comment referring to a tasklet was left over from that commit
and never fixed up.

Given this, I don't think the new paragraph starting "Historically"
is correct (or even relevant) as I think it misinterprets the original
comment - and "this function" is ambiguous in it, but either way its
still wrong.

If we assume that "this function" refers to the one below the comment,
then this has never been called directly from mvpp2_tx() nor the
tasklet, and talking about a bottom half workqueue makes no sense
because "historically" it's never been called from a bottom half
workqueue.

If we assume that "this function" refers to mvpp2_txq_done(), then
it's not historical that this was called from mvpp2_tx(), because it
still is today. And the bit about being called from a bottom half
workqueue is still false.

Given that bottom half workqueues have absolutely nothing to do with
this code path, the sentence beginning with "Verify" seems totally
irrelevant (at least to me.)

So, I think I've comprehensively ripped the new comment to shreds.
It would be far better to leave the driver alone and not change the
comment despite it incorrectly referring to a tasklet that has
already been eliminated (and at least was historically correct),
rather than the new comment which just seems wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!