Re: [PATCH] net: macb: Fix tx_ptr_lock locking

[Sean Anderson]

On 8/28/25 12:13, Robert Hancock wrote:
> On Thu, 2025-08-28 at 12:00 -0400, Sean Anderson wrote:
>> macb_start_xmit can be called with bottom-halves disabled (e.g.
>> transmitting from softirqs) as well as with interrupts disabled (with
>> netpoll). Because of this, all other functions taking tx_ptr_lock
>> must
>> disable IRQs, and macb_start_xmit must only re-enable IRQs if they
>> were already enabled.
>> 
>> Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
>> Reported-by: Mike Galbraith <efault@xxxxxx>
>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
>> ---
>> 
>>  drivers/net/ethernet/cadence/macb_main.c | 25 ++++++++++++----------
>> --
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 16d28a8b3b56..b0a8dfa341ea 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1228,7 +1228,7 @@ static int macb_tx_complete(struct macb_queue
>> *queue, int budget)
>>         int packets = 0;
>>         u32 bytes = 0;
>> 
>> -       spin_lock(&queue->tx_ptr_lock);
>> +       spin_lock_irq(&queue->tx_ptr_lock);
>> 
> 
> Hm, I think I used a non-IRQ lock here to avoid potentially disabling
> interrupts for so long during TX completion processing. I don't think I
> considered the netpoll case where start_xmit can be called with IRQs
> disabled however. Not sure if there is a better solution to satisfy
> that case without turning IRQs off entirely here?

Well, we have a single producer (macb_start_xmit) so we don't need to
take a lock to enqueue anything as long as we add barriers in the right
places.

--Sean