netdev - Re: [PATCH net-next v5 1/4] net: mdio: common handling of phy reset properties

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQId3lVoDBE0557D@debianbuilder>
Date: Wed, 29 Oct 2025 14:59:58 +0100
From: Buday Csaba <buday.csaba@...lan.hu>
To: Andrew Lunn <andrew@...n.ch>
CC: Heiner Kallweit <hkallweit1@...il.com>, Russell King
	<linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Philipp Zabel <p.zabel@...gutronix.de>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v5 1/4] net: mdio: common handling of phy reset
 properties

On Wed, Oct 29, 2025 at 02:03:30PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 11:23:41AM +0100, Buday Csaba wrote:
> > Unify the handling of reset properties for an `mdio_device`.
> > Replace mdiobus_register_gpiod() and mdiobus_register_reset() with
> > mdio_device_register_reset() and mdio_device_unregister_reset(),
> > and move them from mdio_bus.c to mdio_device.c, where they belong.
> 
> You should probably mention here that there are two sets of reset
> properties. One set applies to the bus as a whole, and the second is
> per device on the bus. You would expect the whole bus properties to be
> handled in mdio_bus.c, where as the per device properties might make
> more sense in mdio_device.c. So you can comment you are only moving
> the per device reset code.
> 

Okay, I will do that.

> > 
> > The new functions handle both reset-controllers and reset-gpios,
> > and also read the corresponding firmware properties from the
> > device tree, which were previously handled in fwnode_mdio.c.
> > This makes tracking the reset properties easier.
> 
> Please split this patch.
> 
> It is normal when moving a function to just move it, make no
> additional changes. That makes the change smaller, easier to review,
> since it is all about, does the new location make sense?
> 
> Once it is in the new location, you can then have patches which
> refactor it.

Understood, I will change it accordingly.

> 
> > -static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> > -{
> > -	/* Deassert the optional reset signal */
> > -	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> > -						 "reset", GPIOD_OUT_LOW);
> > -	if (IS_ERR(mdiodev->reset_gpio))
> > -		return PTR_ERR(mdiodev->reset_gpio);
> > -
> > -	if (mdiodev->reset_gpio)
> > -		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
> > -
> > -	return 0;
> > -}
> > -
> 
> > +/**
> > + * mdio_device_register_reset - Read and initialize the reset properties of
> > + *				an mdio device
> > + * @mdiodev: mdio_device structure
> > + *
> > + * Return: Zero if successful, negative error code on failure
> > + */
> > +int mdio_device_register_reset(struct mdio_device *mdiodev)
> > +{
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mdio_device_register_reset);
> 
> Before it did not require an EXPORT_SYMBOL, but now it does?  That
> makes me wounder if it is in the correct place. This should be a core
> function, why would something outside of the core need it?

I am using these in the third patch in fwnode_mdio.c, so I assumed it was
necessary. But you are right, neither can be in a module. I will remove it.

> 
> Also, please use the _GPL variant.
> 
> 	Andrew
> 

Csaba