Message-ID:
 <AM9PR04MB8505744A93921B488AD9505C8891A@AM9PR04MB8505.eurprd04.prod.outlook.com>
Date: Wed, 28 Jan 2026 09:36:24 +0000
From: Wei Fang <wei.fang@....com>
To: Maxime Chevallier <maxime.chevallier@...tlin.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 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.

Powered by blists - more mailing lists