| Message ID | 20240104140037.374166-7-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 525366b81f3382ad1c76ba5e47b71e8b7925c85e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ds->user_mii_bus cleanup (part 1) | expand |
On Thu, Jan 04, 2024 at 04:00:33PM +0200, Vladimir Oltean wrote: > To simplify reasoning about why the DSA framework provides the > ds->user_mii_bus functionality, drivers should only use it if they > need to. The qca8k driver appears to also use it simply as storage > for a pointer, which is not a good enough reason to make the core > much more difficult to follow. > > ds->user_mii_bus is useful for only 2 cases: > > 1. The driver probes on platform_data (no OF) > 2. The driver probes on OF, but there is no OF node for the MDIO bus. > > It is unclear if case (1) is supported with qca8k. It might not be: > the driver might crash when of_device_get_match_data() returns NULL > and then it dereferences priv->info without NULL checking. > > Anyway, let us limit the ds->user_mii_bus usage only to the above cases, > and not assign it when an OF node is present. > > The bus->phy_mask assignment follows along with the movement, because > __of_mdiobus_register() overwrites this bus field anyway. The value set > by the driver only matters for the non-OF code path. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 5 +++-- > drivers/net/dsa/qca/qca8k-leds.c | 4 ++-- > drivers/net/dsa/qca/qca8k.h | 1 + > 3 files changed, 6 insertions(+), 4 deletions(-)
On 1/4/24 06:00, Vladimir Oltean wrote: > To simplify reasoning about why the DSA framework provides the > ds->user_mii_bus functionality, drivers should only use it if they > need to. The qca8k driver appears to also use it simply as storage > for a pointer, which is not a good enough reason to make the core > much more difficult to follow. > > ds->user_mii_bus is useful for only 2 cases: > > 1. The driver probes on platform_data (no OF) > 2. The driver probes on OF, but there is no OF node for the MDIO bus. > > It is unclear if case (1) is supported with qca8k. It might not be: > the driver might crash when of_device_get_match_data() returns NULL > and then it dereferences priv->info without NULL checking. > > Anyway, let us limit the ds->user_mii_bus usage only to the above cases, > and not assign it when an OF node is present. > > The bus->phy_mask assignment follows along with the movement, because > __of_mdiobus_register() overwrites this bus field anyway. The value set > by the driver only matters for the non-OF code path. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> To simplify reasoning about why the DSA framework provides the > ds->user_mii_bus functionality, drivers should only use it if they > need to. The qca8k driver appears to also use it simply as storage > for a pointer, which is not a good enough reason to make the core > much more difficult to follow. > > ds->user_mii_bus is useful for only 2 cases: > > 1. The driver probes on platform_data (no OF) > 2. The driver probes on OF, but there is no OF node for the MDIO bus. > > It is unclear if case (1) is supported with qca8k. It might not be: > the driver might crash when of_device_get_match_data() returns NULL > and then it dereferences priv->info without NULL checking. There are plenty of places that will not work without OF. For example, qca8k_setup_mdio_bus will return -EINVAL without ports or ethernet-ports in the device-tree. That error will abort qca8k_setup. I think it is safe to assume case (2). > Anyway, let us limit the ds->user_mii_bus usage only to the above cases, > and not assign it when an OF node is present. > > The bus->phy_mask assignment follows along with the movement, because > __of_mdiobus_register() overwrites this bus field anyway. The value set > by the driver only matters for the non-OF code path. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index 21e36bc3c015..8f69b95c894d 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -961,12 +961,11 @@ qca8k_mdio_register(struct qca8k_priv *priv) goto out_put_node; } + priv->internal_mdio_bus = bus; bus->priv = (void *)priv; snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d", ds->dst->index, ds->index); bus->parent = ds->dev; - bus->phy_mask = ~ds->phys_mii_mask; - ds->user_mii_bus = bus; /* Check if the devicetree declare the port:phy mapping */ if (mdio) { @@ -980,6 +979,8 @@ qca8k_mdio_register(struct qca8k_priv *priv) /* If a mapping can't be found the legacy mapping is used, * using the qca8k_port_to_phy function */ + ds->user_mii_bus = bus; + bus->phy_mask = ~ds->phys_mii_mask; bus->name = "qca8k-legacy user mii"; bus->read = qca8k_legacy_mdio_read; bus->write = qca8k_legacy_mdio_write; diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c index 90e30c2909e4..811ebeeff4ed 100644 --- a/drivers/net/dsa/qca/qca8k-leds.c +++ b/drivers/net/dsa/qca/qca8k-leds.c @@ -366,7 +366,6 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p { struct fwnode_handle *led = NULL, *leds = NULL; struct led_init_data init_data = { }; - struct dsa_switch *ds = priv->ds; enum led_default_state state; struct qca8k_led *port_led; int led_num, led_index; @@ -429,7 +428,8 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p init_data.default_label = ":port"; init_data.fwnode = led; init_data.devname_mandatory = true; - init_data.devicename = kasprintf(GFP_KERNEL, "%s:0%d", ds->user_mii_bus->id, + init_data.devicename = kasprintf(GFP_KERNEL, "%s:0%d", + priv->internal_mdio_bus->id, port_num); if (!init_data.devicename) return -ENOMEM; diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h index 2ac7e88f8da5..c8785c36c54e 100644 --- a/drivers/net/dsa/qca/qca8k.h +++ b/drivers/net/dsa/qca/qca8k.h @@ -454,6 +454,7 @@ struct qca8k_priv { struct qca8k_ports_config ports_config; struct regmap *regmap; struct mii_bus *bus; + struct mii_bus *internal_mdio_bus; struct dsa_switch *ds; struct mutex reg_mutex; struct device *dev;
To simplify reasoning about why the DSA framework provides the ds->user_mii_bus functionality, drivers should only use it if they need to. The qca8k driver appears to also use it simply as storage for a pointer, which is not a good enough reason to make the core much more difficult to follow. ds->user_mii_bus is useful for only 2 cases: 1. The driver probes on platform_data (no OF) 2. The driver probes on OF, but there is no OF node for the MDIO bus. It is unclear if case (1) is supported with qca8k. It might not be: the driver might crash when of_device_get_match_data() returns NULL and then it dereferences priv->info without NULL checking. Anyway, let us limit the ds->user_mii_bus usage only to the above cases, and not assign it when an OF node is present. The bus->phy_mask assignment follows along with the movement, because __of_mdiobus_register() overwrites this bus field anyway. The value set by the driver only matters for the non-OF code path. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/qca/qca8k-8xxx.c | 5 +++-- drivers/net/dsa/qca/qca8k-leds.c | 4 ++-- drivers/net/dsa/qca/qca8k.h | 1 + 3 files changed, 6 insertions(+), 4 deletions(-)