Re: [PATCH net v2] net: stmmac: resume PHY before hardware setup when opening the interface

Hi Paolo, hi Maxime,

On 2026-07-21 10:12, Paolo Abeni wrote:
> Hi,
> 
> On 7/8/26 11:16 AM, Maxime Chevallier wrote:
>> On 7/7/26 21:54, Stefan Agner wrote:
>>> Since the referenced commit, changing the MTU on a running interface no
>>> longer disconnects and reconnects the PHY; __stmmac_release() merely
>>> stops phylink, which also suspends the PHY (BMCR power-down) when WoL
>>> is not enabled. __stmmac_open() then performs the DMA software reset in
>>> stmmac_hw_setup() before phylink_start() resumes the PHY again.
>>>
>>> IEEE 802.3 22.2.4.1.5 allows a PHY to stop its receive clock while
>>> powered down, and stmmac requires a running receive clock for the DMA
>>> software reset to complete (the phylink config sets mac_requires_rxc).
>>> On such setups, e.g. the RK3566-based Home Assistant Green with an
>>> RTL8211F-VD PHY in RGMII mode, any runtime MTU change now times out and
>>> leaves the interface dead:
>>>
>>>   rk_gmac-dwmac fe010000.ethernet end0: Failed to reset the dma
>>>   rk_gmac-dwmac fe010000.ethernet end0: stmmac_hw_setup: DMA engine initialization failed
>>>   rk_gmac-dwmac fe010000.ethernet end0: __stmmac_open: Hw setup failed
>>>   rk_gmac-dwmac fe010000.ethernet end0: failed reopening the interface after MTU change
>>>
>>> In the field this is triggered by NetworkManager applying an MTU while
>>> activating the connection, breaking networking entirely.
>>>
>>> Resume the PHY in __stmmac_open() before the hardware setup, making it
>>> the counterpart of the phylink_stop() in __stmmac_release(), like
>>> stmmac_resume() already does for the same reason. phylink_start() also
>>> resumes the PHY, but only after stmmac_hw_setup(), and it cannot be
>>> moved before the hardware setup since it may bring the link up
>>> immediately from a workqueue, racing with the initialization (see the
>>> comment in stmmac_resume()). For the regular ndo_open path the PHY has
>>> just been attached and is not suspended, in which case
>>> phylink_prepare_resume() does nothing.
>>>
>>> Fixes: db299a0c09e9 ("net: stmmac: move PHY handling out of __stmmac_open()/release()")
>>> Link: https://github.com/home-assistant/operating-system/issues/4858
>>> Signed-off-by: Stefan Agner <[email protected]>
>> 
>> I was able to reproduce the issue on imx8mp and socfpga. Adding this case to
>> my periodic test list...
>> 
>> Indeed the assymetry isn't very nice, OTOH there's not phylink counterpart
>> for phylink_prepare_resume(). This helper was added for the suspend/resume
>> case, and only for it as the doc states, it just happens to do exactly
>> what we need to fix the issue :
>> 
>> /**
>>  * phylink_prepare_resume() - prepare to resume a network device
>>  * @pl: a pointer to a &struct phylink returned from phylink_create()
>>  *
>>  * Optional, but if called must be called prior to phylink_resume().
>>  *
>>  * Prepare to resume a network device, preparing the PHY as necessary.
>>  */
>> void phylink_prepare_resume(struct phylink *pl)
>> 
>> I think this helper should be renamed and the doc updated, stmmac is the
>> sole user, and it's really about controlling that rxc and not about
>> suspend / resume. Resuming from suspend is just one of the cases where we
>> need that RXC early on.
>> 
>> So either something like 'phylink_prepare_start_or_resume' but it's long,
>> or maybe we can be more explicit about it and simply call it:
>> 
>> phylink_start_rxc(pl)
>> 
>> (without a corresponding stop)
> I read the above as a possible follow-up more than actual changes
> requested to this patch, am I correct?

Since this fixes a bug, I'd prefer to keep the patch minimal to make it
easier to backport. Therefor I would prefer to do make such structural
changes as a follow-up change rather than part of this patch.

Does that work for you, Maxime?

--
Stefan