| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eb265cb2-b079-4bca-bc35-17a9f4d0ec3e@t-8ch.de>
Date: Wed, 18 Dec 2024 21:29:31 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Nathan Chancellor <nathan@...nel.org>,
Sebastian Reichel <sre@...nel.org>
Cc: Armin Wolf <W_Armin@....de>, Hans de Goede <hdegoede@...hat.com>,
Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>,
linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev, linux-pm@...r.kernel.org
Subject: Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover
currently used extensions
Hi Nathan,
On 2024-12-18 12:52:29-0700, Nathan Chancellor wrote:
> On Wed, Dec 11, 2024 at 08:57:58PM +0100, Thomas Weißschuh wrote:
> > Userspace wants to now about the used power supply extensions,
> > for example to handle a device extended by a certain extension
> > differently or to discover information about the extending device.
> >
> > Add a sysfs directory to the power supply device.
> > This directory contains links which are named after the used extension
> > and point to the device implementing that extension.
> >
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > ---
> > Documentation/ABI/testing/sysfs-class-power | 9 +++++++++
> > drivers/power/supply/cros_charge-control.c | 5 ++++-
> > drivers/power/supply/power_supply.h | 2 ++
> > drivers/power/supply/power_supply_core.c | 19 +++++++++++++++++--
> > drivers/power/supply/power_supply_sysfs.c | 10 ++++++++++
> > drivers/power/supply/test_power.c | 4 +++-
> > include/linux/power_supply.h | 2 ++
> > 7 files changed, 47 insertions(+), 4 deletions(-)
> ...
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 531785516d2ac31f9a7f73a58e15e64cb81820ed..9ed749cd09369f0f13017847687509736b30aae8 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -39,6 +40,7 @@ struct power_supply_ext_registration {
> >
> > extern void __init power_supply_init_attrs(void);
> > extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> > +extern const struct attribute_group power_supply_extension_group;
> > extern const struct attribute_group *power_supply_attr_groups[];
> >
> > #else
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index bc22edbd0e6a02c27500132075f5c98d814a7330..5142fbd580ee3d629a2aae7d0b9bcd5709162129 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
> > }
> >
> > int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> > - void *data)
> > + struct device *dev, void *data)
> > {
> > struct power_supply_ext_registration *reg;
> > size_t i;
> > int ret;
> >
> > - if (!psy || !ext || !ext->properties || !ext->num_properties)
> > + if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
> > return -EINVAL;
> >
> > guard(rwsem_write)(&psy->extensions_sem);
> >
> > + power_supply_for_each_extension(reg, psy)
> > + if (strcmp(ext->name, reg->ext->name) == 0)
> > + return -EEXIST;
> > +
> > for (i = 0; i < ext->num_properties; i++)
> > if (power_supply_has_property(psy, ext->properties[i]))
> > return -EEXIST;
> > @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
> > return -ENOMEM;
> >
> > reg->ext = ext;
> > + reg->dev = dev;
> > reg->data = data;
> > list_add(®->list_head, &psy->extensions);
> >
> > + ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> > + &dev->kobj, ext->name);
> > + if (ret)
> > + goto sysfs_link_failed;
> > +
> > ret = power_supply_update_sysfs_and_hwmon(psy);
> > if (ret)
> > goto sysfs_hwmon_failed;
> > @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
> > return 0;
> >
> > sysfs_hwmon_failed:
> > + sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> > +sysfs_link_failed:
> > list_del(®->list_head);
> > kfree(reg);
> > return ret;
> > @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
> > if (reg->ext == ext) {
> > list_del(®->list_head);
> > kfree(reg);
> > + sysfs_remove_link_from_group(&psy->dev.kobj,
> > + power_supply_extension_group.name,
> > + reg->ext->name);
> > power_supply_update_sysfs_and_hwmon(psy);
> > return;
> > }
>
> I am seeing a build failure in certain configurations because
> power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
> but this code can be built without CONFIG_SYSFS.
Thanks for the report.
> $ echo 'CONFIG_EXPERT=y
> CONFIG_SYSFS=n' >allno.config
>
> $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
> drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
> drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> 1389 | ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | power_supply_attr_groups
> drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
> drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
> drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> 1419 | power_supply_extension_group.name,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | power_supply_attr_groups
The reproducer doesn't actually enable CONFIG_POWER_SUPPLY, when I use it
I get a whole array of errors.
> Should the declaration be moved out from the ifdef or is there some
> other solution I am not seeing?
This, inline constants or a #define.
Sebastian, do you want me to send a patch?
> Cheers,
> Nathan
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 9ed749cd0936..6fc9939145fc 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -40,7 +40,6 @@ struct power_supply_ext_registration {
>
> extern void __init power_supply_init_attrs(void);
> extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> -extern const struct attribute_group power_supply_extension_group;
> extern const struct attribute_group *power_supply_attr_groups[];
>
> #else
> @@ -51,6 +50,8 @@ static inline void power_supply_init_attrs(void) {}
>
> #endif /* CONFIG_SYSFS */
>
> +extern const struct attribute_group power_supply_extension_group;
> +
> #ifdef CONFIG_LEDS_TRIGGERS
>
> extern void power_supply_update_leds(struct power_supply *psy);
Powered by blists - more mailing lists