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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 7/31/26 12:03, Stefan Agner wrote:
> 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 <stefan@agner.ch>
>>>
>>> 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?

That works for me yeah, I agree we need that fixed.

Thanks Stefan,

Maxime