Hi Dan,
On Mon, Jan 25, 2021 at 11:42:03AM +0300, Dan Carpenter wrote:
> 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.
> 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@xxxxxxx>
Tested-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
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!
