From: Hans de Goede <hansg@kernel.org>
To: Sebastian Reichel <sebastian.reichel@collabora.com>,
Sebastian Reichel <sre@kernel.org>,
Mark Brown <broonie@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>
Cc: "Liam Girdwood" <lgirdwood@gmail.com>,
"Vinod Koul" <vkoul@kernel.org>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
"Chen-Yu Tsai" <wens@csie.org>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Samuel Holland" <samuel@sholland.org>,
"Matti Vaittinen" <mazziesaccount@gmail.com>,
"Pali Rohár" <pali@kernel.org>,
"Krzysztof Kozlowski" <krzk@kernel.org>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora.com>,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/5] power: supply: core: battery-info: fully switch to fwnode
Date: Sun, 8 Jun 2025 21:57:14 +0200 [thread overview]
Message-ID: <68792092-4689-4e3e-8dbb-9157889f1a3d@kernel.org> (raw)
In-Reply-To: <20250430-psy-core-convert-to-fwnode-v2-3-f9643b958677@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 8787 bytes --]
Hi Sebastian,
On 30-Apr-25 12:54 AM, Sebastian Reichel wrote:
> Also use fwnode based parsing for "ocv-capacity-celsius" and
> "resistance-temp-table", so that any DT specific bits are
> removed from the power-supply core.
>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
I have been testing this converting the ug3105 driver to
use power_supply_batinfo_ocv2cap(), replacing the hardcoded
ocv -> capacity table in that driver.
While testing I hit a bug and while looking closer at this
patch it needs more work on top of fixing that bug.
See comments inline, also I've attached 3 fixup patches
which can be squashed into this patch to address the remarks.
> ---
> drivers/power/supply/power_supply_core.c | 109 ++++++++++++++++++-------------
> 1 file changed, 63 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 89947f1fe610d8a75756e1e4e5339b06349f9ab8..a8d1fe66e2486a833ccaa3ed77b861c6e52c5760 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -585,32 +585,19 @@ int power_supply_get_battery_info(struct power_supply *psy,
> {
> struct power_supply_resistance_temp_table *resist_table;
> struct power_supply_battery_info *info;
> - struct device_node *battery_np = NULL;
> - struct fwnode_reference_args args;
> - struct fwnode_handle *fwnode = NULL;
> + struct fwnode_handle *srcnode, *fwnode;
> const char *value;
> - int err, len, index;
> - const __be32 *list;
> + int err, len, index, proplen;
> + u32 *propdata;
propname which is also a local-variable for a temporary
malloc-ed buffer uses __free(kfree) instead of explicit
kfree() calls IMHO it would be good to do that here too.
This requires declaring it inside the
"for (index = 0; index < len; index++)" loop like how
this is done for propname, so that it gets freed on
every loop iteration.
> u32 min_max[2];
>
> - if (psy->dev.of_node) {
> - battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0);
> - if (!battery_np)
> - return -ENODEV;
> + srcnode = dev_fwnode(&psy->dev);
> + if (!srcnode && psy->dev.parent)
> + srcnode = dev_fwnode(psy->dev.parent);
>
> - fwnode = fwnode_handle_get(of_fwnode_handle(battery_np));
> - } else if (psy->dev.parent) {
> - err = fwnode_property_get_reference_args(
> - dev_fwnode(psy->dev.parent),
> - "monitored-battery", NULL, 0, 0, &args);
> - if (err)
> - return err;
> -
> - fwnode = args.fwnode;
> - }
> -
> - if (!fwnode)
> - return -ENOENT;
> + fwnode = fwnode_find_reference(srcnode, "monitored-battery", 0);
> + if (IS_ERR(fwnode))
> + return PTR_ERR(fwnode);
>
> err = fwnode_property_read_string(fwnode, "compatible", &value);
> if (err)
> @@ -740,15 +727,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
> info->temp_max = min_max[1];
> }
>
> - /*
> - * The below code uses raw of-data parsing to parse
> - * /schemas/types.yaml#/definitions/uint32-matrix
> - * data, so for now this is only support with of.
> - */
> - if (!battery_np)
> - goto out_ret_pointer;
> -
> - len = of_property_count_u32_elems(battery_np, "ocv-capacity-celsius");
> + len = fwnode_property_count_u32(fwnode, "ocv-capacity-celsius");
> if (len < 0 && len != -EINVAL) {
> err = len;
> goto out_put_node;
> @@ -757,13 +736,13 @@ int power_supply_get_battery_info(struct power_supply *psy,
> err = -EINVAL;
> goto out_put_node;
> } else if (len > 0) {
> - of_property_read_u32_array(battery_np, "ocv-capacity-celsius",
> + fwnode_property_read_u32_array(fwnode, "ocv-capacity-celsius",
> info->ocv_temp, len);
> }
>
> for (index = 0; index < len; index++) {
> struct power_supply_battery_ocv_table *table;
> - int i, tab_len, size;
> + int i, tab_len;
>
> char *propname __free(kfree) = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d",
> index);
> @@ -772,60 +751,98 @@ int power_supply_get_battery_info(struct power_supply *psy,
> err = -ENOMEM;
> goto out_put_node;
> }
> - list = of_get_property(battery_np, propname, &size);
> - if (!list || !size) {
> + proplen = fwnode_property_count_u32(fwnode, propname);
> + if (proplen < 0 || proplen % 2 != 0) {
> dev_err(&psy->dev, "failed to get %s\n", propname);
> power_supply_put_battery_info(psy, info);
> err = -EINVAL;
> goto out_put_node;
> }
> + propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
As mentioned above I suggest to use the following here instead:
u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
> + if (!propdata) {
> + kfree(propname);
propname must NOT be free-ed here since it is marked __free(kfree),
freeing it here will cause a double-free bug.
> + power_supply_put_battery_info(psy, info);
> + err = -EINVAL;
> + goto out_put_node;
> + }
> + err = fwnode_property_read_u32_array(fwnode, propname, propdata, proplen);
> + if (err < 0) {
> + dev_err(&psy->dev, "failed to get %s\n", propname);
> + kfree(propname);
same as above.
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> + power_supply_put_battery_info(psy, info);
> + goto out_put_node;
> + }
>
> - tab_len = size / (2 * sizeof(__be32));
> + tab_len = proplen / 2;
> info->ocv_table_size[index] = tab_len;
>
> info->ocv_table[index] = table =
> devm_kcalloc(&psy->dev, tab_len, sizeof(*table), GFP_KERNEL);
> if (!info->ocv_table[index]) {
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> power_supply_put_battery_info(psy, info);
> err = -ENOMEM;
> goto out_put_node;
> }
>
> for (i = 0; i < tab_len; i++) {
> - table[i].ocv = be32_to_cpu(*list);
> - list++;
> - table[i].capacity = be32_to_cpu(*list);
> - list++;
> + table[i].ocv = propdata[i*2];
> + table[i].capacity = propdata[i*2+1];
> }
> +
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> }
>
> - list = of_get_property(battery_np, "resistance-temp-table", &len);
> - if (!list || !len)
> + proplen = fwnode_property_count_u32(fwnode, "resistance-temp-table");
This will return -EINVAL when the property does not exist, making
power_supply_get_battery_info() always fail when there is no
"resistance-temp-table" in the battery fwnode. See the attached fixup
patch for a suggested fix.
> + if (proplen < 0 || proplen % 2 != 0) {
> + power_supply_put_battery_info(psy, info);
> + err = -ENOMEM;
-ENOMEM is the wrong error code here.
> goto out_ret_pointer;
This should be "goto out_put_node" since this is an error path.
> + } else if (proplen == 0) {
> + goto out_ret_pointer;
> + }
>
> - info->resist_table_size = len / (2 * sizeof(__be32));
> + propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
As mentioned above I suggest to use the following here instead:
u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
> + if (!propdata) {
> + power_supply_put_battery_info(psy, info);
> + err = -ENOMEM;
> + goto out_ret_pointer;
This should be "goto out_put_node" since this is an error path.
> + }
> +
> + err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
> + propdata, proplen);
> + if (err < 0) {
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> + power_supply_put_battery_info(psy, info);
> + goto out_put_node;
> + }
> +
> + info->resist_table_size = proplen / 2;
> info->resist_table = resist_table = devm_kcalloc(&psy->dev,
> info->resist_table_size,
> sizeof(*resist_table),
> GFP_KERNEL);
> if (!info->resist_table) {
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> power_supply_put_battery_info(psy, info);
> err = -ENOMEM;
> goto out_put_node;
> }
>
> for (index = 0; index < info->resist_table_size; index++) {
> - resist_table[index].temp = be32_to_cpu(*list++);
> - resist_table[index].resistance = be32_to_cpu(*list++);
> + resist_table[index].temp = propdata[index*2];
> + resist_table[index].resistance = propdata[index*2+1];
> }
>
> + kfree(propdata);
with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
> +
> out_ret_pointer:
> /* Finally return the whole thing */
> *info_out = info;
>
> out_put_node:
> fwnode_handle_put(fwnode);
> - of_node_put(battery_np);
> return err;
> }
> EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
>
Regards,
Hans
[-- Attachment #2: 0004-fixup-power-supply-core-battery-info-fully-switch-to.patch --]
[-- Type: text/x-patch, Size: 2103 bytes --]
From 68d2414d201083904dad83af4ebb2c35fc850b79 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 8 Jun 2025 21:18:11 +0200
Subject: [PATCH 04/10] fixup! power: supply: core: battery-info: fully switch
to fwnode
After ("power: supply: core: battery-info: fully switch to fwnode")
power_supply_get_battery_info() will always fail for battery fwnodes
which do not define a "resistance-temp-table".
Fix this by cleanly exiting on both 0 and EINVAL returns from
fwnode_property_count_u32(fwnode, "resistance-temp-table") which
indicates that the property is empty or not there.
While at it also fix:
1. The weird -ENOMEM return for other errors. For other errors
propagate the existing error or -EINVAL for an odd proplen.
2. Wrongly using "goto out_ret_pointer" on errors, out_ret_pointer
should only be used on success, error paths should use
out_put_node;
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
drivers/power/supply/power_supply_core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index a8d1fe66e248..9bbc3be2e483 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -795,19 +795,20 @@ int power_supply_get_battery_info(struct power_supply *psy,
}
proplen = fwnode_property_count_u32(fwnode, "resistance-temp-table");
- if (proplen < 0 || proplen % 2 != 0) {
+ if (proplen == 0 || proplen == -EINVAL) {
+ err = 0;
+ goto out_ret_pointer;
+ } else if (proplen < 0 || proplen % 2 != 0) {
power_supply_put_battery_info(psy, info);
- err = -ENOMEM;
- goto out_ret_pointer;
- } else if (proplen == 0) {
- goto out_ret_pointer;
+ err = (proplen < 0) ? proplen : -EINVAL;
+ goto out_put_node;
}
propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
if (!propdata) {
power_supply_put_battery_info(psy, info);
err = -ENOMEM;
- goto out_ret_pointer;
+ goto out_put_node;
}
err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
--
2.49.0
[-- Attachment #3: 0005-fixup-power-supply-core-battery-info-fully-switch-to.patch --]
[-- Type: text/x-patch, Size: 3573 bytes --]
From b92ac226fa6888d7a53fc76815110e76e13b85ae Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 8 Jun 2025 21:37:59 +0200
Subject: [PATCH 05/10] fixup! power: supply: core: battery-info: fully switch
to fwnode
propname must NOT be manually free-ed here since it is marked
__free(kfree), freeing it will cause a double-free bug.
Also move propdata to be a __free(kfree) value for consistency
and also for cleaner code.
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
drivers/power/supply/power_supply_core.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 9bbc3be2e483..0e42b9bafcab 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -588,7 +588,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
struct fwnode_handle *srcnode, *fwnode;
const char *value;
int err, len, index, proplen;
- u32 *propdata;
u32 min_max[2];
srcnode = dev_fwnode(&psy->dev);
@@ -758,9 +757,9 @@ int power_supply_get_battery_info(struct power_supply *psy,
err = -EINVAL;
goto out_put_node;
}
- propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
+
+ u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
if (!propdata) {
- kfree(propname);
power_supply_put_battery_info(psy, info);
err = -EINVAL;
goto out_put_node;
@@ -768,8 +767,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
err = fwnode_property_read_u32_array(fwnode, propname, propdata, proplen);
if (err < 0) {
dev_err(&psy->dev, "failed to get %s\n", propname);
- kfree(propname);
- kfree(propdata);
power_supply_put_battery_info(psy, info);
goto out_put_node;
}
@@ -780,7 +777,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
info->ocv_table[index] = table =
devm_kcalloc(&psy->dev, tab_len, sizeof(*table), GFP_KERNEL);
if (!info->ocv_table[index]) {
- kfree(propdata);
power_supply_put_battery_info(psy, info);
err = -ENOMEM;
goto out_put_node;
@@ -790,8 +786,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
table[i].ocv = propdata[i*2];
table[i].capacity = propdata[i*2+1];
}
-
- kfree(propdata);
}
proplen = fwnode_property_count_u32(fwnode, "resistance-temp-table");
@@ -804,7 +798,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
goto out_put_node;
}
- propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
+ u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
if (!propdata) {
power_supply_put_battery_info(psy, info);
err = -ENOMEM;
@@ -814,7 +808,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
propdata, proplen);
if (err < 0) {
- kfree(propdata);
power_supply_put_battery_info(psy, info);
goto out_put_node;
}
@@ -825,7 +818,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
sizeof(*resist_table),
GFP_KERNEL);
if (!info->resist_table) {
- kfree(propdata);
power_supply_put_battery_info(psy, info);
err = -ENOMEM;
goto out_put_node;
@@ -836,8 +828,6 @@ int power_supply_get_battery_info(struct power_supply *psy,
resist_table[index].resistance = propdata[index*2+1];
}
- kfree(propdata);
-
out_ret_pointer:
/* Finally return the whole thing */
*info_out = info;
--
2.49.0
next prev parent reply other threads:[~2025-06-08 19:57 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-29 22:54 [PATCH v2 0/5] power: supply: core: convert " Sebastian Reichel
2025-04-29 22:54 ` [PATCH v2 1/5] regulator: act8865-regulator: switch psy_cfg from of_node " Sebastian Reichel
2025-06-08 19:25 ` Hans de Goede
2025-04-29 22:54 ` [PATCH v2 2/5] power: supply: core: remove of_node from power_supply_config Sebastian Reichel
2025-06-08 19:26 ` Hans de Goede
2025-04-29 22:54 ` [PATCH v2 3/5] power: supply: core: battery-info: fully switch to fwnode Sebastian Reichel
2025-06-08 19:57 ` Hans de Goede [this message]
2025-06-08 20:16 ` Hans de Goede
2025-04-29 22:54 ` [PATCH v2 4/5] power: supply: core: convert to fwnnode Sebastian Reichel
2025-06-08 20:01 ` Hans de Goede
2025-04-29 22:54 ` [PATCH v2 5/5] power: supply: core: rename power_supply_get_by_phandle to power_supply_get_by_reference Sebastian Reichel
2025-06-08 20:25 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=68792092-4689-4e3e-8dbb-9157889f1a3d@kernel.org \
--to=hansg@kernel.org \
--cc=angelogioacchino.delregno@collabora.com \
--cc=broonie@kernel.org \
--cc=jernej.skrabec@gmail.com \
--cc=kishon@kernel.org \
--cc=krzk@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=pali@kernel.org \
--cc=samuel@sholland.org \
--cc=sebastian.reichel@collabora.com \
--cc=sre@kernel.org \
--cc=vkoul@kernel.org \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®