Re: [PATCH net] net: phy: micrel: fix clk warning when removing the driver

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

On 28/01/2026 10:36, Wei Fang wrote:
>> On 26/01/2026 09:15, Wei Fang wrote:
>>> Since the commit 25c6a5ab151f ("net: phy: micrel: Dynamically control
>>> external clock of KSZ PHY"), the clock of Micrel PHY has been enabled
>>> by phy_driver::resume() and disabled by phy_driver::suspend(). However,
>>> devm_clk_get_optional_enabled() is used in kszphy_probe(), so the clock
>>> will automatically be disabled when the device is unbound from the bus.
>>> Therefore, this could cause the clock to be disabled twice, resulting
>>> in clk driver warnings.
>>>
>>> For example, this issue can be reproduced on i.MX6ULL platform, and we
>>> can see the following logs when removing the FEC MAC drivers.
>>>
>>> $ echo 2188000.ethernet > /sys/bus/platform/drivers/fec/unbind
>>> $ echo 20b4000.ethernet > /sys/bus/platform/drivers/fec/unbind
>>> [  109.758207] ------------[ cut here ]------------
>>> [  109.758240] WARNING: drivers/clk/clk.c:1188 at
>> clk_core_disable+0xb4/0xd0, CPU#0: sh/639
>>> [  109.771011] enet2_ref already disabled
>>> [  109.793359] Call trace:
>>> [  109.822006]  clk_core_disable from clk_disable+0x28/0x34
>>> [  109.827340]  clk_disable from clk_disable_unprepare+0xc/0x18
>>> [  109.833029]  clk_disable_unprepare from devm_clk_release+0x1c/0x28
>>> [  109.839241]  devm_clk_release from devres_release_all+0x98/0x100
>>> [  109.845278]  devres_release_all from device_unbind_cleanup+0xc/0x70
>>> [  109.851571]  device_unbind_cleanup from
>> device_release_driver_internal+0x1a4/0x1f4
>>> [  109.859170]  device_release_driver_internal from
>> bus_remove_device+0xbc/0xe4
>>> [  109.866243]  bus_remove_device from device_del+0x140/0x458
>>> [  109.871757]  device_del from phy_mdio_device_remove+0xc/0x24
>>> [  109.877452]  phy_mdio_device_remove from
>> mdiobus_unregister+0x40/0xac
>>> [  109.883918]  mdiobus_unregister from
>> fec_enet_mii_remove+0x40/0x78
>>> [  109.890125]  fec_enet_mii_remove from fec_drv_remove+0x4c/0x158
>>> [  109.896076]  fec_drv_remove from
>> device_release_driver_internal+0x17c/0x1f4
>>> [  109.962748] WARNING: drivers/clk/clk.c:1047 at
>> clk_core_unprepare+0xfc/0x13c, CPU#0: sh/639
>>> [  109.975805] enet2_ref already unprepared
>>> [  110.002866] Call trace:
>>> [  110.031758]  clk_core_unprepare from clk_unprepare+0x24/0x2c
>>> [  110.037440]  clk_unprepare from devm_clk_release+0x1c/0x28
>>> [  110.042957]  devm_clk_release from devres_release_all+0x98/0x100
>>> [  110.048989]  devres_release_all from device_unbind_cleanup+0xc/0x70
>>> [  110.055280]  device_unbind_cleanup from
>> device_release_driver_internal+0x1a4/0x1f4
>>> [  110.062877]  device_release_driver_internal from
>> bus_remove_device+0xbc/0xe4
>>> [  110.069950]  bus_remove_device from device_del+0x140/0x458
>>> [  110.075469]  device_del from phy_mdio_device_remove+0xc/0x24
>>> [  110.081165]  phy_mdio_device_remove from
>> mdiobus_unregister+0x40/0xac
>>> [  110.087632]  mdiobus_unregister from
>> fec_enet_mii_remove+0x40/0x78
>>> [  110.093836]  fec_enet_mii_remove from fec_drv_remove+0x4c/0x158
>>> [  110.099782]  fec_drv_remove from
>> device_release_driver_internal+0x17c/0x1f4
>>>
>>> After analyzing the process of removing the FEC driver, as shown below,
>>> it can be seen that the clock was disabled twice by the PHY driver.
>>>
>>> fec_drv_remove()
>>>   --> fec_enet_close()
>>>     --> phy_stop()
>>>       --> phy_suspend()
>>>         --> kszphy_suspend() #1 The clock is disabled
>>>   --> fec_enet_mii_remove()
>>>     --> mdiobus_unregister()
>>>       --> phy_mdio_device_remove()
>>>         --> device_del()
>>>           --> devm_clk_release() #2 The clock is disabled again
>>>
>>> Therefore, devm_clk_get_optional() is used to fix the above issue. And
>>> to avoid the issue mentioned by the commit 985329462723 ("net: phy:
>>> micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"), the
>>> clock is enabled by clk_prepare_enable() to get the correct clock rate.
>>>
>>> Fixes: 25c6a5ab151f ("net: phy: micrel: Dynamically control external clock of
>> KSZ PHY")
>>> Signed-off-by: Wei Fang <wei.fang@nxp.com>
>>
>> This should fix the issue indeed. The dance of enabling/disabling then
>> re-enabling at resume() again feels odd, but at least it's balanced.
>>
>> We may be able to avoid that entirely by dealing with the ref_mii_clk
>> selection only in the .resume() path, as it's also called no long after
>> config_init(). That would mean reading the clock rate in
>> kszphy_config_reset(), then performing the kszphy_rmii_clk_sel()
>> accordingly.
>>
> 
> It seems rather strange to get the clock rate every time the .resume()
> is called. Otherwise, we would need to add some additional logic to 
> kszphy_rmii_clk_sel() to ensure that the clock rate is only obtained
> once. IMO, it would be more reasonable to only get the clock rate in
> the .probe() during the lifetime of the PHY driver.
> 
> Furthermore, this seems to break the previous logic: when the clock
> rate is incorrect, the .probe() will return an error, meaning the PHY
> driver will not be loaded.
> 

Makes sense to validate it early indeed :) All in all, this seems ok then.

Maxime