Skip to contentSkip to conversation

net: pcs: add helpers to xpcs and lynx to manage mdiodev

PATCH Applied Russell King (Oracle) 8 participants 24 messages May 26, 2023 – May 30, 2023

Subsystems (10)
Networking Network drivers Arm STM32 Ethernet PHY +6 more subsystems
lore
Share & tools
lore

Russell King (Oracle)

author reviewer
#0 lore raw

cover net: pcs: add helpers to xpcs and lynx to manage mdiodev

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(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/6 net: mdio: add mdio_device_get() and mdio_device_put()

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) <[email protected]>
---
 include/linux/mdio.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
include/linux/mdio.h +10 -0
index 0670cc6e067c..c1b7008826e5 100644
@@ -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);
Signature · 1 line
2.30.2
#2 lore raw

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

Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:

Quoted 4 lines

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.

...

include/linux/mdio.h int mdio_driver_register(struct mdio_driver *drv); · line 111

Quoted 3 lines
include/linux/mdio.h +3 -0
+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.

Quoted 1 line

+}

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

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#3 lore raw

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

Quoted 14 lines · On Fri, May 26, 2023 at 09:20:17PM +0300, [email protected] wrote:

Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:

Quoted 4 lines

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.

...

include/linux/mdio.h int mdio_driver_register(struct mdio_driver *drv); · line 111

Quoted 3 lines
include/linux/mdio.h +3 -0
+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

Quoted 5 lines
	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

#4 lore raw

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

Quoted 2 lines · Andrew Lunn
Quoted 1 line · On Fri, May 26, 2023 at 09:20:17PM +0300, [email protected] wrote:

Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:

...

Quoted 6 lines
Quoted 6 lines

include/linux/mdio.h int mdio_driver_register(struct mdio_driver *drv); · line 111

Quoted 3 lines
include/linux/mdio.h +3 -0
+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

Quoted 8 lines
Quoted 5 lines
      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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author reviewer
#5 lore raw

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

Quoted 15 lines · Andy Shevchenko
Quoted 2 lines · Andrew Lunn
Quoted 1 line · On Fri, May 26, 2023 at 09:20:17PM +0300, [email protected] wrote:

Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:

...

Quoted 9 lines
Quoted 9 lines

include/linux/mdio.h int mdio_driver_register(struct mdio_driver *drv); · line 111

Quoted 3 lines
include/linux/mdio.h +3 -0
+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));
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer Reviewed-by on v1 1/6
#6 lore raw

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

In reply to Russell King (Oracle)
Quoted 6 lines · 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) <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

#7 lore raw

PATCH 2/6 net: pcs: xpcs: add xpcs_create_mdiodev()

In reply to 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) <[email protected]>
---
 drivers/net/pcs/pcs-xpcs.c   | 28 ++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs.h |  2 ++
 2 files changed, 30 insertions(+)
drivers/net/pcs/pcs-xpcs.c +28 -0
index 736776e40c25..1ba214429e01 100644
@@ -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");
include/linux/pcs/pcs-xpcs.h +2 -0
index d2da1e0b4a92..a99972a6d046 100644
@@ -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 */
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 2/6
#8 lore raw

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

drivers/net/pcs/pcs-xpcs.c EXPORT_SYMBOL_GPL(xpcs_create); · line 1283

Quoted 6 lines

void xpcs_destroy(struct dw_xpcs *xpcs)
{

drivers/net/pcs/pcs-xpcs.c +2 -0
+	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 <[email protected]>

Andrew

author reviewer
#9 lore raw

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

Quoted 11 lines · Andrew Lunn

drivers/net/pcs/pcs-xpcs.c EXPORT_SYMBOL_GPL(xpcs_create); · line 1283

Quoted 6 lines

void xpcs_destroy(struct dw_xpcs *xpcs)
{

drivers/net/pcs/pcs-xpcs.c +2 -0
+	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.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer
#10 lore raw

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

In reply to Andrew Lunn
Quoted 11 lines · Andrew Lunn

drivers/net/pcs/pcs-xpcs.c EXPORT_SYMBOL_GPL(xpcs_create); · line 1283

Quoted 6 lines

void xpcs_destroy(struct dw_xpcs *xpcs)
{

drivers/net/pcs/pcs-xpcs.c +2 -0
+	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.

#11 lore raw

PATCH 3/6 net: stmmac: use xpcs_create_mdiodev()

In reply to 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) <[email protected]>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +3 -12
index 6807c4c1a0a2..3db1cb0fd160 100644
@@ -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;
Signature · 1 line
2.30.2
#13 lore raw

PATCH 4/6 net: pcs: lynx: add lynx_pcs_create_mdiodev()

In reply to 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) <[email protected]>
---
 drivers/net/pcs/pcs-lynx.c | 31 +++++++++++++++++++++++++++++++
 include/linux/pcs-lynx.h   |  1 +
 2 files changed, 32 insertions(+)
drivers/net/pcs/pcs-lynx.c +31 -0
index 622c3de3f3a8..f04dc580ffb8 100644
@@ -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);
include/linux/pcs-lynx.h +1 -0
index 5712cc2ce775..885b59d10581 100644
@@ -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);
Signature · 1 line
2.30.2
#14 lore raw

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

Fri, May 26, 2023 at 11:14:39AM +0100, Russell King (Oracle) kirjoitti:

Quoted 7 lines

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.

...

drivers/net/pcs/pcs-lynx.c struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) · line 327

Quoted 2 lines
drivers/net/pcs/pcs-lynx.c +1 -0
+	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);
Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer Reviewed-by on v1 4/6
#15 lore raw

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

In reply to Russell King (Oracle)
Quoted 9 lines · 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) <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

maintainer Reviewed-by Tested-by on v1 4/6
#16 lore raw

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

In reply to Russell King (Oracle)
Quoted 9 lines · 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) <[email protected]>
Reviewed-by Ioana Ciornei <[email protected]>
Tested-by Ioana Ciornei <[email protected]>
#17 lore raw

PATCH 5/6 net: dsa: ocelot: use lynx_pcs_create_mdiodev()

In reply to 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) <[email protected]>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 20 ++++----------------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 20 ++++----------------
 2 files changed, 8 insertions(+), 32 deletions(-)
drivers/net/dsa/ocelot/felix_vsc9959.c +4 -16
index cfb3faeaa5bf..030738fef60e 100644
@@ -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);
drivers/net/dsa/ocelot/seville_vsc9953.c +4 -16
index 96d4972a62f0..15003b2af264 100644
@@ -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 */
Signature · 1 line
2.30.2
maintainer Reviewed-by Tested-by on v1 5/6
#19 lore raw

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

In reply to Russell King (Oracle)
Quoted 5 lines · 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) <[email protected]>

---

Reviewed-by Vladimir Oltean <[email protected]>
Tested-by Vladimir Oltean <[email protected]>
#20 lore raw

PATCH 6/6 net: enetc: use lynx_pcs_create_mdiodev()

In reply to 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) <[email protected]>
---
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 22 ++++---------------
 1 file changed, 4 insertions(+), 18 deletions(-)
drivers/net/ethernet/freescale/enetc/enetc_pf.c +4 -18
index 7cd22d370caa..1416262d4296 100644
@@ -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);
Signature · 1 line
2.30.2
maintainer Reviewed-by Tested-by on v1 6/6
#22 lore raw

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

In reply to Russell King (Oracle)
Quoted 5 lines · 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) <[email protected]>

---

Reviewed-by Vladimir Oltean <[email protected]>
Tested-by Vladimir Oltean <[email protected]>
CI Applied
#23 lore raw
In reply to Russell King (Oracle)
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 8 lines · 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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html