[PATCH net-next 0/6] net: pcs: add helpers to xpcs and lynx to manage mdiodev

[PATCH net-next 0/6] net: pcs: add helpers to xpcs and lynx to manage mdiodev

[Russell King (Oracle)]

Hi,

This morning, we have had two instances where the destruction of the
MDIO device associated with XPCS and Lynx has been wrong. Rather than
allowing this pattern of errors to continue, let's make it easier for
driver authors to get this right by adding a helper.

The changes are essentially:

1. Add two new mdio device helpers to manage the underlying struct
   device reference count. Note that the existing mdio_device_free()
   doesn't actually free anything, it merely puts the reference count.

2. Make the existing _create() and _destroy() PCS driver methods
   increment and decrement this refcount using these helpers. This
   results in no overall change, although drivers may hang on to
   the mdio device for a few cycles longer.

3. Add _create_mdiodev() which creates the mdio device before calling
   the existing _create() method. Once the _create() method has
   returned, we put the reference count on the mdio device.

   If _create() was successful, then the reference count taken there
   will "hold" the mdio device for the lifetime of the PCS (in other
   words, until _destroy() is called.) However, if _create() failed,
   then dropping the refcount at this point will free the mdio device.

   This is the exact behaviour we desire.

4. Convert users that create a mdio device and then call the PCS's
   _create() method over to the new _create_mdiodev() method, and
   simplify the cleanup.

We also have DPAA2 and fmem_memac that look up their PCS rather than
creating it. These could also drop their reference count on the MDIO
device immediately after calling lynx_pcs_create(), which would then
mean we wouldn't need lynx_get_mdio_device() and the associated
complexity to put the device in dpaa2_pcs_destroy() and pcs_put().
Note that DPAA2 bypasses the mdio device's abstractions by calling
put_device() directly.

 drivers/net/dsa/ocelot/felix_vsc9959.c            | 20 +++------------
 drivers/net/dsa/ocelot/seville_vsc9953.c          | 20 +++------------
 drivers/net/ethernet/freescale/enetc/enetc_pf.c   | 22 +++-------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 15 +++--------
 drivers/net/pcs/pcs-lynx.c                        | 31 +++++++++++++++++++++++
 drivers/net/pcs/pcs-xpcs.c                        | 28 ++++++++++++++++++++
 include/linux/mdio.h                              | 10 ++++++++
 include/linux/pcs-lynx.h                          |  1 +
 include/linux/pcs/pcs-xpcs.h                      |  2 ++
 9 files changed, 87 insertions(+), 62 deletions(-)

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

[PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[Russell King (Oracle)]

Add two new operations for a mdio device to manage the refcount on the
underlying struct device. This will be used by mdio PCS drivers to
simplify the creation and destruction handling, making it easier for
users to get it correct.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 include/linux/mdio.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 0670cc6e067c..c1b7008826e5 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -106,6 +106,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);
-- 
2.30.2

[PATCH net-next 2/6] net: pcs: xpcs: add xpcs_create_mdiodev()

[Russell King (Oracle)]

Add xpcs_create_mdiodev() to simplify the creation of the mdio device
associated with the XPCS. In order to allow xpcs_destroy() to clean
this up, we need to arrange for xpcs_create() to take a refcount on
the mdiodev, and xpcs_destroy() to put it.

Adding the refcounting to xpcs_create()..xpcs_destroy() will be
transparent to existing users of these interfaces.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c   | 28 ++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 736776e40c25..1ba214429e01 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1235,6 +1235,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);
@@ -1267,6 +1268,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 	ret = -ENODEV;
 
 out:
+	mdio_device_put(mdiodev);
 	kfree(xpcs);
 
 	return ERR_PTR(ret);
@@ -1275,8 +1277,34 @@ EXPORT_SYMBOL_GPL(xpcs_create);
 
 void xpcs_destroy(struct dw_xpcs *xpcs)
 {
+	if (xpcs)
+		mdio_device_put(xpcs->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/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index d2da1e0b4a92..a99972a6d046 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -37,6 +37,8 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);
 struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 			    phy_interface_t interface);
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
+				    phy_interface_t interface);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.30.2

[PATCH net-next 3/6] net: stmmac: use xpcs_create_mdiodev()

[Russell King (Oracle)]

Use the new xpcs_create_mdiodev() creator, which simplifies the
creation and destruction of the mdio device associated with xpcs.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 6807c4c1a0a2..3db1cb0fd160 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -491,7 +491,6 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 int stmmac_xpcs_setup(struct mii_bus *bus)
 {
 	struct net_device *ndev = bus->priv;
-	struct mdio_device *mdiodev;
 	struct stmmac_priv *priv;
 	struct dw_xpcs *xpcs;
 	int mode, addr;
@@ -501,16 +500,10 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
 
 	/* Try to probe the XPCS by scanning all addresses. */
 	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
-		mdiodev = mdio_device_create(bus, addr);
-		if (IS_ERR(mdiodev))
+		xpcs = xpcs_create_mdiodev(bus, addr, mode);
+		if (IS_ERR(xpcs))
 			continue;
 
-		xpcs = xpcs_create(mdiodev, mode);
-		if (IS_ERR_OR_NULL(xpcs)) {
-			mdio_device_free(mdiodev);
-			continue;
-		}
-
 		priv->hw->xpcs = xpcs;
 		break;
 	}
@@ -669,10 +662,8 @@ int stmmac_mdio_unregister(struct net_device *ndev)
 	if (!priv->mii)
 		return 0;
 
-	if (priv->hw->xpcs) {
-		mdio_device_free(priv->hw->xpcs->mdiodev);
+	if (priv->hw->xpcs)
 		xpcs_destroy(priv->hw->xpcs);
-	}
 
 	mdiobus_unregister(priv->mii);
 	priv->mii->priv = NULL;
-- 
2.30.2

[PATCH net-next 4/6] net: pcs: lynx: add lynx_pcs_create_mdiodev()

[Russell King (Oracle)]

Add lynx_pcs_create_mdiodev() to simplify the creation of the mdio
device associated with lynx PCS. In order to allow lynx_pcs_destroy()
to clean this up, we need to arrange for lynx_pcs_create() to take a
refcount on the mdiodev, and lynx_pcs_destroy() to put it.

Adding the refcounting to lynx_pcs_create()..lynx_pcs_destroy() will
be transparent to existing users of these interfaces.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-lynx.c | 31 +++++++++++++++++++++++++++++++
 include/linux/pcs-lynx.h   |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 622c3de3f3a8..f04dc580ffb8 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -323,6 +323,7 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 	if (!lynx)
 		return NULL;
 
+	mdio_device_get(mdio);
 	lynx->mdio = mdio;
 	lynx->pcs.ops = &lynx_pcs_phylink_ops;
 	lynx->pcs.poll = true;
@@ -331,10 +332,40 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 }
 EXPORT_SYMBOL(lynx_pcs_create);
 
+struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr)
+{
+	struct mdio_device *mdio;
+	struct phylink_pcs *pcs;
+
+	mdio = mdio_device_create(bus, addr);
+	if (IS_ERR(mdio))
+		return ERR_CAST(mdio);
+
+	pcs = lynx_pcs_create(mdio);
+
+	/* Convert failure to create the PCS to an error pointer, so this
+	 * function has a consistent return value strategy.
+	 */
+	if (!pcs)
+		pcs = ERR_PTR(-ENOMEM);
+
+	/* lynx_create() has taken a refcount on the mdiodev if it was
+	 * successful. If lynx_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
+	 * lynx_destroy() to automatically free the mdio device.
+	 */
+	mdio_device_put(mdio);
+
+	return pcs;
+}
+EXPORT_SYMBOL(lynx_pcs_create_mdiodev);
+
 void lynx_pcs_destroy(struct phylink_pcs *pcs)
 {
 	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
 
+	mdio_device_put(lynx->mdio);
 	kfree(lynx);
 }
 EXPORT_SYMBOL(lynx_pcs_destroy);
diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
index 5712cc2ce775..885b59d10581 100644
--- a/include/linux/pcs-lynx.h
+++ b/include/linux/pcs-lynx.h
@@ -12,6 +12,7 @@
 struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
 
 struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
+struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr);
 
 void lynx_pcs_destroy(struct phylink_pcs *pcs);
 
-- 
2.30.2

[PATCH net-next 5/6] net: dsa: ocelot: use lynx_pcs_create_mdiodev()

[Russell King (Oracle)]

Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
creation and destruction of the lynx PCS.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 20 ++++----------------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 20 ++++----------------
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index cfb3faeaa5bf..030738fef60e 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1021,7 +1021,6 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
 		struct phylink_pcs *phylink_pcs;
-		struct mdio_device *mdio_device;
 
 		if (dsa_is_unused_port(felix->ds, port))
 			continue;
@@ -1029,16 +1028,10 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		mdio_device = mdio_device_create(felix->imdio, port);
-		if (IS_ERR(mdio_device))
+		phylink_pcs = lynx_pcs_create_mdiodev(felix->imdio, port);
+		if (IS_ERR(phylink_pcs))
 			continue;
 
-		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (!phylink_pcs) {
-			mdio_device_free(mdio_device);
-			continue;
-		}
-
 		felix->pcs[port] = phylink_pcs;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
@@ -1054,14 +1047,9 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
 		struct phylink_pcs *phylink_pcs = felix->pcs[port];
-		struct mdio_device *mdio_device;
-
-		if (!phylink_pcs)
-			continue;
 
-		mdio_device = lynx_get_mdio_device(phylink_pcs);
-		mdio_device_free(mdio_device);
-		lynx_pcs_destroy(phylink_pcs);
+		if (phylink_pcs)
+			lynx_pcs_destroy(phylink_pcs);
 	}
 	mdiobus_unregister(felix->imdio);
 	mdiobus_free(felix->imdio);
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 96d4972a62f0..15003b2af264 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -912,7 +912,6 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
 		struct phylink_pcs *phylink_pcs;
-		struct mdio_device *mdio_device;
 		int addr = port + 4;
 
 		if (dsa_is_unused_port(felix->ds, port))
@@ -921,16 +920,10 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		mdio_device = mdio_device_create(felix->imdio, addr);
-		if (IS_ERR(mdio_device))
+		phylink_pcs = lynx_pcs_create_mdiodev(felix->imdio, addr);
+		if (IS_ERR(phylink_pcs))
 			continue;
 
-		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (!phylink_pcs) {
-			mdio_device_free(mdio_device);
-			continue;
-		}
-
 		felix->pcs[port] = phylink_pcs;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
@@ -946,14 +939,9 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
 		struct phylink_pcs *phylink_pcs = felix->pcs[port];
-		struct mdio_device *mdio_device;
-
-		if (!phylink_pcs)
-			continue;
 
-		mdio_device = lynx_get_mdio_device(phylink_pcs);
-		mdio_device_free(mdio_device);
-		lynx_pcs_destroy(phylink_pcs);
+		if (phylink_pcs)
+			lynx_pcs_destroy(phylink_pcs);
 	}
 
 	/* mdiobus_unregister and mdiobus_free handled by devres */
-- 
2.30.2

[PATCH net-next 6/6] net: enetc: use lynx_pcs_create_mdiodev()

[Russell King (Oracle)]

Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
creation and destruction of the lynx PCS.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 22 ++++---------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 7cd22d370caa..1416262d4296 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -863,7 +863,6 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 	struct device *dev = &pf->si->pdev->dev;
 	struct enetc_mdio_priv *mdio_priv;
 	struct phylink_pcs *phylink_pcs;
-	struct mdio_device *mdio_device;
 	struct mii_bus *bus;
 	int err;
 
@@ -889,17 +888,9 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 		goto free_mdio_bus;
 	}
 
-	mdio_device = mdio_device_create(bus, 0);
-	if (IS_ERR(mdio_device)) {
-		err = PTR_ERR(mdio_device);
-		dev_err(dev, "cannot create mdio device (%d)\n", err);
-		goto unregister_mdiobus;
-	}
-
-	phylink_pcs = lynx_pcs_create(mdio_device);
-	if (!phylink_pcs) {
-		mdio_device_free(mdio_device);
-		err = -ENOMEM;
+	phylink_pcs = lynx_pcs_create_mdiodev(bus, 0);
+	if (IS_ERR(phylink_pcs)) {
+		err = PTR_ERR(phylink_pcs);
 		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
 		goto unregister_mdiobus;
 	}
@@ -918,13 +909,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 
 static void enetc_imdio_remove(struct enetc_pf *pf)
 {
-	struct mdio_device *mdio_device;
-
-	if (pf->pcs) {
-		mdio_device = lynx_get_mdio_device(pf->pcs);
-		mdio_device_free(mdio_device);
+	if (pf->pcs)
 		lynx_pcs_destroy(pf->pcs);
-	}
 	if (pf->imdio) {
 		mdiobus_unregister(pf->imdio);
 		mdiobus_free(pf->imdio);
-- 
2.30.2

Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[andy.shevchenko]

Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:
> Add two new operations for a mdio device to manage the refcount on the
> underlying struct device. This will be used by mdio PCS drivers to
> simplify the creation and destruction handling, making it easier for
> users to get it correct.

...

> +static inline void mdio_device_get(struct mdio_device *mdiodev)
> +{
> +	get_device(&mdiodev->dev);

Dunno what is the practice in net, but it makes sense to have the pattern as

	if (mdiodev)
		return to_mdiodev(get_device(&mdiodev->dev));

which might be helpful in some cases. This approach is used in many cases in
the kernel.

> +}

P.S. Just my 2c, you may ignore the above comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next 4/6] net: pcs: lynx: add lynx_pcs_create_mdiodev()

[andy.shevchenko]

Fri, May 26, 2023 at 11:14:39AM +0100, Russell King (Oracle) kirjoitti:
> Add lynx_pcs_create_mdiodev() to simplify the creation of the mdio
> device associated with lynx PCS. In order to allow lynx_pcs_destroy()
> to clean this up, we need to arrange for lynx_pcs_create() to take a
> refcount on the mdiodev, and lynx_pcs_destroy() to put it.
> 
> Adding the refcounting to lynx_pcs_create()..lynx_pcs_destroy() will
> be transparent to existing users of these interfaces.

...

> +	mdio_device_get(mdio);
>  	lynx->mdio = mdio;

Just for the sake of example of how it can be used (taking into account my
previous comment):

  	lynx->mdio = mdio_device_get(mdio);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[Andrew Lunn]

On Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) wrote:
> Add two new operations for a mdio device to manage the refcount on the
> underlying struct device. This will be used by mdio PCS drivers to
> simplify the creation and destruction handling, making it easier for
> users to get it correct.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[Andrew Lunn]

On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote:
> Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:
> > Add two new operations for a mdio device to manage the refcount on the
> > underlying struct device. This will be used by mdio PCS drivers to
> > simplify the creation and destruction handling, making it easier for
> > users to get it correct.
> 
> ...
> 
> > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > +{
> > +	get_device(&mdiodev->dev);
> 
> Dunno what is the practice in net, but it makes sense to have the pattern as
> 
> 	if (mdiodev)
> 		return to_mdiodev(get_device(&mdiodev->dev));
> 
> which might be helpful in some cases. This approach is used in many cases in
> the kernel.

The device should exist. If it does not, it is a bug, and the
resulting opps will help find the bug.

	  Andrew

Re: [PATCH net-next 2/6] net: pcs: xpcs: add xpcs_create_mdiodev()

[Andrew Lunn]

>  void xpcs_destroy(struct dw_xpcs *xpcs)
>  {
> +	if (xpcs)
> +		mdio_device_put(xpcs->mdiodev);
>  	kfree(xpcs);
>  }

Nit:

Is the if () needed? Can destroy be called if create was not
successful?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/6] net: stmmac: use xpcs_create_mdiodev()

[Andrew Lunn]

On Fri, May 26, 2023 at 11:14:34AM +0100, Russell King (Oracle) wrote:
> Use the new xpcs_create_mdiodev() creator, which simplifies the
> creation and destruction of the mdio device associated with xpcs.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 4/6] net: pcs: lynx: add lynx_pcs_create_mdiodev()

[Andrew Lunn]

On Fri, May 26, 2023 at 11:14:39AM +0100, Russell King (Oracle) wrote:
> Add lynx_pcs_create_mdiodev() to simplify the creation of the mdio
> device associated with lynx PCS. In order to allow lynx_pcs_destroy()
> to clean this up, we need to arrange for lynx_pcs_create() to take a
> refcount on the mdiodev, and lynx_pcs_destroy() to put it.
> 
> Adding the refcounting to lynx_pcs_create()..lynx_pcs_destroy() will
> be transparent to existing users of these interfaces.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 5/6] net: dsa: ocelot: use lynx_pcs_create_mdiodev()

[Andrew Lunn]

On Fri, May 26, 2023 at 11:14:44AM +0100, Russell King (Oracle) wrote:
> Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
> creation and destruction of the lynx PCS.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 6/6] net: enetc: use lynx_pcs_create_mdiodev()

[Andrew Lunn]

On Fri, May 26, 2023 at 11:14:50AM +0100, Russell King (Oracle) wrote:
> Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
> creation and destruction of the lynx PCS.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 6/6] net: enetc: use lynx_pcs_create_mdiodev()

[Vladimir Oltean]

On Fri, May 26, 2023 at 11:14:50AM +0100, Russell King (Oracle) wrote:
> Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
> creation and destruction of the lynx PCS.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 5/6] net: dsa: ocelot: use lynx_pcs_create_mdiodev()

[Vladimir Oltean]

On Fri, May 26, 2023 at 11:14:44AM +0100, Russell King (Oracle) wrote:
> Use the newly introduced lynx_pcs_create_mdiodev() which simplifies the
> creation and destruction of the lynx PCS.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 2/6] net: pcs: xpcs: add xpcs_create_mdiodev()

[Russell King (Oracle)]

On Mon, May 29, 2023 at 05:25:45PM +0200, Andrew Lunn wrote:
> >  void xpcs_destroy(struct dw_xpcs *xpcs)
> >  {
> > +	if (xpcs)
> > +		mdio_device_put(xpcs->mdiodev);
> >  	kfree(xpcs);
> >  }
> 
> Nit:
> 
> Is the if () needed? Can destroy be called if create was not
> successful?

kfree(NULL) is a no-op, so xpcs_destroy() will not oops if passed a NULL
pointer. So it makes sense to preserve this.

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

Re: [PATCH net-next 2/6] net: pcs: xpcs: add xpcs_create_mdiodev()

[Vladimir Oltean]

On Mon, May 29, 2023 at 05:25:45PM +0200, Andrew Lunn wrote:
> >  void xpcs_destroy(struct dw_xpcs *xpcs)
> >  {
> > +	if (xpcs)
> > +		mdio_device_put(xpcs->mdiodev);
> >  	kfree(xpcs);
> >  }
> 
> Nit:
> 
> Is the if () needed? Can destroy be called if create was not
> successful?

No, xpcs_destroy() shouldn't be (and isn't) called if xpcs_create()
or xpcs_create_mdiodev() wasn't successful. If it was, it would be
an indication of sloppy coding style, which can easily be avoided
for minor things like this.

Re: [PATCH net-next 4/6] net: pcs: lynx: add lynx_pcs_create_mdiodev()

[Ioana Ciornei]

On Fri, May 26, 2023 at 11:14:39AM +0100, Russell King (Oracle) wrote:
> Add lynx_pcs_create_mdiodev() to simplify the creation of the mdio
> device associated with lynx PCS. In order to allow lynx_pcs_destroy()
> to clean this up, we need to arrange for lynx_pcs_create() to take a
> refcount on the mdiodev, and lynx_pcs_destroy() to put it.
> 
> Adding the refcounting to lynx_pcs_create()..lynx_pcs_destroy() will
> be transparent to existing users of these interfaces.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Tested-by: Ioana Ciornei <ioana.ciornei@nxp.com>

Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[Andy Shevchenko]

On Mon, May 29, 2023 at 6:21 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote:
> > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:

...

> > > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > > +{
> > > +   get_device(&mdiodev->dev);
> >
> > Dunno what is the practice in net, but it makes sense to have the pattern as
> >
> >       if (mdiodev)
> >               return to_mdiodev(get_device(&mdiodev->dev));
> >
> > which might be helpful in some cases. This approach is used in many cases in
> > the kernel.
>
> The device should exist. If it does not, it is a bug, and the
> resulting opps will help find the bug.

The main point in the returned value, the 'if' can be dropped, it's
not a big deal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

[Russell King (Oracle)]

On Mon, May 29, 2023 at 11:34:42PM +0300, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 6:21 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote:
> > > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:
> 
> ...
> 
> > > > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > > > +{
> > > > +   get_device(&mdiodev->dev);
> > >
> > > Dunno what is the practice in net, but it makes sense to have the pattern as
> > >
> > >       if (mdiodev)
> > >               return to_mdiodev(get_device(&mdiodev->dev));
> > >
> > > which might be helpful in some cases. This approach is used in many cases in
> > > the kernel.
> >
> > The device should exist. If it does not, it is a bug, and the
> > resulting opps will help find the bug.
> 
> The main point in the returned value, the 'if' can be dropped, it's
> not a big deal.

And if you do, that results in a latent bug.

The whole point of doing the above is to cater for the case when
mdiodev is NULL. If mdiodev is NULL, provided "dev" is the first member
of mdiodev, then &mdiodev->dev will also be NULL. That will mean
get_device() will see a NULL pointer and itself return NULL. The
to_mdiodev() will then convert back to a mdiodev which will also be
NULL. So everything works correctly.

If dev is not the first member, then &mdiodev->dev will no longer be
NULL, but will be offset by that amount.

get_device() will check whether that is NULL - it won't be, so it'll
try to pass &dev->kobj to kobject_get(). That will also not be a NULL
pointer. kobject_get() will likely oops the kernel.

So no, it isn't safe to drop that if().

However, count the number of places in the kernel that actually pay
attention to the return value of get_device()...

In drivers/base, which should be the prime area where things are
done correctly, there are 42 places where get_device() is called.
Of those 42 places, 13 places make use of the returned value in
some way, which mean 29 do not bother to check.

Now, what use is checking that return value? Does get_device()
ever return anything different from what was passed in?

Well, we need to delve further down into kobject_get(), which
does not - kobject_get() returns whatever it was given. Note
that kref_get() doesn't return anything, nor does refcount_inc(),
both of which post-date get_device().

So, that in turn means that get_device() will only ever return
what it was passed. So:

	ret = get_device(dev);

`ret' is _guaranteed_ to always be exactly the same was `dev' in
all cases (except if "dev" results in get_device() performing an
invalid dereference and Oopsing the kernel, by which time we won't
care about `ret'.)

Now, if we think about situations where this will be called - they
will always be called where the MDIO device already has reference
counts against it - we have to be holding at least one refcount
to already have a reference to this device. If we don't have that,
then we're in the situation where the memory pointed to by the
mdio device pointer has probably already been freed, and could
already be used for some other purpose - and using the return value
from get_device() isn't going to save you from such a racy stupidity.

If we extend this to mdio_device_get(), then we end up in exactly
the same scenario - and what benefit does it give to the code?
Does it make the code more readable? No, it makes it less readable.
Does it make the code more robust? No, it makes no difference.
Does it make the code more correct? Again, no difference.

So, I don't see any point in changing the proposal I've put forward
as mdio_device_get().

Things would be different if get_device() used kobject_get_unless_zero()
which _can_ alter the returned value, but as I've stated above, if the
refcount were to become zero at the point that mdio_device_get() may
be called, we've *already* lost.

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

Re: [PATCH net-next 0/6] net: pcs: add helpers to xpcs and lynx to manage mdiodev

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 26 May 2023 11:13:59 +0100 you wrote:
> Hi,
> 
> This morning, we have had two instances where the destruction of the
> MDIO device associated with XPCS and Lynx has been wrong. Rather than
> allowing this pattern of errors to continue, let's make it easier for
> driver authors to get this right by adding a helper.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] net: mdio: add mdio_device_get() and mdio_device_put()
    https://git.kernel.org/netdev/net-next/c/c4933fa88a68
  - [net-next,2/6] net: pcs: xpcs: add xpcs_create_mdiodev()
    https://git.kernel.org/netdev/net-next/c/9a5d500cffdb
  - [net-next,3/6] net: stmmac: use xpcs_create_mdiodev()
    https://git.kernel.org/netdev/net-next/c/727e373f897d
  - [net-next,4/6] net: pcs: lynx: add lynx_pcs_create_mdiodev()
    https://git.kernel.org/netdev/net-next/c/86b5f2d8cd78
  - [net-next,5/6] net: dsa: ocelot: use lynx_pcs_create_mdiodev()
    https://git.kernel.org/netdev/net-next/c/5767c6a8d9b7
  - [net-next,6/6] net: enetc: use lynx_pcs_create_mdiodev()
    https://git.kernel.org/netdev/net-next/c/b7d5d0438e01

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html