| Message ID | 20230518153230.1584962-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: Fix additional refcount leak in rk817_charger_probe | expand |
Hi, On Thu, May 18, 2023 at 10:32:30AM -0500, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > Dan Carpenter reports that the Smatch static checker warning has found > that there is another refcount leak in the probe function. While > of_node_put() was added in one of the return paths, it should in > fact be added for ALL return paths that return an error. > > Fixes: 54c03bfd094f ("power: supply: Fix refcount leak in rk817_charger_probe") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Closes: https://lore.kernel.org/linux-pm/dc0bb0f8-212d-4be7-be69-becd2a3f9a80@kili.mountain/ > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- This kind of fell through the cracks, sorry. Looking at it now it's incomplete: Even after this patch there is still a leak on module removal. You should use devm_add_action_or_reset() to register a handler doing of_node_put(). That also results in a much smaller diff :) Greetings, -- Sebastian > drivers/power/supply/rk817_charger.c | 76 ++++++++++++++++++---------- > 1 file changed, 48 insertions(+), 28 deletions(-) > > diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c > index 1a2143641e66..bd4f530910a5 100644 > --- a/drivers/power/supply/rk817_charger.c > +++ b/drivers/power/supply/rk817_charger.c > @@ -1063,8 +1063,8 @@ static int rk817_charger_probe(struct platform_device *pdev) > > charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL); > if (!charger) { > - of_node_put(node); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out_node_put; > } > > charger->rk808 = rk808; > @@ -1085,8 +1085,9 @@ static int rk817_charger_probe(struct platform_device *pdev) > ret = of_property_read_u32(node, "rockchip,resistor-sense-micro-ohms", > &of_value); > if (ret < 0) { > - return dev_err_probe(dev, ret, > - "Error reading sample resistor value\n"); > + dev_err_probe(dev, ret, > + "Error reading sample resistor value\n"); > + goto out_node_put; > } > /* > * Store as a 1 or a 2, since all we really use the value for is as a > @@ -1102,8 +1103,9 @@ static int rk817_charger_probe(struct platform_device *pdev) > "rockchip,sleep-enter-current-microamp", > &of_value); > if (ret < 0) { > - return dev_err_probe(dev, ret, > - "Error reading sleep enter cur value\n"); > + dev_err_probe(dev, ret, > + "Error reading sleep enter cur value\n"); > + goto out_node_put; > } > charger->sleep_enter_current_ua = of_value; > > @@ -1112,29 +1114,35 @@ static int rk817_charger_probe(struct platform_device *pdev) > "rockchip,sleep-filter-current-microamp", > &of_value); > if (ret < 0) { > - return dev_err_probe(dev, ret, > - "Error reading sleep filter cur value\n"); > + dev_err_probe(dev, ret, > + "Error reading sleep filter cur value\n"); > + goto out_node_put; > } > > charger->sleep_filter_current_ua = of_value; > > charger->bat_ps = devm_power_supply_register(&pdev->dev, > &rk817_bat_desc, &pscfg); > - if (IS_ERR(charger->bat_ps)) > - return dev_err_probe(dev, -EINVAL, > - "Battery failed to probe\n"); > + if (IS_ERR(charger->bat_ps)) { > + dev_err_probe(dev, -EINVAL, > + "Battery failed to probe\n"); > + goto out_node_put; > + } > > charger->chg_ps = devm_power_supply_register(&pdev->dev, > &rk817_chg_desc, &pscfg); > - if (IS_ERR(charger->chg_ps)) > - return dev_err_probe(dev, -EINVAL, > - "Charger failed to probe\n"); > + if (IS_ERR(charger->chg_ps)) { > + dev_err_probe(dev, -EINVAL, > + "Charger failed to probe\n"); > + goto out_node_put; > + } > > ret = power_supply_get_battery_info(charger->bat_ps, > &bat_info); > if (ret) { > - return dev_err_probe(dev, ret, > - "Unable to get battery info: %d\n", ret); > + dev_err_probe(dev, ret, > + "Unable to get battery info: %d\n", ret); > + goto out_node_put; > } > > if ((bat_info->charge_full_design_uah <= 0) || > @@ -1143,8 +1151,10 @@ static int rk817_charger_probe(struct platform_device *pdev) > (bat_info->constant_charge_voltage_max_uv <= 0) || > (bat_info->constant_charge_current_max_ua <= 0) || > (bat_info->charge_term_current_ua <= 0)) { > - return dev_err_probe(dev, -EINVAL, > - "Required bat info missing or invalid\n"); > + ret = -EINVAL; > + dev_err_probe(dev, ret, > + "Required bat info missing or invalid\n"); > + goto out_node_put; > } > > charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah; > @@ -1157,25 +1167,30 @@ static int rk817_charger_probe(struct platform_device *pdev) > */ > ret = rk817_battery_init(charger, bat_info); > if (ret) > - return ret; > + goto out_node_put; > > power_supply_put_battery_info(charger->bat_ps, bat_info); > > plugin_irq = platform_get_irq(pdev, 0); > - if (plugin_irq < 0) > - return plugin_irq; > + if (plugin_irq < 0) { > + ret = plugin_irq; > + goto out_node_put; > + } > > plugout_irq = platform_get_irq(pdev, 1); > - if (plugout_irq < 0) > - return plugout_irq; > + if (plugout_irq < 0) { > + ret = plugout_irq; > + goto out_node_put; > + } > > ret = devm_request_threaded_irq(charger->dev, plugin_irq, NULL, > rk817_plug_in_isr, > IRQF_TRIGGER_RISING | IRQF_ONESHOT, > "rk817_plug_in", charger); > if (ret) { > - return dev_err_probe(&pdev->dev, ret, > - "plug_in_irq request failed!\n"); > + dev_err_probe(&pdev->dev, ret, > + "plug_in_irq request failed!\n"); > + goto out_node_put; > } > > ret = devm_request_threaded_irq(charger->dev, plugout_irq, NULL, > @@ -1183,19 +1198,24 @@ static int rk817_charger_probe(struct platform_device *pdev) > IRQF_TRIGGER_RISING | IRQF_ONESHOT, > "rk817_plug_out", charger); > if (ret) { > - return dev_err_probe(&pdev->dev, ret, > - "plug_out_irq request failed!\n"); > + dev_err_probe(&pdev->dev, ret, > + "plug_out_irq request failed!\n"); > + goto out_node_put; > } > > ret = devm_delayed_work_autocancel(&pdev->dev, &charger->work, > rk817_charging_monitor); > if (ret) > - return ret; > + goto out_node_put; > > /* Force the first update immediately. */ > mod_delayed_work(system_wq, &charger->work, 0); > > return 0; > + > +out_node_put: > + of_node_put(node); > + return ret; > } > > > -- > 2.34.1 >
diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c index 1a2143641e66..bd4f530910a5 100644 --- a/drivers/power/supply/rk817_charger.c +++ b/drivers/power/supply/rk817_charger.c @@ -1063,8 +1063,8 @@ static int rk817_charger_probe(struct platform_device *pdev) charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL); if (!charger) { - of_node_put(node); - return -ENOMEM; + ret = -ENOMEM; + goto out_node_put; } charger->rk808 = rk808; @@ -1085,8 +1085,9 @@ static int rk817_charger_probe(struct platform_device *pdev) ret = of_property_read_u32(node, "rockchip,resistor-sense-micro-ohms", &of_value); if (ret < 0) { - return dev_err_probe(dev, ret, - "Error reading sample resistor value\n"); + dev_err_probe(dev, ret, + "Error reading sample resistor value\n"); + goto out_node_put; } /* * Store as a 1 or a 2, since all we really use the value for is as a @@ -1102,8 +1103,9 @@ static int rk817_charger_probe(struct platform_device *pdev) "rockchip,sleep-enter-current-microamp", &of_value); if (ret < 0) { - return dev_err_probe(dev, ret, - "Error reading sleep enter cur value\n"); + dev_err_probe(dev, ret, + "Error reading sleep enter cur value\n"); + goto out_node_put; } charger->sleep_enter_current_ua = of_value; @@ -1112,29 +1114,35 @@ static int rk817_charger_probe(struct platform_device *pdev) "rockchip,sleep-filter-current-microamp", &of_value); if (ret < 0) { - return dev_err_probe(dev, ret, - "Error reading sleep filter cur value\n"); + dev_err_probe(dev, ret, + "Error reading sleep filter cur value\n"); + goto out_node_put; } charger->sleep_filter_current_ua = of_value; charger->bat_ps = devm_power_supply_register(&pdev->dev, &rk817_bat_desc, &pscfg); - if (IS_ERR(charger->bat_ps)) - return dev_err_probe(dev, -EINVAL, - "Battery failed to probe\n"); + if (IS_ERR(charger->bat_ps)) { + dev_err_probe(dev, -EINVAL, + "Battery failed to probe\n"); + goto out_node_put; + } charger->chg_ps = devm_power_supply_register(&pdev->dev, &rk817_chg_desc, &pscfg); - if (IS_ERR(charger->chg_ps)) - return dev_err_probe(dev, -EINVAL, - "Charger failed to probe\n"); + if (IS_ERR(charger->chg_ps)) { + dev_err_probe(dev, -EINVAL, + "Charger failed to probe\n"); + goto out_node_put; + } ret = power_supply_get_battery_info(charger->bat_ps, &bat_info); if (ret) { - return dev_err_probe(dev, ret, - "Unable to get battery info: %d\n", ret); + dev_err_probe(dev, ret, + "Unable to get battery info: %d\n", ret); + goto out_node_put; } if ((bat_info->charge_full_design_uah <= 0) || @@ -1143,8 +1151,10 @@ static int rk817_charger_probe(struct platform_device *pdev) (bat_info->constant_charge_voltage_max_uv <= 0) || (bat_info->constant_charge_current_max_ua <= 0) || (bat_info->charge_term_current_ua <= 0)) { - return dev_err_probe(dev, -EINVAL, - "Required bat info missing or invalid\n"); + ret = -EINVAL; + dev_err_probe(dev, ret, + "Required bat info missing or invalid\n"); + goto out_node_put; } charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah; @@ -1157,25 +1167,30 @@ static int rk817_charger_probe(struct platform_device *pdev) */ ret = rk817_battery_init(charger, bat_info); if (ret) - return ret; + goto out_node_put; power_supply_put_battery_info(charger->bat_ps, bat_info); plugin_irq = platform_get_irq(pdev, 0); - if (plugin_irq < 0) - return plugin_irq; + if (plugin_irq < 0) { + ret = plugin_irq; + goto out_node_put; + } plugout_irq = platform_get_irq(pdev, 1); - if (plugout_irq < 0) - return plugout_irq; + if (plugout_irq < 0) { + ret = plugout_irq; + goto out_node_put; + } ret = devm_request_threaded_irq(charger->dev, plugin_irq, NULL, rk817_plug_in_isr, IRQF_TRIGGER_RISING | IRQF_ONESHOT, "rk817_plug_in", charger); if (ret) { - return dev_err_probe(&pdev->dev, ret, - "plug_in_irq request failed!\n"); + dev_err_probe(&pdev->dev, ret, + "plug_in_irq request failed!\n"); + goto out_node_put; } ret = devm_request_threaded_irq(charger->dev, plugout_irq, NULL, @@ -1183,19 +1198,24 @@ static int rk817_charger_probe(struct platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_ONESHOT, "rk817_plug_out", charger); if (ret) { - return dev_err_probe(&pdev->dev, ret, - "plug_out_irq request failed!\n"); + dev_err_probe(&pdev->dev, ret, + "plug_out_irq request failed!\n"); + goto out_node_put; } ret = devm_delayed_work_autocancel(&pdev->dev, &charger->work, rk817_charging_monitor); if (ret) - return ret; + goto out_node_put; /* Force the first update immediately. */ mod_delayed_work(system_wq, &charger->work, 0); return 0; + +out_node_put: + of_node_put(node); + return ret; }