[PATCH net-next] net: pcs: pcs-lynx: remove lynx_get_mdio_device() and refactor cleanup

As of today, the lynx_get_mdio_device() function is only used during the
cleanup phase, to free the underlying mdio_device. This can be
factored inside lynx_pcs_destroy().

Part of the effort driving this is the merge of pcs-altera-tse into
pcs-lynx, as both have very similar register layouts. One of the main
difference is that the TSE pcs is memory-mapped, and the merge into
pcs-lynx would first require a conversion of pcs-lynx to regmap.

Removing lynx_get_mdio_device() makes pcs-lynx somewhat less
mdio-specific.

The following drivers have been trivialy modified to account for the
modification :
 - felix_vsc9959.c
 - seville_vsc9953.c
 - enetc_pf.c
 - fman_memac.c

For dpaa2-mac.c, the cleanup sequence used put_device(&mdio->dev), which
is exactly what mdio_device_free(mdio) does.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
Although this patch covers multiple drivers, it was kept as a single
patch to keep things bisectable. It was also only compile-tested, any
review and test is very welcome.

 drivers/net/dsa/ocelot/felix_vsc9959.c           |  3 ---
 drivers/net/dsa/ocelot/seville_vsc9953.c         |  3 ---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c |  4 ----
 drivers/net/ethernet/freescale/enetc/enetc_pf.c  |  8 ++------
 drivers/net/ethernet/freescale/fman/fman_memac.c |  4 ----
 drivers/net/pcs/pcs-lynx.c                       | 10 ++--------
 include/linux/pcs-lynx.h                         |  2 --
 7 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 43dc8ed4854d..1696d1eaa570 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c

@@ -1054,13 +1054,10 @@ 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);
 	}
 	mdiobus_unregister(felix->imdio);

diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 88ed3a2e487a..0f7b947cb43b 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c

@@ -946,13 +946,10 @@ 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);
 	}
 

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index c886f33f8c6f..c4733c46c896 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

@@ -284,11 +284,7 @@ static void dpaa2_pcs_destroy(struct dpaa2_mac *mac)
 	struct phylink_pcs *phylink_pcs = mac->pcs;
 
 	if (phylink_pcs) {
-		struct mdio_device *mdio = lynx_get_mdio_device(phylink_pcs);
-		struct device *dev = &mdio->dev;
-
 		lynx_pcs_destroy(phylink_pcs);
-		put_device(dev);
 		mac->pcs = NULL;
 	}
 }

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 7facc7d5261e..39940bd25b8d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c

@@ -915,13 +915,9 @@ 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);

diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
index 9349f841bd06..555729c7c67e 100644
--- a/drivers/net/ethernet/freescale/fman/fman_memac.c
+++ b/drivers/net/ethernet/freescale/fman/fman_memac.c

@@ -976,14 +976,10 @@ static int memac_init(struct fman_mac *memac)
 
 static void pcs_put(struct phylink_pcs *pcs)
 {
-	struct mdio_device *mdiodev;
-
 	if (IS_ERR_OR_NULL(pcs))
 		return;
 
-	mdiodev = lynx_get_mdio_device(pcs);
 	lynx_pcs_destroy(pcs);
-	mdio_device_free(mdiodev);
 }
 
 static int memac_free(struct fman_mac *memac)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 3903f3baba2b..4457298f7fb8 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c

@@ -34,14 +34,6 @@ enum sgmii_speed {
 #define phylink_pcs_to_lynx(pl_pcs) container_of((pl_pcs), struct lynx_pcs, pcs)
 #define lynx_to_phylink_pcs(lynx) (&(lynx)->pcs)
 
-struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs)
-{
-	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
-
-	return lynx->mdio;
-}
-EXPORT_SYMBOL(lynx_get_mdio_device);
-
 static void lynx_pcs_get_state_usxgmii(struct mdio_device *pcs,
 				       struct phylink_link_state *state)
 {

@@ -335,6 +327,8 @@ void lynx_pcs_destroy(struct phylink_pcs *pcs)
 {
 	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
 
+	mdio_device_free(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..d8327323ddf7 100644
--- a/include/linux/pcs-lynx.h
+++ b/include/linux/pcs-lynx.h

@@ -9,8 +9,6 @@
 #include <linux/mdio.h>
 #include <linux/phylink.h>
 
-struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
-
 struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
 
 void lynx_pcs_destroy(struct phylink_pcs *pcs);

-- 
2.39.1

On Fri, Jan 27, 2023 at 02:40:30PM +0100, Maxime Chevallier wrote:

One of the main difference is that the TSE pcs is memory-mapped, and
the merge into pcs-lynx would first require a conversion of pcs-lynx
to regmap.

I suppose sooner or later you'll want to convert stuff like
phylink_mii_c22_pcs_get_state() to regmap too?

Can't you create an MDIO bus for the TSE PCS which translates MDIO
reads/writes to MMIO accesses?

Hi Vlad,

On Fri, 27 Jan 2023 15:43:51 +0200
Vladimir Oltean [off-list ref] wrote:

On Fri, Jan 27, 2023 at 02:40:30PM +0100, Maxime Chevallier wrote:

quoted

One of the main difference is that the TSE pcs is memory-mapped, and
the merge into pcs-lynx would first require a conversion of pcs-lynx
to regmap.  

I suppose sooner or later you'll want to convert stuff like
phylink_mii_c22_pcs_get_state() to regmap too?

Well that was my next part to tackle indeed...

Can't you create an MDIO bus for the TSE PCS which translates MDIO
reads/writes to MMIO accesses?

TBH I haven't considered that, I guess this would definitely make thing
much easier. Since the register layout of the TSE PCS is very very
similar to the C22 layout, that could be indeed justified, as it's
basically a set of standard mdio registers exposed through mmio.

Thanks for the tip.

However this current patch still makes sense though right ?

Maxime

On Fri, Jan 27, 2023 at 03:07:58PM +0100, Maxime Chevallier wrote:

However this current patch still makes sense though right ?

I have a pretty hard time saying yes; TL;DR yes it's less code, but it's
structured that way with a reason.

I don't think it's lynx_pcs_destroy()'s responsibility to call mdio_device_free(),
just like it isn't lynx_pcs_create()'s responsibility to call mdio_device_create()
(or whatever). In fact that's the reason why the mdiodev isn't completely
absorbed by the lynx_pcs - because there isn't a unified way to get a reference
to it - some platforms have a hardcoded address, others have a phandle in the
device tree.

I know this is entirely subjective, but to me, having functions organized
in pairs which undo precisely what the other has done, and not more, really
helps with spotting resource leakage issues. I realize that it's not the same
for everybody. For example, while reviewing your patch, I noticed this
in the existing code:

static struct phylink_pcs *memac_pcs_create(struct device_node *mac_node,
					    int index)
{
	struct device_node *node;
	struct mdio_device *mdiodev = NULL;
	struct phylink_pcs *pcs;

	node = of_parse_phandle(mac_node, "pcsphy-handle", index);
	if (node && of_device_is_available(node))
		mdiodev = of_mdio_find_device(node);
	of_node_put(node);

	if (!mdiodev)
		return ERR_PTR(-EPROBE_DEFER);

	pcs = lynx_pcs_create(mdiodev); // if this fails, we miss calling mdio_device_free()
	return pcs;
}

and it's clear that what is obvious to me was not obvious to the author
of commit a7c2a32e7f22 ("net: fman: memac: Use lynx pcs driver"), since
this organization scheme didn't work for him.

Hello Vlad,

On Sat, 28 Jan 2023 03:58:41 +0200
Vladimir Oltean [off-list ref] wrote:

On Fri, Jan 27, 2023 at 03:07:58PM +0100, Maxime Chevallier wrote:

quoted

However this current patch still makes sense though right ?  

I have a pretty hard time saying yes; TL;DR yes it's less code, but
it's structured that way with a reason.

I don't think it's lynx_pcs_destroy()'s responsibility to call
mdio_device_free(), just like it isn't lynx_pcs_create()'s
responsibility to call mdio_device_create() (or whatever). In fact
that's the reason why the mdiodev isn't completely absorbed by the
lynx_pcs - because there isn't a unified way to get a reference to it
- some platforms have a hardcoded address, others have a phandle in
the device tree.

I get you and I actually agree, I've been also thinking about that this
weekend and indeed it would create an asymetry that can easily lead to
leaky code.

Let's drop that patch then, thanks a lot for the thourough review and
comments, I appreciate it.

Best regards,

Maxime

I know this is entirely subjective, but to me, having functions
organized in pairs which undo precisely what the other has done, and
not more, really helps with spotting resource leakage issues. I
realize that it's not the same for everybody. For example, while
reviewing your patch, I noticed this in the existing code:

static struct phylink_pcs *memac_pcs_create(struct device_node
*mac_node, int index)
{
	struct device_node *node;
	struct mdio_device *mdiodev = NULL;
	struct phylink_pcs *pcs;

	node = of_parse_phandle(mac_node, "pcsphy-handle", index);
	if (node && of_device_is_available(node))
		mdiodev = of_mdio_find_device(node);
	of_node_put(node);

	if (!mdiodev)
		return ERR_PTR(-EPROBE_DEFER);

	pcs = lynx_pcs_create(mdiodev); // if this fails, we miss
calling mdio_device_free() return pcs;
}

and it's clear that what is obvious to me was not obvious to the
author of commit a7c2a32e7f22 ("net: fman: memac: Use lynx pcs
driver"), since this organization scheme didn't work for him.