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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Jiawen Wu <jiawenwu@trustnetic.com>
Cc: 'Jakub Kicinski' <kuba@kernel.org>,
	netdev@vger.kernel.org, jarkko.nikula@linux.intel.com,
	andriy.shevchenko@linux.intel.com,
	mika.westerberg@linux.intel.com, jsd@semihalf.com,
	Jose.Abreu@synopsys.com, andrew@lunn.ch, hkallweit1@gmail.com,
	linux-i2c@vger.kernel.org, linux-gpio@vger.kernel.org,
	mengyuanlou@net-swift.com
Subject: Re: [PATCH net-next v9 8/9] net: txgbe: Implement phylink pcs
Date: Fri, 26 May 2023 10:07:09 +0100	[thread overview]
Message-ID: <ZHB2vXBP1B2iHXBl@shell.armlinux.org.uk> (raw)
In-Reply-To: <026901d98fb0$b5001d80$1f005880$@trustnetic.com>

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);

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