Re: [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()

On Mon, Jan 25, 2021 at 04:18:07PM +0000, Vladimir Oltean wrote:

Hi Dan,

On Mon, Jan 25, 2021 at 11:42:03AM +0300, Dan Carpenter wrote:

quoted

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.

Argh...  Thanks for spotting that.

quoted

 	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);

quoted

+
+	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?

Yep.  I will resend.  Thanks for basically writing v2 for me.  Your
review comments were very clear but code is always 100% clear so that's
really great.  I've never seen anyone do that before.  I should copy
that for my own reviews and hopefully it's a new trend.

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?

I did send them as a reply to v1.  Patchwork doesn't like that?

regards,
dan carpenter