| Message ID | 20230127134031.156143-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: pcs: pcs-lynx: remove lynx_get_mdio_device() and refactor cleanup | expand |
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 <vladimir.oltean@nxp.com> wrote: > 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? 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 <vladimir.oltean@nxp.com> wrote: > 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 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.
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);
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(-)