Re: [PATCH net-next v6 02/12] net: pse-pd: Add support for reporting events

[Kory Maincent <kory.maincent@bootlin.com>]

Hello Oleksij,

On Mon, 17 Mar 2025 10:28:58 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Köry,
> 
> sorry for late review.

No worry, thank you for the review! :)
 
> On Tue, Mar 04, 2025 at 11:18:51AM +0100, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>  
> ...
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index aea0f03575689..9b41d4697a405 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -18,7 +18,8 @@ MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
> >  
> >  static struct pse_control *
> > -fwnode_find_pse_control(struct fwnode_handle *fwnode)
> > +fwnode_find_pse_control(struct fwnode_handle *fwnode,
> > +			struct phy_device *phydev)
> >  {
> >  	struct pse_control *psec;
> >  	struct device_node *np;
> > @@ -30,7 +31,7 @@ fwnode_find_pse_control(struct fwnode_handle *fwnode)
> >  	if (!np)
> >  		return NULL;
> >  
> > -	psec = of_pse_control_get(np);
> > +	psec = of_pse_control_get(np, phydev);
> >  	if (PTR_ERR(psec) == -ENOENT)
> >  		return NULL;
> >  
> > @@ -128,15 +129,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> >  	u32 phy_id;
> >  	int rc;
> >  
> > -	psec = fwnode_find_pse_control(child);
> > -	if (IS_ERR(psec))
> > -		return PTR_ERR(psec);
> > -
> >  	mii_ts = fwnode_find_mii_timestamper(child);
> > -	if (IS_ERR(mii_ts)) {
> > -		rc = PTR_ERR(mii_ts);
> > -		goto clean_pse;
> > -	}
> > +	if (IS_ERR(mii_ts))
> > +		return PTR_ERR(mii_ts);
> >  
> >  	is_c45 = fwnode_device_is_compatible(child,
> > "ethernet-phy-ieee802.3-c45"); if (is_c45 || fwnode_get_phy_id(child,
> > &phy_id)) @@ -169,6 +164,12 @@ int fwnode_mdiobus_register_phy(struct
> > mii_bus *bus, goto clean_phy;
> >  	}
> >  
> > +	psec = fwnode_find_pse_control(child, phy);
> > +	if (IS_ERR(psec)) {
> > +		rc = PTR_ERR(psec);
> > +		goto unregister_phy;
> > +	}  
> 
> Hm... we are starting to dereference the phydev pointer to a different
> framework without managing the ref-counting.
> 
> We will need to have some form of get_device(&phydev->mdio.dev).
> Normally it is done by phy_attach_direct().
> 
> And the counterpart: put_device() or phy_device_free()

The thing is that the pse_control is already related to the PHY. It is created
(pse_control_get_internal) when a PHY is
registered (fwnode_mdiobus_register_phy), and removed when the PHY is removed
(phy_device_remove).

If we add a get_device it will increase the refcount of the PHY device but we
won't be able to decrease the refcount as it will never enter the remove
callback due to that refcount.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com