* [PATCH] power: supply: Fix additional refcount leak in rk817_charger_probe
@ 2023-05-18 15:32 Chris Morgan
2023-09-15 22:12 ` Sebastian Reichel
0 siblings, 1 reply; 2+ messages in thread
From: Chris Morgan @ 2023-05-18 15:32 UTC (permalink / raw)
To: linux-pm; +Cc: linqiheng, macromorgan, sre, Dan Carpenter
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>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] power: supply: Fix additional refcount leak in rk817_charger_probe
2023-05-18 15:32 [PATCH] power: supply: Fix additional refcount leak in rk817_charger_probe Chris Morgan
@ 2023-09-15 22:12 ` Sebastian Reichel
0 siblings, 0 replies; 2+ messages in thread
From: Sebastian Reichel @ 2023-09-15 22:12 UTC (permalink / raw)
To: Chris Morgan; +Cc: linux-pm, linqiheng, macromorgan, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 6677 bytes --]
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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-15 22:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-18 15:32 [PATCH] power: supply: Fix additional refcount leak in rk817_charger_probe Chris Morgan
2023-09-15 22:12 ` Sebastian Reichel
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).