* [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
@ 2021-01-25 8:12 Dan Carpenter
2021-01-25 8:13 ` [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dan Carpenter @ 2021-01-25 8:12 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, netdev, kernel-janitors
There are several error handling bugs in mscc_ocelot_init_ports(). I
went through the code, and carefully audited it and made fixes and
cleanups.
1) The ocelot_probe_port() function didn't have a mirror release function
so it was hard to follow. I created the ocelot_release_port()
function.
2) In the ocelot_probe_port() function, if the register_netdev() call
failed, then it lead to a double free_netdev(dev) bug. Fix this
by moving the "ocelot->ports[port] = ocelot_port;" assignment to the
end of the function after everything has succeeded.
3) I was concerned that the "port" which comes from of_property_read_u32()
might be out of bounds so I added a check for that.
4) In the original code if ocelot_regmap_init() failed then the driver
tried to continue but I think that should be a fatal error.
5) If ocelot_probe_port() failed then the most recent devlink was leaked.
Fix this by moving the "registered_ports[port] = true;" assignment
earlier.
6) The error handling if the final ocelot_port_devlink_init() failed had
two problems. The "while (port-- >= 0)" loop should have been
"--port" pre-op instead of a post-op to avoid a buffer underflow.
The "if (!registered_ports[port])" condition was reversed leading to
resource leaks and double frees.
Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 39 +++++++++-------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 30a38df08a21..2c82ffe2c611 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1064,7 +1064,6 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot)
int port;
for (port = 0; port < ocelot->num_phys_ports; port++) {
- struct ocelot_port_private *priv;
struct ocelot_port *ocelot_port;
ocelot_port = ocelot->ports[port];
@@ -1072,12 +1071,7 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot)
continue;
ocelot_deinit_port(ocelot, port);
-
- priv = container_of(ocelot_port, struct ocelot_port_private,
- port);
-
- unregister_netdev(priv->dev);
- free_netdev(priv->dev);
+ ocelot_release_port(ocelot_port);
}
}
@@ -1123,14 +1117,22 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
continue;
port = reg;
+ if (port < 0 || port >= ocelot->num_phys_ports) {
+ dev_err(ocelot->dev,
+ "invalid port number: %d >= %d\n", port,
+ ocelot->num_phys_ports);
+ continue;
+ }
snprintf(res_name, sizeof(res_name), "port%d", port);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
res_name);
target = ocelot_regmap_init(ocelot, res);
- if (IS_ERR(target))
- continue;
+ if (IS_ERR(target)) {
+ err = PTR_ERR(target);
+ goto out_teardown;
+ }
phy_node = of_parse_phandle(portnp, "phy-handle", 0);
if (!phy_node)
@@ -1147,6 +1149,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
of_node_put(portnp);
goto out_teardown;
}
+ registered_ports[port] = true;
err = ocelot_probe_port(ocelot, port, target, phy);
if (err) {
@@ -1154,8 +1157,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
goto out_teardown;
}
- registered_ports[port] = true;
-
ocelot_port = ocelot->ports[port];
priv = container_of(ocelot_port, struct ocelot_port_private,
port);
@@ -1213,15 +1214,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
err = ocelot_port_devlink_init(ocelot, port,
DEVLINK_PORT_FLAVOUR_UNUSED);
- if (err) {
- while (port-- >= 0) {
- if (!registered_ports[port])
- continue;
- ocelot_port_devlink_teardown(ocelot, port);
- }
-
+ if (err)
goto out_teardown;
- }
+ registered_ports[port] = true;
}
kfree(registered_ports);
@@ -1233,10 +1228,8 @@ 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])
- continue;
-
- ocelot_port_devlink_teardown(ocelot, port);
+ if (registered_ports[port])
+ ocelot_port_devlink_teardown(ocelot, port);
}
kfree(registered_ports);
return err;
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe()
2021-01-25 8:12 [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
@ 2021-01-25 8:13 ` Dan Carpenter
2021-01-25 13:23 ` Vladimir Oltean
2021-01-25 8:19 ` [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
2021-01-25 8:42 ` [PATCH v2 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-25 8:13 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
Probe should return an error code if platform_get_irq_byname() fails
but it returns success instead.
Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 2c82ffe2c611..7e4c9b255124 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1307,8 +1307,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
goto out_free_devlink;
irq_xtr = platform_get_irq_byname(pdev, "xtr");
- if (irq_xtr < 0)
+ if (irq_xtr < 0) {
+ err = irq_xtr;
goto out_free_devlink;
+ }
err = devm_request_threaded_irq(&pdev->dev, irq_xtr, NULL,
ocelot_xtr_irq_handler, IRQF_ONESHOT,
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
2021-01-25 8:12 [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
2021-01-25 8:13 ` [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
@ 2021-01-25 8:19 ` Dan Carpenter
2021-01-25 8:42 ` [PATCH v2 " Dan Carpenter
2021-01-25 8:42 ` [PATCH v2 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-25 8:19 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, netdev, kernel-janitors
Oops. Please ignore this patch. Part of it is missing.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
2021-01-25 8:19 ` [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
@ 2021-01-25 8:42 ` Dan Carpenter
2021-01-25 16:18 ` Vladimir Oltean
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-25 8:42 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
There are several error handling bugs in mscc_ocelot_init_ports(). I
went through the code, and carefully audited it and made fixes and
cleanups.
1) The ocelot_probe_port() function didn't have a mirror release function
so it was hard to follow. I created the ocelot_release_port()
function.
2) In the ocelot_probe_port() function, if the register_netdev() call
failed, then it lead to a double free_netdev(dev) bug. Fix this
by moving the "ocelot->ports[port] = ocelot_port;" assignment to the
end of the function after everything has succeeded.
3) I was concerned that the "port" which comes from of_property_read_u32()
might be out of bounds so I added a check for that.
4) In the original code if ocelot_regmap_init() failed then the driver
tried to continue but I think that should be a fatal error.
5) If ocelot_probe_port() failed then the most recent devlink was leaked.
Fix this by moving the "registered_ports[port] = true;" assignment
earlier.
6) The error handling if the final ocelot_port_devlink_init() failed had
two problems. The "while (port-- >= 0)" loop should have been
"--port" pre-op instead of a post-op to avoid a buffer underflow.
The "if (!registered_ports[port])" condition was reversed leading to
resource leaks and double frees.
Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1, I accidentally left out parts of the commit so it didn't
compile.
drivers/net/ethernet/mscc/ocelot.h | 1 +
drivers/net/ethernet/mscc/ocelot_net.c | 15 +++++++--
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 39 +++++++++-------------
3 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index e8621dbc14f7..76b8d8ce3b48 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -121,6 +121,7 @@ void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg);
int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
struct phy_device *phy);
+void ocelot_release_port(struct ocelot_port *ocelot_port);
int ocelot_devlink_init(struct ocelot *ocelot);
void ocelot_devlink_teardown(struct ocelot *ocelot);
int ocelot_port_devlink_init(struct ocelot *ocelot, int port,
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;
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);
+
+ unregister_netdev(priv->dev);
+ free_netdev(priv->dev);
}
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 30a38df08a21..2c82ffe2c611 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1064,7 +1064,6 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot)
int port;
for (port = 0; port < ocelot->num_phys_ports; port++) {
- struct ocelot_port_private *priv;
struct ocelot_port *ocelot_port;
ocelot_port = ocelot->ports[port];
@@ -1072,12 +1071,7 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot)
continue;
ocelot_deinit_port(ocelot, port);
-
- priv = container_of(ocelot_port, struct ocelot_port_private,
- port);
-
- unregister_netdev(priv->dev);
- free_netdev(priv->dev);
+ ocelot_release_port(ocelot_port);
}
}
@@ -1123,14 +1117,22 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
continue;
port = reg;
+ if (port < 0 || port >= ocelot->num_phys_ports) {
+ dev_err(ocelot->dev,
+ "invalid port number: %d >= %d\n", port,
+ ocelot->num_phys_ports);
+ continue;
+ }
snprintf(res_name, sizeof(res_name), "port%d", port);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
res_name);
target = ocelot_regmap_init(ocelot, res);
- if (IS_ERR(target))
- continue;
+ if (IS_ERR(target)) {
+ err = PTR_ERR(target);
+ goto out_teardown;
+ }
phy_node = of_parse_phandle(portnp, "phy-handle", 0);
if (!phy_node)
@@ -1147,6 +1149,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
of_node_put(portnp);
goto out_teardown;
}
+ registered_ports[port] = true;
err = ocelot_probe_port(ocelot, port, target, phy);
if (err) {
@@ -1154,8 +1157,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
goto out_teardown;
}
- registered_ports[port] = true;
-
ocelot_port = ocelot->ports[port];
priv = container_of(ocelot_port, struct ocelot_port_private,
port);
@@ -1213,15 +1214,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev,
err = ocelot_port_devlink_init(ocelot, port,
DEVLINK_PORT_FLAVOUR_UNUSED);
- if (err) {
- while (port-- >= 0) {
- if (!registered_ports[port])
- continue;
- ocelot_port_devlink_teardown(ocelot, port);
- }
-
+ if (err)
goto out_teardown;
- }
+ registered_ports[port] = true;
}
kfree(registered_ports);
@@ -1233,10 +1228,8 @@ 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])
- continue;
-
- ocelot_port_devlink_teardown(ocelot, port);
+ if (registered_ports[port])
+ ocelot_port_devlink_teardown(ocelot, port);
}
kfree(registered_ports);
return err;
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe()
2021-01-25 8:12 [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
2021-01-25 8:13 ` [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
2021-01-25 8:19 ` [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
@ 2021-01-25 8:42 ` Dan Carpenter
2 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2021-01-25 8:42 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
Probe should return an error code if platform_get_irq_byname() fails
but it returns success instead.
Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: no change
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 2c82ffe2c611..7e4c9b255124 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1307,8 +1307,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
goto out_free_devlink;
irq_xtr = platform_get_irq_byname(pdev, "xtr");
- if (irq_xtr < 0)
+ if (irq_xtr < 0) {
+ err = irq_xtr;
goto out_free_devlink;
+ }
err = devm_request_threaded_irq(&pdev->dev, irq_xtr, NULL,
ocelot_xtr_irq_handler, IRQF_ONESHOT,
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe()
2021-01-25 8:13 ` [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
@ 2021-01-25 13:23 ` Vladimir Oltean
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-01-25 13:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
On Mon, Jan 25, 2021 at 11:13:19AM +0300, Dan Carpenter wrote:
> Probe should return an error code if platform_get_irq_byname() fails
> but it returns success instead.
>
> Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
2021-01-25 8:42 ` [PATCH v2 " Dan Carpenter
@ 2021-01-25 16:18 ` Vladimir Oltean
2021-01-26 7:27 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Oltean @ 2021-01-25 16:18 UTC (permalink / raw)
To: Dan Carpenter
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
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@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!
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
2021-01-25 16:18 ` Vladimir Oltean
@ 2021-01-26 7:27 ` Dan Carpenter
2021-01-26 8:51 ` Vladimir Oltean
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-01-26 7:27 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
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:
> > 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.
> > 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?
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports()
2021-01-26 7:27 ` Dan Carpenter
@ 2021-01-26 8:51 ` Vladimir Oltean
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-01-26 8:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
David S. Miller, Jakub Kicinski, Florian Fainelli, netdev,
kernel-janitors
On Tue, Jan 26, 2021 at 10:27:53AM +0300, Dan Carpenter wrote:
> > 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.
Until the code is wrong :)
> I've never seen anyone do that before. I should copy
> that for my own reviews and hopefully it's a new trend.
I took that from Florian, it was helpful to me when I wasn't minding my
own business and touching code I couldn't test:
https://patchwork.kernel.org/project/netdevbpf/patch/20201218223852.2717102-4-vladimir.oltean@nxp.com/#23858835
> > 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?
Nope.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-26 21:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 8:12 [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
2021-01-25 8:13 ` [PATCH 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
2021-01-25 13:23 ` Vladimir Oltean
2021-01-25 8:19 ` [PATCH 1/2 net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter
2021-01-25 8:42 ` [PATCH v2 " Dan Carpenter
2021-01-25 16:18 ` Vladimir Oltean
2021-01-26 7:27 ` Dan Carpenter
2021-01-26 8:51 ` Vladimir Oltean
2021-01-25 8:42 ` [PATCH v2 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).