Re: [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2021-01-25 16:20:29
Also in:
kernel-janitors
Subsystem:
networking drivers, ocelot ethernet switch driver, the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Vladimir Oltean, Linus Torvalds
Hi Dan, On Mon, Jan 25, 2021 at 11:42:03AM +0300, Dan Carpenter wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c index 9553eb3e441c..875ab8532d8c 100644 --- a/drivers/net/ethernet/mscc/ocelot_net.c +++ b/drivers/net/ethernet/mscc/ocelot_net.c@@ -1262,7 +1262,6 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, ocelot_port = &priv->port; ocelot_port->ocelot = ocelot; ocelot_port->target = target; - ocelot->ports[port] = ocelot_port;
You cannot remove this from here just like that, because ocelot_init_port right below accesses ocelot->ports[port], and it will dereference through a NULL pointer otherwise.
quoted hunk ↗ jump to hunk
dev->netdev_ops = &ocelot_port_netdev_ops; dev->ethtool_ops = &ocelot_ethtool_ops;@@ -1282,7 +1281,19 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, if (err) { dev_err(ocelot->dev, "register_netdev failed\n"); free_netdev(dev); + return err; } - return err; + ocelot->ports[port] = ocelot_port; + return 0; +} + +void ocelot_release_port(struct ocelot_port *ocelot_port) +{ + struct ocelot_port_private *priv = container_of(ocelot_port, + struct ocelot_port_private, + port);
Can this assignment please be done separately from the declaration? struct ocelot_port_private *priv; priv = container_of(ocelot_port, struct ocelot_port_private, port);
+ + unregister_netdev(priv->dev); + free_netdev(priv->dev); }
Fun, isn't it? :D Thanks for taking the time to untangle this. Additionally, you have changed the meaning of "registered_ports" from "this port had its net_device registered" to "this port had its devlink_port registered". This is ok, but I would like the variable renamed now, too. I think devlink_ports_registered would be ok. In hindsight, I was foolish for using a heap-allocated boolean array for registered_ports, because this switch architecture is guaranteed to not have more than 32 ports, so a u32 bitmask is fine. If you resend, can you please squash this diff on top of your patch? -----------------------------[cut here]-----------------------------
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 1ca531b13497..7b2045707c5a 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c@@ -1196,6 +1196,7 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, ocelot_port = &priv->port; ocelot_port->ocelot = ocelot; ocelot_port->target = target; + ocelot->ports[port] = ocelot_port; dev->netdev_ops = &ocelot_port_netdev_ops; dev->ethtool_ops = &ocelot_ethtool_ops;
@@ -1215,10 +1216,10 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, if (err) { dev_err(ocelot->dev, "register_netdev failed\n"); free_netdev(dev); + ocelot->ports[port] = NULL; return err; } - ocelot->ports[port] = ocelot_port; return 0; }
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index e99b9cdcc4a7..761834466541 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c@@ -939,8 +939,8 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, struct device_node *ports) { struct ocelot *ocelot = platform_get_drvdata(pdev); + u32 devlink_ports_registered = 0; struct device_node *portnp; - bool *registered_ports; int port, err; u32 reg;
@@ -956,11 +956,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, if (!ocelot->devlink_ports) return -ENOMEM; - registered_ports = kcalloc(ocelot->num_phys_ports, sizeof(bool), - GFP_KERNEL); - if (!registered_ports) - return -ENOMEM; - for_each_available_child_of_node(ports, portnp) { struct ocelot_port_private *priv; struct ocelot_port *ocelot_port;
@@ -1009,7 +1004,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, of_node_put(portnp); goto out_teardown; } - registered_ports[port] = true; + devlink_ports_registered |= BIT(port); err = ocelot_probe_port(ocelot, port, target, phy); if (err) {
@@ -1069,17 +1064,16 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, /* Initialize unused devlink ports at the end */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (registered_ports[port]) + if (devlink_ports_registered & BIT(port)) continue; err = ocelot_port_devlink_init(ocelot, port, DEVLINK_PORT_FLAVOUR_UNUSED); if (err) goto out_teardown; - registered_ports[port] = true; - } - kfree(registered_ports); + devlink_ports_registered |= BIT(port); + } return 0;
@@ -1088,10 +1082,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, mscc_ocelot_release_ports(ocelot); /* Tear down devlink ports for the registered network interfaces */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (registered_ports[port]) + if (devlink_ports_registered & BIT(port)) ocelot_port_devlink_teardown(ocelot, port); } - kfree(registered_ports); return err; } -----------------------------[cut here]-----------------------------
Then you can add: Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> Also, it's strange but I don't see the v2 patches in patchwork. Did you send them in-reply-to v1 or something? Thanks!