Re: [PATCH net-next v2 3/5] net: cadence: macb: implement EEE TX LPI support

[Nicolai Buchwitz <nb@tipi-net.de>]

On 24.2.2026 11:17, Russell King (Oracle) wrote:
> On Tue, Feb 24, 2026 at 10:18:19AM +0100, Nicolai Buchwitz wrote:
>> Implement Energy Efficient Ethernet TX Low Power Idle using phylink's
>> managed EEE framework. The Cadence GEM MAC has no built-in idle timer
>> - TXLPIEN (NCR bit 19) immediately blocks all TX when set and the MAC
>> does NOT auto-wake - so the driver uses a software delayed_work timer
>> for idle detection.
>> 
>> The TX LPI lifecycle:
>>   - phylink calls mac_enable_tx_lpi() after link-up with the 
>> negotiated
>>     timer value. The driver defers the first LPI entry by 1 second per
>>     IEEE 802.3az section 22.7a.
>>   - macb_tx_complete() reschedules the idle timer after each TX drain.
>>   - macb_start_xmit() wakes from LPI by clearing TXLPIEN, cancelling
>>     the pending work, and waiting 50us (conservative Tw_sys) before
>>     initiating the transmit.
>>   - phylink calls mac_disable_tx_lpi() before link-down, which cancels
>>     the work and clears TXLPIEN.
>> 
>> The phylink_config is populated with LPI capabilities (MII, GMII, 
>> RGMII
>> modes; 100FD and 1000FD speeds) and a 250ms default idle timer, gated
>> on MACB_CAPS_EEE.
> 
> Much better, thanks. One suggestion:

Thanks for the quick feedback! I will address this in a v3 of this 
series.

> 
>> +static void macb_tx_lpi_set(struct macb *bp, bool enable)
> 
> bool return type.
> 
>> +{
>> +	unsigned long flags;
>> +	u32 ncr;
> 
> 	u32 old, ncr;
> 
>> +
>> +	spin_lock_irqsave(&bp->lock, flags);
>> +	ncr = macb_readl(bp, NCR);
> 
> 	old = ncr;
> 
>> +	if (enable)
>> +		ncr |= GEM_BIT(TXLPIEN);
>> +	else
>> +		ncr &= ~GEM_BIT(TXLPIEN);
> 
> 	if (old != ncr)
>> +	macb_writel(bp, NCR, ncr);
>> +	bp->tx_lpi_enabled = enable;
> 
> No need for tx_lpi_enabled
> 
>> +	spin_unlock_irqrestore(&bp->lock, flags);
> 
> 	return old != ncr;
> 
>> +}
> 
> ...
> 
>> +/* Wake from LPI before transmitting. The MAC must deassert TXLPIEN
>> + * and wait for the PHY to exit LPI before any frame can be sent.
>> + * IEEE 802.3az Tw_sys is ~17us for 1000BASE-T, ~30us for 100BASE-TX;
>> + * we use a conservative 50us.
>> + */
>> +static void macb_tx_lpi_wake(struct macb *bp)
>> +{
>> +	if (!bp->tx_lpi_enabled)
>> +		return;
> 
> No need for this if().
> 
>> +
>> +	macb_tx_lpi_set(bp, false);
> 
> instead:
> 	if (!macb_tx_lpi_set(bp, false))
> 		return;
> 
> The presence of the spinlock in macb_tx_lpi_set() suggests that there
> could be races, so keeping the state completely inside the spinlock
> region would be sensible.
> 
>> +	cancel_delayed_work(&bp->tx_lpi_work);
> 
> I wonder whether this is reliable on its own. cancel_delayed_work()
> documentation says:
> 
>  * Note:
>  * The work callback function may still be running on return, unless
>  * it returns %true and the work doesn't re-arm itself.  Explicitly 
> flush or
>  * use cancel_delayed_work_sync() to wait on it.
> 
> That means macb_tx_lpi_work_fn() could have just been entered at the
> point that cancel_delayed_work() has been called. Would that cause a
> problem, e.g. setting LPI mode again, or would we be guaranteed that
> macb_tx_all_queues_idle() returns false preventing LPI mode being set
> again?

AFAIK, the race shouldn't be an issue here because macb_tx_lpi_wake()
is called from macb_start_xmit() after the TX descriptors have been
written and tx_head has been advanced (there's a wmb() before the
call to ensure visibility). So if macb_tx_lpi_work_fn() happens to
be running concurrently, macb_tx_all_queues_idle() will see
tx_head != tx_tail and return false, preventing LPI from being
re-enabled.

> 
> Thanks.

Nicolai