[PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Heiner Kallweit]

Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
Easiest fix is to call fixed_phy_del() for each possible phy address.
This may consume a few cpu cycles more, but is much easier to read.

Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
Cc: stable@vger.kernel.org
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebase for net
v3:
- add missing blamed author
---
 drivers/net/phy/fixed_phy.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 033656d57..a1db96944 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -352,17 +352,13 @@ module_init(fixed_mdio_bus_init);
 static void __exit fixed_mdio_bus_exit(void)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
-	struct fixed_phy *fp, *tmp;
 
 	mdiobus_unregister(fmb->mii_bus);
 	mdiobus_free(fmb->mii_bus);
 	faux_device_destroy(fdev);
 
-	list_for_each_entry_safe(fp, tmp, &fmb->phys, node) {
-		list_del(&fp->node);
-		kfree(fp);
-	}
-	ida_destroy(&phy_fixed_ida);
+	for (int i = 0; i < PHY_MAX_ADDR; i++)
+		fixed_phy_del(i);
 }
 module_exit(fixed_mdio_bus_exit);
 
-- 
2.50.1

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Russell King (Oracle)]

On Wed, Aug 27, 2025 at 11:02:55PM +0200, Heiner Kallweit wrote:
> Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
> Easiest fix is to call fixed_phy_del() for each possible phy address.
> This may consume a few cpu cycles more, but is much easier to read.
> 
> Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")

Here's a question that should be considered as well. Do we still need
to keep the link-gpios for fixed-phy?

$ grep -r link-gpios arch/*/boot/dts/
arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 2
arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 3

These are used with the mv88e6xxx DSA switch, and DSA being fully
converted to phylink, means that fixed-phy isn't used for these
link-gpios properties, and hasn't been for some time.

So, is this now redundant code that can be removed, or should we
consider updating it for another kernel cycle but print a deprecation
notice should someone use it (e.g. openwrt.)

Should we also describe the SFF modules on Zii rev B properly?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Florian Fainelli]

On 8/27/25 14:09, Russell King (Oracle) wrote:
> On Wed, Aug 27, 2025 at 11:02:55PM +0200, Heiner Kallweit wrote:
>> Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
>> Easiest fix is to call fixed_phy_del() for each possible phy address.
>> This may consume a few cpu cycles more, but is much easier to read.
>>
>> Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
> 
> Here's a question that should be considered as well. Do we still need
> to keep the link-gpios for fixed-phy?
> 
> $ grep -r link-gpios arch/*/boot/dts/
> arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 2
> arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 3
> 
> These are used with the mv88e6xxx DSA switch, and DSA being fully
> converted to phylink, means that fixed-phy isn't used for these
> link-gpios properties, and hasn't been for some time.
> 
> So, is this now redundant code that can be removed, or should we
> consider updating it for another kernel cycle but print a deprecation
> notice should someone use it (e.g. openwrt.)
> 
> Should we also describe the SFF modules on Zii rev B properly?
> 

Do we need to maintain the ZII Device Tree sources given that there has 
not been any work done on those, and it's unclear if they are still even 
as useful as they once were?
-- 
Florian

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Russell King (Oracle)]

On Wed, Aug 27, 2025 at 02:14:54PM -0700, Florian Fainelli wrote:
> On 8/27/25 14:09, Russell King (Oracle) wrote:
> > On Wed, Aug 27, 2025 at 11:02:55PM +0200, Heiner Kallweit wrote:
> > > Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
> > > Easiest fix is to call fixed_phy_del() for each possible phy address.
> > > This may consume a few cpu cycles more, but is much easier to read.
> > > 
> > > Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
> > 
> > Here's a question that should be considered as well. Do we still need
> > to keep the link-gpios for fixed-phy?
> > 
> > $ grep -r link-gpios arch/*/boot/dts/
> > arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 2
> > arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 3
> > 
> > These are used with the mv88e6xxx DSA switch, and DSA being fully
> > converted to phylink, means that fixed-phy isn't used for these
> > link-gpios properties, and hasn't been for some time.
> > 
> > So, is this now redundant code that can be removed, or should we
> > consider updating it for another kernel cycle but print a deprecation
> > notice should someone use it (e.g. openwrt.)
> > 
> > Should we also describe the SFF modules on Zii rev B properly?
> > 
> 
> Do we need to maintain the ZII Device Tree sources given that there has not
> been any work done on those, and it's unclear if they are still even as
> useful as they once were?

Last time I booted mine was in April, because it's the only way I can
test a greater range of Marvell DSA switches. So yes, it gets used
for testing here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Andrew Lunn]

> Last time I booted mine was in April

Mine died a few years ago.

It is quiet an interesting board for testing because it has an older
generation switch.

So we should keep it, while there is still an example in use.

	Andrew

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Heiner Kallweit]

On 8/27/2025 11:09 PM, Russell King (Oracle) wrote:
> On Wed, Aug 27, 2025 at 11:02:55PM +0200, Heiner Kallweit wrote:
>> Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
>> Easiest fix is to call fixed_phy_del() for each possible phy address.
>> This may consume a few cpu cycles more, but is much easier to read.
>>
>> Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
> 
> Here's a question that should be considered as well. Do we still need
> to keep the link-gpios for fixed-phy?
> 
> $ grep -r link-gpios arch/*/boot/dts/
> arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 2
> arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 3
> 
> These are used with the mv88e6xxx DSA switch, and DSA being fully
> converted to phylink, means that fixed-phy isn't used for these
> link-gpios properties, and hasn't been for some time.
> 
Means we can remove all "fixed-link" blocks from vf610-zii-dev-rev-b.dts?

> So, is this now redundant code that can be removed, or should we
> consider updating it for another kernel cycle but print a deprecation
> notice should someone use it (e.g. openwrt.)
> 
Good question. I just grepped the openwrt repo for link-gpios,
and there's no use either.

link-gpio is referenced here (apart from vf610-zii-dev-rev-b.dts):
Documentation/devicetree/bindings/net/ethernet-controller.yaml
Documentation/devicetree/bindings/net/nixge.txt

So once we have removed link-gpios from vf610-zii-dev-rev-b.dts,
I'd propose to remove fixed phy gpio support and see whether
any out-of-tree user complains.

> Should we also describe the SFF modules on Zii rev B properly?
> 

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Russell King (Oracle)]

On Wed, Aug 27, 2025 at 11:29:43PM +0200, Heiner Kallweit wrote:
> On 8/27/2025 11:09 PM, Russell King (Oracle) wrote:
> > On Wed, Aug 27, 2025 at 11:02:55PM +0200, Heiner Kallweit wrote:
> >> Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
> >> Easiest fix is to call fixed_phy_del() for each possible phy address.
> >> This may consume a few cpu cycles more, but is much easier to read.
> >>
> >> Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
> > 
> > Here's a question that should be considered as well. Do we still need
> > to keep the link-gpios for fixed-phy?
> > 
> > $ grep -r link-gpios arch/*/boot/dts/
> > arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 2
> > arch/arm/boot/dts/nxp/vf/vf610-zii-dev-rev-b.dts:                              link-gpios = <&gpio6 3
> > 
> > These are used with the mv88e6xxx DSA switch, and DSA being fully
> > converted to phylink, means that fixed-phy isn't used for these
> > link-gpios properties, and hasn't been for some time.
> > 
> Means we can remove all "fixed-link" blocks from vf610-zii-dev-rev-b.dts?

phylink has support for it (because phylink needed to be 100%
compatible to avoid regressions!) Thus, there's no need to remove it,
nor drop it from the DT bindings.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 net] net: phy: fixed_phy: fix missing calls to gpiod_put in fixed_mdio_bus_exit

[Heiner Kallweit]

On 8/27/2025 11:02 PM, Heiner Kallweit wrote:
> Cleanup in fixed_mdio_bus_exit() misses to call gpiod_put().
> Easiest fix is to call fixed_phy_del() for each possible phy address.
> This may consume a few cpu cycles more, but is much easier to read.
> 
> Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rebase for net
> v3:
> - add missing blamed author
> ---
>  drivers/net/phy/fixed_phy.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index 033656d57..a1db96944 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -352,17 +352,13 @@ module_init(fixed_mdio_bus_init);
>  static void __exit fixed_mdio_bus_exit(void)
>  {
>  	struct fixed_mdio_bus *fmb = &platform_fmb;
> -	struct fixed_phy *fp, *tmp;
>  
>  	mdiobus_unregister(fmb->mii_bus);
>  	mdiobus_free(fmb->mii_bus);
>  	faux_device_destroy(fdev);
>  
> -	list_for_each_entry_safe(fp, tmp, &fmb->phys, node) {
> -		list_del(&fp->node);
> -		kfree(fp);
> -	}
> -	ida_destroy(&phy_fixed_ida);
> +	for (int i = 0; i < PHY_MAX_ADDR; i++)
> +		fixed_phy_del(i);
>  }
>  module_exit(fixed_mdio_bus_exit);
>  
--
pw-bot: cr