* [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
@ 2025-06-27 20:51 Armin Wolf
2025-06-27 20:51 ` [PATCH 2/3] power: supply: test-power: Test access to extended power supply Armin Wolf
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Armin Wolf @ 2025-06-27 20:51 UTC (permalink / raw)
To: sre, hdegoede, ilpo.jarvinen; +Cc: linux-pm, platform-driver-x86, linux-kernel
Power supply extensions might want to interact with the underlying
power supply to retrieve data like serial numbers, charging status
and more. However doing so causes psy->extensions_sem to be locked
twice, possibly causing a deadlock.
Provide special variants of power_supply_get/set_property() that
ignore any power supply extensions and thus do not touch the
associated psy->extensions_sem lock.
Suggested-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
include/linux/power_supply.h | 8 +++
2 files changed, 78 insertions(+), 12 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index aedb20c1d276..e70ffedf1a80 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
return false;
}
-int power_supply_get_property(struct power_supply *psy,
- enum power_supply_property psp,
- union power_supply_propval *val)
+static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val, bool use_extensions)
{
struct power_supply_ext_registration *reg;
@@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- scoped_guard(rwsem_read, &psy->extensions_sem) {
- power_supply_for_each_extension(reg, psy) {
- if (power_supply_ext_has_property(reg->ext, psp))
+ if (use_extensions) {
+ scoped_guard(rwsem_read, &psy->extensions_sem) {
+ power_supply_for_each_extension(reg, psy) {
+ if (!power_supply_ext_has_property(reg->ext, psp))
+ continue;
+
return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
+ }
}
}
@@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
else
return -EINVAL;
}
+
+int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ return __power_supply_get_property(psy, psp, val, true);
+}
EXPORT_SYMBOL_GPL(power_supply_get_property);
-int power_supply_set_property(struct power_supply *psy,
- enum power_supply_property psp,
- const union power_supply_propval *val)
+/**
+ * power_supply_get_property_direct - Read a power supply property without checking for extensions
+ * @psy: The power supply
+ * @psp: The power supply property to read
+ * @val: The resulting value of the power supply property
+ *
+ * Read a power supply property without taking into account any power supply extensions registered
+ * on the given power supply. This is mostly useful for power supply extensions that want to access
+ * their own power supply as using power_supply_get_property() directly will result in a potential
+ * deadlock.
+ *
+ * Return: 0 on success or negative error code on failure.
+ */
+int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ return __power_supply_get_property(psy, psp, val, false);
+}
+EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
+
+
+static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val, bool use_extensions)
{
struct power_supply_ext_registration *reg;
if (atomic_read(&psy->use_cnt) <= 0)
return -ENODEV;
- scoped_guard(rwsem_read, &psy->extensions_sem) {
- power_supply_for_each_extension(reg, psy) {
- if (power_supply_ext_has_property(reg->ext, psp)) {
+ if (use_extensions) {
+ scoped_guard(rwsem_read, &psy->extensions_sem) {
+ power_supply_for_each_extension(reg, psy) {
+ if (!power_supply_ext_has_property(reg->ext, psp))
+ continue;
+
if (reg->ext->set_property)
return reg->ext->set_property(psy, reg->ext, reg->data,
psp, val);
@@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
return psy->desc->set_property(psy, psp, val);
}
+
+int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ return __power_supply_set_property(psy, psp, val, true);
+}
EXPORT_SYMBOL_GPL(power_supply_set_property);
+/**
+ * power_supply_set_property_direct - Write a power supply property without checking for extensions
+ * @psy: The power supply
+ * @psp: The power supply property to write
+ * @val: The value to write to the power supply property
+ *
+ * Write a power supply property without taking into account any power supply extensions registered
+ * on the given power supply. This is mostly useful for power supply extensions that want to access
+ * their own power supply as using power_supply_set_property() directly will result in a potential
+ * deadlock.
+ *
+ * Return: 0 on success or negative error code on failure.
+ */
+int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ return __power_supply_set_property(psy, psp, val, false);
+}
+EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
+
int power_supply_property_is_writeable(struct power_supply *psy,
enum power_supply_property psp)
{
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 45468959dd98..f21f806bfb38 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
extern int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val);
+int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val);
#if IS_ENABLED(CONFIG_POWER_SUPPLY)
extern int power_supply_set_property(struct power_supply *psy,
enum power_supply_property psp,
const union power_supply_propval *val);
+int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val);
#else
static inline int power_supply_set_property(struct power_supply *psy,
enum power_supply_property psp,
const union power_supply_propval *val)
{ return 0; }
+static inline int power_supply_set_property_direct(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{ return 0; }
#endif
extern void power_supply_external_power_changed(struct power_supply *psy);
--
2.39.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/3] power: supply: test-power: Test access to extended power supply
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
@ 2025-06-27 20:51 ` Armin Wolf
2025-07-06 23:13 ` Sebastian Reichel
2025-06-27 20:51 ` [PATCH 3/3] platform/x86: dell-ddv: Fix taking the psy->extensions_sem lock twice Armin Wolf
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Armin Wolf @ 2025-06-27 20:51 UTC (permalink / raw)
To: sre, hdegoede, ilpo.jarvinen; +Cc: linux-pm, platform-driver-x86, linux-kernel
Test that power supply extensions can access properties of their
power supply using power_supply_get_property_direct(). This both
ensures that the functionality works and serves as an example for
future driver developers.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/power/supply/test_power.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 5bfdfcf6013b..2c0e9ad820c0 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -259,6 +259,7 @@ static const struct power_supply_config test_power_configs[] = {
static int test_power_battery_extmanufacture_year = 1234;
static int test_power_battery_exttemp_max = 1000;
static const enum power_supply_property test_power_battery_extprops[] = {
+ POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
POWER_SUPPLY_PROP_TEMP_MAX,
};
@@ -270,6 +271,9 @@ static int test_power_battery_extget_property(struct power_supply *psy,
union power_supply_propval *val)
{
switch (psp) {
+ case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
+ return power_supply_get_property_direct(psy, POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
+ val);
case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
val->intval = test_power_battery_extmanufacture_year;
break;
--
2.39.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] power: supply: test-power: Test access to extended power supply
2025-06-27 20:51 ` [PATCH 2/3] power: supply: test-power: Test access to extended power supply Armin Wolf
@ 2025-07-06 23:13 ` Sebastian Reichel
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Reichel @ 2025-07-06 23:13 UTC (permalink / raw)
To: Armin Wolf
Cc: hdegoede, ilpo.jarvinen, linux-pm, platform-driver-x86,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]
Hi,
On Fri, Jun 27, 2025 at 10:51:23PM +0200, Armin Wolf wrote:
> Test that power supply extensions can access properties of their
> power supply using power_supply_get_property_direct(). This both
> ensures that the functionality works and serves as an example for
> future driver developers.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> drivers/power/supply/test_power.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 5bfdfcf6013b..2c0e9ad820c0 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -259,6 +259,7 @@ static const struct power_supply_config test_power_configs[] = {
> static int test_power_battery_extmanufacture_year = 1234;
> static int test_power_battery_exttemp_max = 1000;
> static const enum power_supply_property test_power_battery_extprops[] = {
> + POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
> POWER_SUPPLY_PROP_TEMP_MAX,
> };
> @@ -270,6 +271,9 @@ static int test_power_battery_extget_property(struct power_supply *psy,
> union power_supply_propval *val)
> {
> switch (psp) {
> + case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> + return power_supply_get_property_direct(psy, POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
> + val);
> case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
> val->intval = test_power_battery_extmanufacture_year;
> break;
> --
> 2.39.5
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] platform/x86: dell-ddv: Fix taking the psy->extensions_sem lock twice
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
2025-06-27 20:51 ` [PATCH 2/3] power: supply: test-power: Test access to extended power supply Armin Wolf
@ 2025-06-27 20:51 ` Armin Wolf
2025-06-28 9:25 ` [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Hans de Goede
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Armin Wolf @ 2025-06-27 20:51 UTC (permalink / raw)
To: sre, hdegoede, ilpo.jarvinen; +Cc: linux-pm, platform-driver-x86, linux-kernel
Calling power_supply_get_property() inside
dell_wmi_ddv_battery_translate() can cause a deadlock since this
function is also being called from the power supply extension code,
in which case psy->extensions_sem is already being held.
Fix this by using the new power_supply_get_property_direct() function
to ignore any power supply extensions when retrieving the battery
serial number.
Tested on a Dell Inspiron 3505.
Reported-by: Hans de Goede <hansg@kernel.org>
Fixes: 058de163a376 ("platform/x86: dell-ddv: Implement the battery matching algorithm")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/dell/dell-wmi-ddv.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 67f3d7158403..62e3d060f038 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -689,9 +689,13 @@ static int dell_wmi_ddv_battery_translate(struct dell_wmi_ddv_data *data,
dev_dbg(&data->wdev->dev, "Translation cache miss\n");
- /* Perform a translation between a ACPI battery and a battery index */
-
- ret = power_supply_get_property(battery, POWER_SUPPLY_PROP_SERIAL_NUMBER, &val);
+ /*
+ * Perform a translation between a ACPI battery and a battery index.
+ * We have to use power_supply_get_property_direct() here because this
+ * function will also get called from the callbacks of the power supply
+ * extension.
+ */
+ ret = power_supply_get_property_direct(battery, POWER_SUPPLY_PROP_SERIAL_NUMBER, &val);
if (ret < 0)
return ret;
--
2.39.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
2025-06-27 20:51 ` [PATCH 2/3] power: supply: test-power: Test access to extended power supply Armin Wolf
2025-06-27 20:51 ` [PATCH 3/3] platform/x86: dell-ddv: Fix taking the psy->extensions_sem lock twice Armin Wolf
@ 2025-06-28 9:25 ` Hans de Goede
2025-06-28 21:34 ` Armin Wolf
2025-07-06 23:11 ` Sebastian Reichel
2025-07-07 13:00 ` Ilpo Järvinen
4 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2025-06-28 9:25 UTC (permalink / raw)
To: Armin Wolf, sre, hdegoede, ilpo.jarvinen
Cc: linux-pm, platform-driver-x86, linux-kernel
Hi Armin,
On 27-Jun-25 10:51 PM, Armin Wolf wrote:
> Power supply extensions might want to interact with the underlying
> power supply to retrieve data like serial numbers, charging status
> and more. However doing so causes psy->extensions_sem to be locked
> twice, possibly causing a deadlock.
>
> Provide special variants of power_supply_get/set_property() that
> ignore any power supply extensions and thus do not touch the
> associated psy->extensions_sem lock.
>
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Thank you for your work on this.
The entire series looks good to me:
Reviewed-by: Hans de Goede <hansg@kernel.org>
for the series.
There is the question of how to merge this. I think it might
be best for the entire series to go through the power-supply
tree.
Ilpo would that work for you and if yes can we have your ack ?
Sebastian, IMHO this should be merged as fixed not as for-next
material.
Regards,
Hans
> ---
> drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
> include/linux/power_supply.h | 8 +++
> 2 files changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index aedb20c1d276..e70ffedf1a80 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
> return false;
> }
>
> -int power_supply_get_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - union power_supply_propval *val)
> +static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> @@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
> return -ENODEV;
> }
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp))
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> + }
> }
> }
>
> @@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
> else
> return -EINVAL;
> }
> +
> +int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_get_property);
>
> -int power_supply_set_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - const union power_supply_propval *val)
> +/**
> + * power_supply_get_property_direct - Read a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to read
> + * @val: The resulting value of the power supply property
> + *
> + * Read a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_get_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
> +
> +
> +static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> if (atomic_read(&psy->use_cnt) <= 0)
> return -ENODEV;
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp)) {
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> if (reg->ext->set_property)
> return reg->ext->set_property(psy, reg->ext, reg->data,
> psp, val);
> @@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
>
> return psy->desc->set_property(psy, psp, val);
> }
> +
> +int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_set_property);
>
> +/**
> + * power_supply_set_property_direct - Write a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to write
> + * @val: The value to write to the power supply property
> + *
> + * Write a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_set_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
> +
> int power_supply_property_is_writeable(struct power_supply *psy,
> enum power_supply_property psp)
> {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 45468959dd98..f21f806bfb38 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
> extern int power_supply_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val);
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val);
> #if IS_ENABLED(CONFIG_POWER_SUPPLY)
> extern int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val);
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val);
> #else
> static inline int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val)
> { return 0; }
> +static inline int power_supply_set_property_direct(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{ return 0; }
> #endif
> extern void power_supply_external_power_changed(struct power_supply *psy);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-28 9:25 ` [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Hans de Goede
@ 2025-06-28 21:34 ` Armin Wolf
2025-06-30 8:58 ` Ilpo Järvinen
0 siblings, 1 reply; 12+ messages in thread
From: Armin Wolf @ 2025-06-28 21:34 UTC (permalink / raw)
To: Hans de Goede, sre, hdegoede, ilpo.jarvinen
Cc: linux-pm, platform-driver-x86, linux-kernel
Am 28.06.25 um 11:25 schrieb Hans de Goede:
> Hi Armin,
>
> On 27-Jun-25 10:51 PM, Armin Wolf wrote:
>> Power supply extensions might want to interact with the underlying
>> power supply to retrieve data like serial numbers, charging status
>> and more. However doing so causes psy->extensions_sem to be locked
>> twice, possibly causing a deadlock.
>>
>> Provide special variants of power_supply_get/set_property() that
>> ignore any power supply extensions and thus do not touch the
>> associated psy->extensions_sem lock.
>>
>> Suggested-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Thank you for your work on this.
>
> The entire series looks good to me:
>
> Reviewed-by: Hans de Goede <hansg@kernel.org>
>
> for the series.
>
> There is the question of how to merge this. I think it might
> be best for the entire series to go through the power-supply
> tree.
>
> Ilpo would that work for you and if yes can we have your ack ?
>
> Sebastian, IMHO this should be merged as fixed not as for-next
> material.
>
> Regards,
>
> Hans
Personally i would prefer to merge this through the pdx86 tree as the
uniwill-laptop driver currently under review will also require this functionality.
Thanks,
Armin Wolf
>> ---
>> drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
>> include/linux/power_supply.h | 8 +++
>> 2 files changed, 78 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index aedb20c1d276..e70ffedf1a80 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
>> return false;
>> }
>>
>> -int power_supply_get_property(struct power_supply *psy,
>> - enum power_supply_property psp,
>> - union power_supply_propval *val)
>> +static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
>> + union power_supply_propval *val, bool use_extensions)
>> {
>> struct power_supply_ext_registration *reg;
>>
>> @@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
>> return -ENODEV;
>> }
>>
>> - scoped_guard(rwsem_read, &psy->extensions_sem) {
>> - power_supply_for_each_extension(reg, psy) {
>> - if (power_supply_ext_has_property(reg->ext, psp))
>> + if (use_extensions) {
>> + scoped_guard(rwsem_read, &psy->extensions_sem) {
>> + power_supply_for_each_extension(reg, psy) {
>> + if (!power_supply_ext_has_property(reg->ext, psp))
>> + continue;
>> +
>> return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
>> + }
>> }
>> }
>>
>> @@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
>> else
>> return -EINVAL;
>> }
>> +
>> +int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
>> + union power_supply_propval *val)
>> +{
>> + return __power_supply_get_property(psy, psp, val, true);
>> +}
>> EXPORT_SYMBOL_GPL(power_supply_get_property);
>>
>> -int power_supply_set_property(struct power_supply *psy,
>> - enum power_supply_property psp,
>> - const union power_supply_propval *val)
>> +/**
>> + * power_supply_get_property_direct - Read a power supply property without checking for extensions
>> + * @psy: The power supply
>> + * @psp: The power supply property to read
>> + * @val: The resulting value of the power supply property
>> + *
>> + * Read a power supply property without taking into account any power supply extensions registered
>> + * on the given power supply. This is mostly useful for power supply extensions that want to access
>> + * their own power supply as using power_supply_get_property() directly will result in a potential
>> + * deadlock.
>> + *
>> + * Return: 0 on success or negative error code on failure.
>> + */
>> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
>> + union power_supply_propval *val)
>> +{
>> + return __power_supply_get_property(psy, psp, val, false);
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
>> +
>> +
>> +static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
>> + const union power_supply_propval *val, bool use_extensions)
>> {
>> struct power_supply_ext_registration *reg;
>>
>> if (atomic_read(&psy->use_cnt) <= 0)
>> return -ENODEV;
>>
>> - scoped_guard(rwsem_read, &psy->extensions_sem) {
>> - power_supply_for_each_extension(reg, psy) {
>> - if (power_supply_ext_has_property(reg->ext, psp)) {
>> + if (use_extensions) {
>> + scoped_guard(rwsem_read, &psy->extensions_sem) {
>> + power_supply_for_each_extension(reg, psy) {
>> + if (!power_supply_ext_has_property(reg->ext, psp))
>> + continue;
>> +
>> if (reg->ext->set_property)
>> return reg->ext->set_property(psy, reg->ext, reg->data,
>> psp, val);
>> @@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
>>
>> return psy->desc->set_property(psy, psp, val);
>> }
>> +
>> +int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{
>> + return __power_supply_set_property(psy, psp, val, true);
>> +}
>> EXPORT_SYMBOL_GPL(power_supply_set_property);
>>
>> +/**
>> + * power_supply_set_property_direct - Write a power supply property without checking for extensions
>> + * @psy: The power supply
>> + * @psp: The power supply property to write
>> + * @val: The value to write to the power supply property
>> + *
>> + * Write a power supply property without taking into account any power supply extensions registered
>> + * on the given power supply. This is mostly useful for power supply extensions that want to access
>> + * their own power supply as using power_supply_set_property() directly will result in a potential
>> + * deadlock.
>> + *
>> + * Return: 0 on success or negative error code on failure.
>> + */
>> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{
>> + return __power_supply_set_property(psy, psp, val, false);
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
>> +
>> int power_supply_property_is_writeable(struct power_supply *psy,
>> enum power_supply_property psp)
>> {
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index 45468959dd98..f21f806bfb38 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
>> extern int power_supply_get_property(struct power_supply *psy,
>> enum power_supply_property psp,
>> union power_supply_propval *val);
>> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
>> + union power_supply_propval *val);
>> #if IS_ENABLED(CONFIG_POWER_SUPPLY)
>> extern int power_supply_set_property(struct power_supply *psy,
>> enum power_supply_property psp,
>> const union power_supply_propval *val);
>> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
>> + const union power_supply_propval *val);
>> #else
>> static inline int power_supply_set_property(struct power_supply *psy,
>> enum power_supply_property psp,
>> const union power_supply_propval *val)
>> { return 0; }
>> +static inline int power_supply_set_property_direct(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{ return 0; }
>> #endif
>> extern void power_supply_external_power_changed(struct power_supply *psy);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-28 21:34 ` Armin Wolf
@ 2025-06-30 8:58 ` Ilpo Järvinen
2025-07-06 23:14 ` Sebastian Reichel
0 siblings, 1 reply; 12+ messages in thread
From: Ilpo Järvinen @ 2025-06-30 8:58 UTC (permalink / raw)
To: Armin Wolf, sre
Cc: Hans de Goede, Hans de Goede, linux-pm, platform-driver-x86, LKML
On Sat, 28 Jun 2025, Armin Wolf wrote:
> Am 28.06.25 um 11:25 schrieb Hans de Goede:
>
> > Hi Armin,
> >
> > On 27-Jun-25 10:51 PM, Armin Wolf wrote:
> > > Power supply extensions might want to interact with the underlying
> > > power supply to retrieve data like serial numbers, charging status
> > > and more. However doing so causes psy->extensions_sem to be locked
> > > twice, possibly causing a deadlock.
> > >
> > > Provide special variants of power_supply_get/set_property() that
> > > ignore any power supply extensions and thus do not touch the
> > > associated psy->extensions_sem lock.
> > >
> > > Suggested-by: Hans de Goede <hansg@kernel.org>
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > Thank you for your work on this.
> >
> > The entire series looks good to me:
> >
> > Reviewed-by: Hans de Goede <hansg@kernel.org>
> >
> > for the series.
> >
> > There is the question of how to merge this. I think it might
> > be best for the entire series to go through the power-supply
> > tree.
> >
> > Ilpo would that work for you and if yes can we have your ack ?
> >
> > Sebastian, IMHO this should be merged as fixed not as for-next
> > material.
> >
> > Regards,
> >
> > Hans
>
> Personally i would prefer to merge this through the pdx86 tree as the
> uniwill-laptop driver currently under review will also require this
> functionality.
Sebastian, are you okay if I take this through pdx86 fixes branch as
requested by Armin? If yes, can I have your ack please.
--
i.
> > > ---
> > > drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
> > > include/linux/power_supply.h | 8 +++
> > > 2 files changed, 78 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/power/supply/power_supply_core.c
> > > b/drivers/power/supply/power_supply_core.c
> > > index aedb20c1d276..e70ffedf1a80 100644
> > > --- a/drivers/power/supply/power_supply_core.c
> > > +++ b/drivers/power/supply/power_supply_core.c
> > > @@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply
> > > *psy,
> > > return false;
> > > }
> > > -int power_supply_get_property(struct power_supply *psy,
> > > - enum power_supply_property psp,
> > > - union power_supply_propval *val)
> > > +static int __power_supply_get_property(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + union power_supply_propval *val, bool
> > > use_extensions)
> > > {
> > > struct power_supply_ext_registration *reg;
> > > @@ -1253,10 +1252,14 @@ int power_supply_get_property(struct
> > > power_supply *psy,
> > > return -ENODEV;
> > > }
> > > - scoped_guard(rwsem_read, &psy->extensions_sem) {
> > > - power_supply_for_each_extension(reg, psy) {
> > > - if (power_supply_ext_has_property(reg->ext, psp))
> > > + if (use_extensions) {
> > > + scoped_guard(rwsem_read, &psy->extensions_sem) {
> > > + power_supply_for_each_extension(reg, psy) {
> > > + if (!power_supply_ext_has_property(reg->ext,
> > > psp))
> > > + continue;
> > > +
> > > return reg->ext->get_property(psy, reg->ext,
> > > reg->data, psp, val);
> > > + }
> > > }
> > > }
> > > @@ -1267,20 +1270,49 @@ int power_supply_get_property(struct
> > > power_supply *psy,
> > > else
> > > return -EINVAL;
> > > }
> > > +
> > > +int power_supply_get_property(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + union power_supply_propval *val)
> > > +{
> > > + return __power_supply_get_property(psy, psp, val, true);
> > > +}
> > > EXPORT_SYMBOL_GPL(power_supply_get_property);
> > > -int power_supply_set_property(struct power_supply *psy,
> > > - enum power_supply_property psp,
> > > - const union power_supply_propval *val)
> > > +/**
> > > + * power_supply_get_property_direct - Read a power supply property
> > > without checking for extensions
> > > + * @psy: The power supply
> > > + * @psp: The power supply property to read
> > > + * @val: The resulting value of the power supply property
> > > + *
> > > + * Read a power supply property without taking into account any power
> > > supply extensions registered
> > > + * on the given power supply. This is mostly useful for power supply
> > > extensions that want to access
> > > + * their own power supply as using power_supply_get_property() directly
> > > will result in a potential
> > > + * deadlock.
> > > + *
> > > + * Return: 0 on success or negative error code on failure.
> > > + */
> > > +int power_supply_get_property_direct(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + union power_supply_propval *val)
> > > +{
> > > + return __power_supply_get_property(psy, psp, val, false);
> > > +}
> > > +EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
> > > +
> > > +
> > > +static int __power_supply_set_property(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + const union power_supply_propval *val,
> > > bool use_extensions)
> > > {
> > > struct power_supply_ext_registration *reg;
> > > if (atomic_read(&psy->use_cnt) <= 0)
> > > return -ENODEV;
> > > - scoped_guard(rwsem_read, &psy->extensions_sem) {
> > > - power_supply_for_each_extension(reg, psy) {
> > > - if (power_supply_ext_has_property(reg->ext, psp)) {
> > > + if (use_extensions) {
> > > + scoped_guard(rwsem_read, &psy->extensions_sem) {
> > > + power_supply_for_each_extension(reg, psy) {
> > > + if (!power_supply_ext_has_property(reg->ext,
> > > psp))
> > > + continue;
> > > +
> > > if (reg->ext->set_property)
> > > return reg->ext->set_property(psy,
> > > reg->ext, reg->data,
> > > psp,
> > > val);
> > > @@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply
> > > *psy,
> > > return psy->desc->set_property(psy, psp, val);
> > > }
> > > +
> > > +int power_supply_set_property(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + const union power_supply_propval *val)
> > > +{
> > > + return __power_supply_set_property(psy, psp, val, true);
> > > +}
> > > EXPORT_SYMBOL_GPL(power_supply_set_property);
> > > +/**
> > > + * power_supply_set_property_direct - Write a power supply property
> > > without checking for extensions
> > > + * @psy: The power supply
> > > + * @psp: The power supply property to write
> > > + * @val: The value to write to the power supply property
> > > + *
> > > + * Write a power supply property without taking into account any power
> > > supply extensions registered
> > > + * on the given power supply. This is mostly useful for power supply
> > > extensions that want to access
> > > + * their own power supply as using power_supply_set_property() directly
> > > will result in a potential
> > > + * deadlock.
> > > + *
> > > + * Return: 0 on success or negative error code on failure.
> > > + */
> > > +int power_supply_set_property_direct(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + const union power_supply_propval *val)
> > > +{
> > > + return __power_supply_set_property(psy, psp, val, false);
> > > +}
> > > +EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
> > > +
> > > int power_supply_property_is_writeable(struct power_supply *psy,
> > > enum power_supply_property psp)
> > > {
> > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > > index 45468959dd98..f21f806bfb38 100644
> > > --- a/include/linux/power_supply.h
> > > +++ b/include/linux/power_supply.h
> > > @@ -878,15 +878,23 @@ static inline int
> > > power_supply_is_system_supplied(void) { return -ENOSYS; }
> > > extern int power_supply_get_property(struct power_supply *psy,
> > > enum power_supply_property psp,
> > > union power_supply_propval *val);
> > > +int power_supply_get_property_direct(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + union power_supply_propval *val);
> > > #if IS_ENABLED(CONFIG_POWER_SUPPLY)
> > > extern int power_supply_set_property(struct power_supply *psy,
> > > enum power_supply_property psp,
> > > const union power_supply_propval *val);
> > > +int power_supply_set_property_direct(struct power_supply *psy, enum
> > > power_supply_property psp,
> > > + const union power_supply_propval *val);
> > > #else
> > > static inline int power_supply_set_property(struct power_supply *psy,
> > > enum power_supply_property psp,
> > > const union power_supply_propval *val)
> > > { return 0; }
> > > +static inline int power_supply_set_property_direct(struct power_supply
> > > *psy,
> > > + enum power_supply_property
> > > psp,
> > > + const union
> > > power_supply_propval *val)
> > > +{ return 0; }
> > > #endif
> > > extern void power_supply_external_power_changed(struct power_supply
> > > *psy);
> > >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-30 8:58 ` Ilpo Järvinen
@ 2025-07-06 23:14 ` Sebastian Reichel
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Reichel @ 2025-07-06 23:14 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Armin Wolf, Hans de Goede, Hans de Goede, linux-pm,
platform-driver-x86, LKML
[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]
Hi,
On Mon, Jun 30, 2025 at 11:58:39AM +0300, Ilpo Järvinen wrote:
> On Sat, 28 Jun 2025, Armin Wolf wrote:
>
> > Am 28.06.25 um 11:25 schrieb Hans de Goede:
> >
> > > Hi Armin,
> > >
> > > On 27-Jun-25 10:51 PM, Armin Wolf wrote:
> > > > Power supply extensions might want to interact with the underlying
> > > > power supply to retrieve data like serial numbers, charging status
> > > > and more. However doing so causes psy->extensions_sem to be locked
> > > > twice, possibly causing a deadlock.
> > > >
> > > > Provide special variants of power_supply_get/set_property() that
> > > > ignore any power supply extensions and thus do not touch the
> > > > associated psy->extensions_sem lock.
> > > >
> > > > Suggested-by: Hans de Goede <hansg@kernel.org>
> > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > Thank you for your work on this.
> > >
> > > The entire series looks good to me:
> > >
> > > Reviewed-by: Hans de Goede <hansg@kernel.org>
> > >
> > > for the series.
> > >
> > > There is the question of how to merge this. I think it might
> > > be best for the entire series to go through the power-supply
> > > tree.
> > >
> > > Ilpo would that work for you and if yes can we have your ack ?
> > >
> > > Sebastian, IMHO this should be merged as fixed not as for-next
> > > material.
> > >
> > > Regards,
> > >
> > > Hans
> >
> > Personally i would prefer to merge this through the pdx86 tree as the
> > uniwill-laptop driver currently under review will also require this
> > functionality.
>
> Sebastian, are you okay if I take this through pdx86 fixes branch as
> requested by Armin? If yes, can I have your ack please.
Sorry, took me a bit to figure out if that works, since the core is
also patched in my next branch. But I think git should figure
everything out on its own. Feel free to merge the whole series.
Greetings,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
` (2 preceding siblings ...)
2025-06-28 9:25 ` [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Hans de Goede
@ 2025-07-06 23:11 ` Sebastian Reichel
2025-07-07 13:00 ` Ilpo Järvinen
4 siblings, 0 replies; 12+ messages in thread
From: Sebastian Reichel @ 2025-07-06 23:11 UTC (permalink / raw)
To: Armin Wolf
Cc: hdegoede, ilpo.jarvinen, linux-pm, platform-driver-x86,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 7463 bytes --]
Hi,
On Fri, Jun 27, 2025 at 10:51:22PM +0200, Armin Wolf wrote:
> Power supply extensions might want to interact with the underlying
> power supply to retrieve data like serial numbers, charging status
> and more. However doing so causes psy->extensions_sem to be locked
> twice, possibly causing a deadlock.
>
> Provide special variants of power_supply_get/set_property() that
> ignore any power supply extensions and thus do not touch the
> associated psy->extensions_sem lock.
>
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
> include/linux/power_supply.h | 8 +++
> 2 files changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index aedb20c1d276..e70ffedf1a80 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
> return false;
> }
>
> -int power_supply_get_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - union power_supply_propval *val)
> +static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> @@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
> return -ENODEV;
> }
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp))
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> + }
> }
> }
>
> @@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
> else
> return -EINVAL;
> }
> +
> +int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_get_property);
>
> -int power_supply_set_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - const union power_supply_propval *val)
> +/**
> + * power_supply_get_property_direct - Read a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to read
> + * @val: The resulting value of the power supply property
> + *
> + * Read a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_get_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
> +
> +
> +static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> if (atomic_read(&psy->use_cnt) <= 0)
> return -ENODEV;
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp)) {
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> if (reg->ext->set_property)
> return reg->ext->set_property(psy, reg->ext, reg->data,
> psp, val);
> @@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
>
> return psy->desc->set_property(psy, psp, val);
> }
> +
> +int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_set_property);
>
> +/**
> + * power_supply_set_property_direct - Write a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to write
> + * @val: The value to write to the power supply property
> + *
> + * Write a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_set_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
> +
> int power_supply_property_is_writeable(struct power_supply *psy,
> enum power_supply_property psp)
> {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 45468959dd98..f21f806bfb38 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
> extern int power_supply_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val);
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val);
> #if IS_ENABLED(CONFIG_POWER_SUPPLY)
> extern int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val);
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val);
> #else
> static inline int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val)
> { return 0; }
> +static inline int power_supply_set_property_direct(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{ return 0; }
> #endif
> extern void power_supply_external_power_changed(struct power_supply *psy);
>
> --
> 2.39.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
` (3 preceding siblings ...)
2025-07-06 23:11 ` Sebastian Reichel
@ 2025-07-07 13:00 ` Ilpo Järvinen
4 siblings, 0 replies; 12+ messages in thread
From: Ilpo Järvinen @ 2025-07-07 13:00 UTC (permalink / raw)
To: sre, Hans de Goede, Armin Wolf
Cc: linux-pm, platform-driver-x86, linux-kernel
On Fri, 27 Jun 2025 22:51:22 +0200, Armin Wolf wrote:
> Power supply extensions might want to interact with the underlying
> power supply to retrieve data like serial numbers, charging status
> and more. However doing so causes psy->extensions_sem to be locked
> twice, possibly causing a deadlock.
>
> Provide special variants of power_supply_get/set_property() that
> ignore any power supply extensions and thus do not touch the
> associated psy->extensions_sem lock.
>
> [...]
Thank you for your contribution, it has been applied to my local
review-ilpo-fixes branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-fixes branch only once I've pushed my
local branch there, which might take a while.
The list of commits applied:
[1/3] power: supply: core: Add power_supply_get/set_property_direct()
commit: 3ebed2fddf6fac5729ffc8c471c87d111b641678
[2/3] power: supply: test-power: Test access to extended power supply
commit: a5f354232118751fe43be6ac896f8d6e7d7418b5
[3/3] platform/x86: dell-ddv: Fix taking the psy->extensions_sem lock twice
commit: d4e83784b2a9be58b938d55efb232d2751c4cab4
--
i.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
@ 2025-06-27 20:53 Armin Wolf
2025-06-27 20:55 ` Armin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: Armin Wolf @ 2025-06-27 20:53 UTC (permalink / raw)
To: sre, hdegoede, ilpo.jarvinen
Cc: linux-pm, platform-driver-x86, linux-kernel, linux
Power supply extensions might want to interact with the underlying
power supply to retrieve data like serial numbers, charging status
and more. However doing so causes psy->extensions_sem to be locked
twice, possibly causing a deadlock.
Provide special variants of power_supply_get/set_property() that
ignore any power supply extensions and thus do not touch the
associated psy->extensions_sem lock.
Suggested-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
include/linux/power_supply.h | 8 +++
2 files changed, 78 insertions(+), 12 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index aedb20c1d276..e70ffedf1a80 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
return false;
}
-int power_supply_get_property(struct power_supply *psy,
- enum power_supply_property psp,
- union power_supply_propval *val)
+static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val, bool use_extensions)
{
struct power_supply_ext_registration *reg;
@@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- scoped_guard(rwsem_read, &psy->extensions_sem) {
- power_supply_for_each_extension(reg, psy) {
- if (power_supply_ext_has_property(reg->ext, psp))
+ if (use_extensions) {
+ scoped_guard(rwsem_read, &psy->extensions_sem) {
+ power_supply_for_each_extension(reg, psy) {
+ if (!power_supply_ext_has_property(reg->ext, psp))
+ continue;
+
return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
+ }
}
}
@@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
else
return -EINVAL;
}
+
+int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ return __power_supply_get_property(psy, psp, val, true);
+}
EXPORT_SYMBOL_GPL(power_supply_get_property);
-int power_supply_set_property(struct power_supply *psy,
- enum power_supply_property psp,
- const union power_supply_propval *val)
+/**
+ * power_supply_get_property_direct - Read a power supply property without checking for extensions
+ * @psy: The power supply
+ * @psp: The power supply property to read
+ * @val: The resulting value of the power supply property
+ *
+ * Read a power supply property without taking into account any power supply extensions registered
+ * on the given power supply. This is mostly useful for power supply extensions that want to access
+ * their own power supply as using power_supply_get_property() directly will result in a potential
+ * deadlock.
+ *
+ * Return: 0 on success or negative error code on failure.
+ */
+int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ return __power_supply_get_property(psy, psp, val, false);
+}
+EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
+
+
+static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val, bool use_extensions)
{
struct power_supply_ext_registration *reg;
if (atomic_read(&psy->use_cnt) <= 0)
return -ENODEV;
- scoped_guard(rwsem_read, &psy->extensions_sem) {
- power_supply_for_each_extension(reg, psy) {
- if (power_supply_ext_has_property(reg->ext, psp)) {
+ if (use_extensions) {
+ scoped_guard(rwsem_read, &psy->extensions_sem) {
+ power_supply_for_each_extension(reg, psy) {
+ if (!power_supply_ext_has_property(reg->ext, psp))
+ continue;
+
if (reg->ext->set_property)
return reg->ext->set_property(psy, reg->ext, reg->data,
psp, val);
@@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
return psy->desc->set_property(psy, psp, val);
}
+
+int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ return __power_supply_set_property(psy, psp, val, true);
+}
EXPORT_SYMBOL_GPL(power_supply_set_property);
+/**
+ * power_supply_set_property_direct - Write a power supply property without checking for extensions
+ * @psy: The power supply
+ * @psp: The power supply property to write
+ * @val: The value to write to the power supply property
+ *
+ * Write a power supply property without taking into account any power supply extensions registered
+ * on the given power supply. This is mostly useful for power supply extensions that want to access
+ * their own power supply as using power_supply_set_property() directly will result in a potential
+ * deadlock.
+ *
+ * Return: 0 on success or negative error code on failure.
+ */
+int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ return __power_supply_set_property(psy, psp, val, false);
+}
+EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
+
int power_supply_property_is_writeable(struct power_supply *psy,
enum power_supply_property psp)
{
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 45468959dd98..f21f806bfb38 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
extern int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val);
+int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val);
#if IS_ENABLED(CONFIG_POWER_SUPPLY)
extern int power_supply_set_property(struct power_supply *psy,
enum power_supply_property psp,
const union power_supply_propval *val);
+int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val);
#else
static inline int power_supply_set_property(struct power_supply *psy,
enum power_supply_property psp,
const union power_supply_propval *val)
{ return 0; }
+static inline int power_supply_set_property_direct(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{ return 0; }
#endif
extern void power_supply_external_power_changed(struct power_supply *psy);
--
2.39.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct()
2025-06-27 20:53 Armin Wolf
@ 2025-06-27 20:55 ` Armin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Armin Wolf @ 2025-06-27 20:55 UTC (permalink / raw)
To: sre, hdegoede, ilpo.jarvinen
Cc: linux-pm, platform-driver-x86, linux-kernel, linux
Am 27.06.25 um 22:53 schrieb Armin Wolf:
> Power supply extensions might want to interact with the underlying
> power supply to retrieve data like serial numbers, charging status
> and more. However doing so causes psy->extensions_sem to be locked
> twice, possibly causing a deadlock.
>
> Provide special variants of power_supply_get/set_property() that
> ignore any power supply extensions and thus do not touch the
> associated psy->extensions_sem lock.
Sorry for sending the same patch series twice, it seems that git ignored my request
to append "RESEND" to the title of each patch :(.
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> drivers/power/supply/power_supply_core.c | 82 ++++++++++++++++++++----
> include/linux/power_supply.h | 8 +++
> 2 files changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index aedb20c1d276..e70ffedf1a80 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1241,9 +1241,8 @@ bool power_supply_has_property(struct power_supply *psy,
> return false;
> }
>
> -int power_supply_get_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - union power_supply_propval *val)
> +static int __power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> @@ -1253,10 +1252,14 @@ int power_supply_get_property(struct power_supply *psy,
> return -ENODEV;
> }
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp))
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> + }
> }
> }
>
> @@ -1267,20 +1270,49 @@ int power_supply_get_property(struct power_supply *psy,
> else
> return -EINVAL;
> }
> +
> +int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_get_property);
>
> -int power_supply_set_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - const union power_supply_propval *val)
> +/**
> + * power_supply_get_property_direct - Read a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to read
> + * @val: The resulting value of the power supply property
> + *
> + * Read a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_get_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + return __power_supply_get_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_property_direct);
> +
> +
> +static int __power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val, bool use_extensions)
> {
> struct power_supply_ext_registration *reg;
>
> if (atomic_read(&psy->use_cnt) <= 0)
> return -ENODEV;
>
> - scoped_guard(rwsem_read, &psy->extensions_sem) {
> - power_supply_for_each_extension(reg, psy) {
> - if (power_supply_ext_has_property(reg->ext, psp)) {
> + if (use_extensions) {
> + scoped_guard(rwsem_read, &psy->extensions_sem) {
> + power_supply_for_each_extension(reg, psy) {
> + if (!power_supply_ext_has_property(reg->ext, psp))
> + continue;
> +
> if (reg->ext->set_property)
> return reg->ext->set_property(psy, reg->ext, reg->data,
> psp, val);
> @@ -1295,8 +1327,34 @@ int power_supply_set_property(struct power_supply *psy,
>
> return psy->desc->set_property(psy, psp, val);
> }
> +
> +int power_supply_set_property(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, true);
> +}
> EXPORT_SYMBOL_GPL(power_supply_set_property);
>
> +/**
> + * power_supply_set_property_direct - Write a power supply property without checking for extensions
> + * @psy: The power supply
> + * @psp: The power supply property to write
> + * @val: The value to write to the power supply property
> + *
> + * Write a power supply property without taking into account any power supply extensions registered
> + * on the given power supply. This is mostly useful for power supply extensions that want to access
> + * their own power supply as using power_supply_set_property() directly will result in a potential
> + * deadlock.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + return __power_supply_set_property(psy, psp, val, false);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_set_property_direct);
> +
> int power_supply_property_is_writeable(struct power_supply *psy,
> enum power_supply_property psp)
> {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 45468959dd98..f21f806bfb38 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -878,15 +878,23 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
> extern int power_supply_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val);
> +int power_supply_get_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + union power_supply_propval *val);
> #if IS_ENABLED(CONFIG_POWER_SUPPLY)
> extern int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val);
> +int power_supply_set_property_direct(struct power_supply *psy, enum power_supply_property psp,
> + const union power_supply_propval *val);
> #else
> static inline int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val)
> { return 0; }
> +static inline int power_supply_set_property_direct(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{ return 0; }
> #endif
> extern void power_supply_external_power_changed(struct power_supply *psy);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-07-07 13:00 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-27 20:51 [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Armin Wolf
2025-06-27 20:51 ` [PATCH 2/3] power: supply: test-power: Test access to extended power supply Armin Wolf
2025-07-06 23:13 ` Sebastian Reichel
2025-06-27 20:51 ` [PATCH 3/3] platform/x86: dell-ddv: Fix taking the psy->extensions_sem lock twice Armin Wolf
2025-06-28 9:25 ` [PATCH 1/3] power: supply: core: Add power_supply_get/set_property_direct() Hans de Goede
2025-06-28 21:34 ` Armin Wolf
2025-06-30 8:58 ` Ilpo Järvinen
2025-07-06 23:14 ` Sebastian Reichel
2025-07-06 23:11 ` Sebastian Reichel
2025-07-07 13:00 ` Ilpo Järvinen
-- strict thread matches above, loose matches on Subject: below --
2025-06-27 20:53 Armin Wolf
2025-06-27 20:55 ` Armin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.