| Message ID | 20201213114838.126922-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f87675b836b324d270fd52f1f5e6d6bb9f4bd1d5 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mscc: ocelot: Fix a resource leak in the error handling path of the probe function | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sun, Dec 13, 2020 at 12:48:38PM +0100, Christophe JAILLET wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > This changes the behavior slightly in another way as well, but it's probably a bug fix. drivers/net/ethernet/mscc/ocelot_vsc7514.c 1250 ports = of_get_child_by_name(np, "ethernet-ports"); 1251 if (!ports) { 1252 dev_err(ocelot->dev, "no ethernet-ports child node found\n"); 1253 return -ENODEV; 1254 } 1255 1256 ocelot->num_phys_ports = of_get_child_count(ports); 1257 ocelot->num_flooding_pgids = 1; 1258 1259 ocelot->vcap = vsc7514_vcap_props; 1260 ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; 1261 ocelot->xtr_prefix = OCELOT_TAG_PREFIX_NONE; 1262 ocelot->npi = -1; 1263 1264 err = ocelot_init(ocelot); 1265 if (err) 1266 goto out_put_ports; 1267 1268 err = mscc_ocelot_init_ports(pdev, ports); 1269 if (err) 1270 goto out_put_ports; 1271 1272 if (ocelot->ptp) { 1273 err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); 1274 if (err) { 1275 dev_err(ocelot->dev, 1276 "Timestamp initialization failed\n"); 1277 ocelot->ptp = 0; 1278 } In the original code, if ocelot_init_timestamp() failed we returned a negative error code but now we return success. This probably is what the original authors intended, though. 1279 } 1280 1281 register_netdevice_notifier(&ocelot_netdevice_nb); 1282 register_switchdev_notifier(&ocelot_switchdev_nb); 1283 register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); 1284 1285 dev_info(&pdev->dev, "Ocelot switch probed\n"); 1286 1287 out_put_ports: 1288 of_node_put(ports); 1289 return err; 1290 } regards, dan carpenter
Le 14/12/2020 à 12:48, Dan Carpenter a écrit : > On Sun, Dec 13, 2020 at 12:48:38PM +0100, Christophe JAILLET wrote: >> In case of error after calling 'ocelot_init()', it must be undone by a >> corresponding 'ocelot_deinit()' call, as already done in the remove >> function. >> > > This changes the behavior slightly in another way as well, but it's > probably a bug fix. > > drivers/net/ethernet/mscc/ocelot_vsc7514.c > 1250 ports = of_get_child_by_name(np, "ethernet-ports"); > 1251 if (!ports) { > 1252 dev_err(ocelot->dev, "no ethernet-ports child node found\n"); > 1253 return -ENODEV; > 1254 } > 1255 > 1256 ocelot->num_phys_ports = of_get_child_count(ports); > 1257 ocelot->num_flooding_pgids = 1; > 1258 > 1259 ocelot->vcap = vsc7514_vcap_props; > 1260 ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; > 1261 ocelot->xtr_prefix = OCELOT_TAG_PREFIX_NONE; > 1262 ocelot->npi = -1; > 1263 > 1264 err = ocelot_init(ocelot); > 1265 if (err) > 1266 goto out_put_ports; > 1267 > 1268 err = mscc_ocelot_init_ports(pdev, ports); > 1269 if (err) > 1270 goto out_put_ports; > 1271 > 1272 if (ocelot->ptp) { > 1273 err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); > 1274 if (err) { > 1275 dev_err(ocelot->dev, > 1276 "Timestamp initialization failed\n"); > 1277 ocelot->ptp = 0; > 1278 } > > In the original code, if ocelot_init_timestamp() failed we returned > a negative error code but now we return success. This probably is what > the original authors intended, though. > Thanks for the detailed review Dan. I agree with you. However this "fix" was not intentional. :( This may worth stating it in the commit message. Can it be done when/if the patch is applied? CJ
On 13/12/2020 12:48:38+0100, Christophe JAILLET wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > > Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > index 1e7729421a82..9cf2bc5f4289 100644 > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > @@ -1267,7 +1267,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) > > err = mscc_ocelot_init_ports(pdev, ports); > if (err) > - goto out_put_ports; > + goto out_ocelot_deinit; > > if (ocelot->ptp) { > err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); > @@ -1282,8 +1282,14 @@ static int mscc_ocelot_probe(struct platform_device *pdev) > register_switchdev_notifier(&ocelot_switchdev_nb); > register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); > > + of_node_put(ports); > + > dev_info(&pdev->dev, "Ocelot switch probed\n"); > > + return 0; > + > +out_ocelot_deinit: > + ocelot_deinit(ocelot); > out_put_ports: > of_node_put(ports); > return err; > -- > 2.27.0 >
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Sun, 13 Dec 2020 12:48:38 +0100 you wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > > Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > [...] Here is the summary with links: - net: mscc: ocelot: Fix a resource leak in the error handling path of the probe function https://git.kernel.org/netdev/net/c/f87675b836b3 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 1e7729421a82..9cf2bc5f4289 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -1267,7 +1267,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) err = mscc_ocelot_init_ports(pdev, ports); if (err) - goto out_put_ports; + goto out_ocelot_deinit; if (ocelot->ptp) { err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); @@ -1282,8 +1282,14 @@ static int mscc_ocelot_probe(struct platform_device *pdev) register_switchdev_notifier(&ocelot_switchdev_nb); register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); + of_node_put(ports); + dev_info(&pdev->dev, "Ocelot switch probed\n"); + return 0; + +out_ocelot_deinit: + ocelot_deinit(ocelot); out_put_ports: of_node_put(ports); return err;
In case of error after calling 'ocelot_init()', it must be undone by a corresponding 'ocelot_deinit()' call, as already done in the remove function. Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)