Message-ID: <fd4bb158-5138-4fc9-ad1f-d0002ec0fee4@bootlin.com>
Date: Wed, 28 Jan 2026 11:02:13 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Wei Fang <wei.fang@....com>, "andrew@...n.ch" <andrew@...n.ch>,
 "hkallweit1@...il.com" <hkallweit1@...il.com>,
 "linux@...linux.org.uk" <linux@...linux.org.uk>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
 <pabeni@...hat.com>,
 "florian.fainelli@...adcom.com" <florian.fainelli@...adcom.com>
Cc: "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: phy: micrel: fix clk warning when removing the
 driver



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@....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

Powered by blists - more mailing lists