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

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?

Thanks,

Paolo