Warning since "power: supply: add charge_behaviour attributes"

Warning since "power: supply: add charge_behaviour attributes"

[Heiner Kallweit]

Since 1b0b6cc8030d ("power: supply: add charge_behaviour attributes") I get the following warning:
power_supply_init_attrs: Property 37 skipped because it is missing from power_supply_attrs
Seems the patch misses the following:

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 2cfce2b2e..ef5109102 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -179,6 +179,7 @@ static struct power_supply_attr power_supply_attrs[] = {
        POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
        POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
        POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
+       POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
        POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
        POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
        POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),

Didn't this show up when testing before submitting the patch?

[PATCH 1/2] power: supply: fix charge_behaviour attribute initialization

[Thomas Weißschuh]

All properties have to be added to power_supply_attrs which was missed
before.

Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 5e3b8c15ddbe..491ffec7bf47 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -178,6 +178,7 @@ static struct power_supply_attr power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
 	POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
 	POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
+	POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
 	POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
 	POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
 	POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),

base-commit: 998e7ea8c641fc6bbca1acd478c6824733ac9851
-- 
2.34.1

[PATCH 2/2] power: supply: validate size of power_supply_attrs at compiletime

[Thomas Weißschuh]

For each member of enum power_supply_property a matching entry in
power_supply_attrs is needed.
Add a basic test at compiletime to validate this in addition to the
existing runtime testing.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 2 ++
 include/linux/power_supply.h              | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 491ffec7bf47..2565052a7a8c 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -403,6 +403,8 @@ void power_supply_init_attrs(struct device_type *dev_type)
 {
 	int i;
 
+	BUILD_BUG_ON(ARRAY_SIZE(power_supply_attrs) != __POWER_SUPPLY_PROP_CNT);
+
 	dev_type->groups = power_supply_attr_groups;
 
 	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 71f0379c2af8..60853f26e25f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -172,6 +172,8 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+
+	__POWER_SUPPLY_PROP_CNT
 };
 
 enum power_supply_type {
-- 
2.34.1

Re: [PATCH 2/2] power: supply: validate size of power_supply_attrs at compiletime

[Thomas Weißschuh]

Hi Sebastian, Hans,

On 2022-01-05 07:42+0100, Thomas Weißschuh wrote:
> For each member of enum power_supply_property a matching entry in
> power_supply_attrs is needed.
> Add a basic test at compiletime to validate this in addition to the
> existing runtime testing.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 2 ++
>  include/linux/power_supply.h              | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 491ffec7bf47..2565052a7a8c 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -403,6 +403,8 @@ void power_supply_init_attrs(struct device_type *dev_type)
>  {
>  	int i;
>  
> +	BUILD_BUG_ON(ARRAY_SIZE(power_supply_attrs) != __POWER_SUPPLY_PROP_CNT);
> +
>  	dev_type->groups = power_supply_attr_groups;
>  
>  	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 71f0379c2af8..60853f26e25f 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -172,6 +172,8 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_MODEL_NAME,
>  	POWER_SUPPLY_PROP_MANUFACTURER,
>  	POWER_SUPPLY_PROP_SERIAL_NUMBER,
> +
> +	__POWER_SUPPLY_PROP_CNT
>  };
>  
>  enum power_supply_type {
> -- 
> 2.34.1
> 

Please ignore this patch. It does not do what is tries to do. Sorry for the
noise.

Thomas

Re: Warning since "power: supply: add charge_behaviour attributes"

[Hans de Goede]

Hi Heiner,

On 1/5/22 00:03, Heiner Kallweit wrote:
> Since 1b0b6cc8030d ("power: supply: add charge_behaviour attributes") I get the following warning:
> power_supply_init_attrs: Property 37 skipped because it is missing from power_supply_attrs
> Seems the patch misses the following:
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 2cfce2b2e..ef5109102 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -179,6 +179,7 @@ static struct power_supply_attr power_supply_attrs[] = {
>         POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
>         POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
>         POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> +       POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
>         POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
>         POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
>         POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> 
> Didn't this show up when testing before submitting the patch?

Thank you for reporting this.

The warning did show up in dmesg, but the attribute still works despite the warning
because it is injected as an "extra" property through the drivers/acpi/battery.c
battery_hook_register() mechanism which uses device_add_groups() to add extra
properties handled by vendor specific extensions.

These extra properties do not use the drivers/power/supply/power_supply_sysfs.c
registered attributed handlers and this caused us to miss this, since everything
still works fine for the current user of the new charge_behaviour attribute
despite the warning.

And we completely missed the warning in dmesg.

Regards,

Hans

Re: [PATCH 1/2] power: supply: fix charge_behaviour attribute initialization

[Hans de Goede]

Hi,

On 1/5/22 07:42, Thomas Weißschuh wrote:
> All properties have to be added to power_supply_attrs which was missed
> before.
> 
> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thank you for the quick fix.

Since the base commit for this is from pdx86/for-next I'm merging this
fix through the pdx86 tree too:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
>  drivers/power/supply/power_supply_sysfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 5e3b8c15ddbe..491ffec7bf47 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -178,6 +178,7 @@ static struct power_supply_attr power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
>  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
>  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> +	POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
>  	POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
>  	POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
>  	POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> 
> base-commit: 998e7ea8c641fc6bbca1acd478c6824733ac9851
>