Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS
From: Vladimir Oltean
Date: Tue Jul 19 2022 - 13:26:39 EST
On Mon, Jul 11, 2022 at 12:05:15PM -0400, Sean Anderson wrote:
> There is a common flow in several drivers where a lynx PCS is created
> without a corresponding firmware node. Consolidate these into one helper
> function. Because we control when the mdiodev is registered, we can add
> a custom match function which will automatically bind our driver
> (instead of using device_driver_attach).
>
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
> ---
>
> drivers/net/dsa/ocelot/felix_vsc9959.c | 25 ++++---------------
> drivers/net/dsa/ocelot/seville_vsc9953.c | 25 ++++---------------
> .../net/ethernet/freescale/enetc/enetc_pf.c | 21 +++-------------
> drivers/net/pcs/pcs-lynx.c | 24 ++++++++++++++++++
> include/linux/pcs-lynx.h | 1 +
> 5 files changed, 39 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 57634e2296c0..0a756c25d5e8 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -11,6 +11,7 @@
> #include <net/tc_act/tc_gate.h>
> #include <soc/mscc/ocelot.h>
> #include <linux/dsa/ocelot.h>
> +#include <linux/pcs.h>
> #include <linux/pcs-lynx.h>
> #include <net/pkt_sched.h>
> #include <linux/iopoll.h>
> @@ -1089,16 +1090,9 @@ 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_on_bus(felix->imdio, port);
> + if (IS_ERR(phylink_pcs))
> continue;
> -
> - phylink_pcs = lynx_pcs_create(mdio_device);
> - if (IS_ERR(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);
> @@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
> struct felix *felix = ocelot_to_felix(ocelot);
> int port;
>
> - 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);
> - }
> + for (port = 0; port < ocelot->num_phys_ports; port++)
> + pcs_put(felix->pcs[port]);
> 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 8c52de5d0b02..9006dec85ef0 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -9,6 +9,7 @@
> #include <linux/mdio/mdio-mscc-miim.h>
> #include <linux/of_mdio.h>
> #include <linux/of_platform.h>
> +#include <linux/pcs.h>
> #include <linux/pcs-lynx.h>
> #include <linux/dsa/ocelot.h>
> #include <linux/iopoll.h>
> @@ -1044,16 +1045,9 @@ 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_on_bus(felix->imdio, addr);
> + if (IS_ERR(phylink_pcs))
> continue;
> -
> - phylink_pcs = lynx_pcs_create(mdio_device);
> - if (IS_ERR(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);
> @@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
> struct felix *felix = ocelot_to_felix(ocelot);
> int port;
>
> - 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);
> - }
> + for (port = 0; port < ocelot->num_phys_ports; port++)
> + pcs_put(felix->pcs[port]);
>
> /* mdiobus_unregister and mdiobus_free handled by devres */
> }
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 8c923a93da88..8da7c8644e44 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -8,6 +8,7 @@
> #include <linux/of_platform.h>
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> +#include <linux/pcs.h>
> #include <linux/pcs-lynx.h>
> #include "enetc_ierb.h"
> #include "enetc_pf.h"
> @@ -827,7 +828,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;
>
> @@ -851,16 +851,8 @@ 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);
> + phylink_pcs = lynx_pcs_create_on_bus(bus, 0);
> if (IS_ERR(phylink_pcs)) {
> - mdio_device_free(mdio_device);
> err = PTR_ERR(phylink_pcs);
> dev_err(dev, "cannot create lynx pcs (%d)\n", err);
> goto unregister_mdiobus;
> @@ -880,13 +872,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);
> - lynx_pcs_destroy(pf->pcs);
> - }
> + if (pf->pcs)
> + pcs_put(pf->pcs);
> if (pf->imdio) {
> mdiobus_unregister(pf->imdio);
> mdiobus_free(pf->imdio);
> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
> index 8272072698e4..adb9fd5ce72e 100644
> --- a/drivers/net/pcs/pcs-lynx.c
> +++ b/drivers/net/pcs/pcs-lynx.c
> @@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
> }
> EXPORT_SYMBOL(lynx_pcs_create);
>
> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
> +{
> + struct mdio_device *mdio;
> + struct phylink_pcs *pcs;
> + int err;
> +
> + mdio = mdio_device_create(bus, addr);
> + if (IS_ERR(mdio))
> + return ERR_CAST(mdio);
> +
> + mdio->bus_match = mdio_device_bus_match;
> + strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias));
> + err = mdio_device_register(mdio);
Yeah, so the reason why mdio_device_register() fails with -EBUSY for the
PCS devices created by felix_vsc9959.c is this:
int mdiobus_register_device(struct mdio_device *mdiodev)
{
int err;
if (mdiodev->bus->mdio_map[mdiodev->addr])
return -EBUSY;
In other words, we already have an existing mdiodev on the bus at
address mdiodev->addr. Funnily enough, that device is actually us.
It was created at MDIO bus creation time, a dummy phydev that no one
connects to, found by mdiobus_scan(). I knew this was taking place,
but forgot/didn't realize the connection with this patch set, and that
dummy phy_device was completely harmless until now.
I can suppress its creation like this: