From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 94C5EC7EE2D for ; Fri, 26 May 2023 09:07:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242674AbjEZJHS (ORCPT ); Fri, 26 May 2023 05:07:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54636 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236818AbjEZJHQ (ORCPT ); Fri, 26 May 2023 05:07:16 -0400 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:32c8:5054:ff:fe00:142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E312612A; Fri, 26 May 2023 02:07:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=ywAOw4y6wdHt7k3wEYHNdxs4ti0WSGVJ93d4/r7t8Wc=; b=jGMryH9LHrq1dEYoYBDB2wTMR2 iTqIgfuXLVStjfy7PQo3jVGi98Cj9TVs54DoHH++QkG6Zu6pwhW+Lg05hCHyfQM9RpsPAhFkhLnyT 2i30oqNgIbSqStsFODHAMcFSzk4JcibFgh5xXImrKAjOuU+mJlXZd/8CbLEmh+IKv9eZA+pxqVY45 pZ+j+Qud789Q5lUrxBGoavxtipzl5mMTYvQHRgH/Z5OJdG56yd8UI6WiJublGGCgWaECY0QgkANTC AYQgmaA3jt5CO2fPS1Ut6cW7sv/l3Bp6Y9qvDqovBBadSOsd1ZRPpu7u4+tvvg9UJxVdeXv/VCeuN LXtZwgRg==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:43364) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1q2TPj-0005Gh-Qd; Fri, 26 May 2023 10:07:11 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1q2TPh-0003au-NP; Fri, 26 May 2023 10:07:09 +0100 Date: Fri, 26 May 2023 10:07:09 +0100 From: "Russell King (Oracle)" To: Jiawen Wu Cc: 'Jakub Kicinski' , 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 Message-ID: References: <20230524091722.522118-1-jiawenwu@trustnetic.com> <20230524091722.522118-9-jiawenwu@trustnetic.com> <20230525211403.44b5f766@kernel.org> <022201d98f9a$4b4ccc00$e1e66400$@trustnetic.com> <026901d98fb0$b5001d80$1f005880$@trustnetic.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <026901d98fb0$b5001d80$1f005880$@trustnetic.com> Sender: Russell King (Oracle) Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org 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!