Re: [PATCH net-next v9 8/9] net: txgbe: Implement phylink pcs

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

On Fri, May 26, 2023 at 05:22:29PM +0800, Jiawen Wu wrote:
> On Friday, May 26, 2023 5:07 PM, Russell King (Oracle) wrote:
> > On Fri, May 26, 2023 at 05:01:49PM +0800, Jiawen Wu wrote:
> > > On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote:
> > > > On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> > > > > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > > > > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > > > > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > > > > > +	if (IS_ERR(mdiodev))
> > > > > > > +		return PTR_ERR(mdiodev);
> > > > > > > +
> > > > > > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > > > > > +	if (IS_ERR(xpcs)) {
> > > > > > > +		mdio_device_free(mdiodev);
> > > > > > > +		return PTR_ERR(xpcs);
> > > > > > > +	}
> > > > > >
> > > > > > How does the mdiodev get destroyed in case of success?
> > > > > > Seems like either freeing it in case of xpcs error is unnecessary
> > > > > > or it needs to also be freed when xpcs is destroyed?
> > > > >
> > > > > When xpcs is destroyed, that means mdiodev is no longer needed.
> > > > > I think there is no need to free mdiodev in case of xpcs error,
> > > > > since devm_* function leads to free it.
> > > >
> > > > If you are relying on the devm-ness of devm_mdiobus_register() then
> > > > it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
> > > > think you are assuming that the mdio device you've created in
> > > > mdio_device_create() will be in that array. MDIO devices only get
> > > > added to that array when mdiobus_register_device() has been called,
> > > > which must only be called from mdio_device_register().
> > > >
> > > > Please arrange to call mdio_device_free() prior to destroying the
> > > > XPCS in every case.
> > >
> > > Get it.
> > 
> > It seems this is becoming a pattern, so I think we need to solve it
> > differently. How about something like this, which means you only have
> > to care about calling xpcs_create_mdiodev() and xpcs_destroy() ?
> > 
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index b87c69c4cdd7..802222581feb 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> >  	if (!xpcs)
> >  		return ERR_PTR(-ENOMEM);
> > 
> > +	mdio_device_get(mdiodev);
> >  	xpcs->mdiodev = mdiodev;
> > 
> >  	xpcs_id = xpcs_get_id(xpcs);
> > @@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> >  	ret = -ENODEV;
> > 
> >  out:
> > +	mdio_device_put(mdiodev);
> >  	kfree(xpcs);
> > 
> >  	return ERR_PTR(ret);
> > @@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create);
> > 
> >  void xpcs_destroy(struct dw_xpcs *xpcs)
> >  {
> > +	mdio_device_put(mdiodev);
> >  	kfree(xpcs);
> >  }
> >  EXPORT_SYMBOL_GPL(xpcs_destroy);
> > 
> > +struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
> > +				    phy_interface_t interface)
> > +{
> > +	struct mdio_device *mdiodev;
> > +	struct dw_xpcs *xpcs;
> > +
> > +	mdiodev = mdio_device_create(bus, addr);
> > +	if (IS_ERR(mdiodev))
> > +		return ERR_CAST(mdiodev);
> > +
> > +	xpcs = xpcs_create(mdiodev, interface);
> > +
> > +	/* xpcs_create() has taken a refcount on the mdiodev if it was
> > +	 * successful. If xpcs_create() fails, this will free the mdio
> > +	 * device here. In any case, we don't need to hold our reference
> > +	 * anymore, and putting it here will allow mdio_device_put() in
> > +	 * xpcs_destroy() to automatically free the mdio device.
> > +	 */
> > +	mdio_device_put(mdiodev);
> > +
> > +	return xpcs;
> > +}
> > +EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> > index 1d7d550bbf1a..537b62330c90 100644
> > --- a/include/linux/mdio.h
> > +++ b/include/linux/mdio.h
> > @@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv);
> >  void mdio_driver_unregister(struct mdio_driver *drv);
> >  int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
> > 
> > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > +{
> > +	get_device(&mdiodev->dev);
> > +}
> > +
> > +static inline void mdio_device_put(struct mdio_device *mdiodev)
> > +{
> > +	mdio_device_free(mdiodev);
> > +}
> > +
> >  static inline bool mdio_phy_id_is_c45(int phy_id)
> >  {
> >  	return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
> 
> Looks great, it can eliminate to create mdiodev in the ethernet driver, this device
> only be used in xpcs.

I'm just creating a patch series for both xpcs and lynx, which this
morning have had patches identifying similar problems with creation
and destruction.

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