Message-Id: <20240904-power-supply-extensions-v3-7-62efeb93f8ec@weissschuh.net>
Date: Wed, 04 Sep 2024 21:25:40 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Sebastian Reichel <sre@...nel.org>, Armin Wolf <W_Armin@....de>, 
 Hans de Goede <hdegoede@...hat.com>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
 Thomas Weißschuh <linux@...ssschuh.net>
Subject: [PATCH RFC v3 7/9] power: supply: core: add locking around
 extension access

Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>

---
This is only separate for easier review during the RFC phase.
It will be squashed into the actual power supply extension commit later.
---
 drivers/power/supply/power_supply.h       |  3 ++
 drivers/power/supply/power_supply_core.c  | 49 ++++++++++++++++++++++++++-----
 drivers/power/supply/power_supply_leds.c  |  2 ++
 drivers/power/supply/power_supply_sysfs.c |  6 ++++
 include/linux/power_supply.h              |  3 ++
 5 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index fb45f0717bd1..b8e14cc70fcf 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -9,6 +9,8 @@
  *  Modified: 2004, Oct     Szabolcs Gyurko
  */
 
+#include <linux/lockdep.h>
+
 struct device;
 struct device_type;
 struct power_supply;
@@ -25,6 +27,7 @@ struct power_supply_ext_registration {
 };
 
 #define power_supply_for_each_extension(pos, psy) \
+	lockdep_assert_held(&(psy)->extensions_sem); \
 	list_for_each_entry(pos, &(psy)->extensions, list_head)
 
 
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index db5e7af57e67..4839be25e6be 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -11,6 +11,7 @@
 
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/cleanup.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
@@ -80,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
 
 static void power_supply_changed_work(struct work_struct *work)
 {
+	int ret;
 	unsigned long flags;
 	struct power_supply *psy = container_of(work, struct power_supply,
 						changed_work);
@@ -87,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
 	dev_dbg(&psy->dev, "%s\n", __func__);
 
 	spin_lock_irqsave(&psy->changed_lock, flags);
+
+	if (unlikely(psy->update_groups)) {
+		psy->update_groups = false;
+		spin_unlock_irqrestore(&psy->changed_lock, flags);
+		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+		if (ret)
+			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
+		spin_lock_irqsave(&psy->changed_lock, flags);
+	}
+
 	/*
 	 * Check 'changed' here to avoid issues due to race between
 	 * power_supply_changed() and this routine. In worst case
@@ -1218,17 +1230,26 @@ bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
 	return found;
 }
 
-bool power_supply_has_property(struct power_supply *psy,
-			       enum power_supply_property psp)
+static bool psy_has_property_no_ext(struct power_supply *psy,
+				    enum power_supply_property psp)
 {
-	struct power_supply_ext_registration *reg;
-
 	if (psy_desc_has_property(psy->desc, psp))
 		return true;
 
 	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
 		return true;
 
+	return false;
+}
+
+bool power_supply_has_property(struct power_supply *psy,
+			       enum power_supply_property psp)
+{
+	struct power_supply_ext_registration *reg;
+
+	if (psy_has_property_no_ext(psy, psp))
+		return true;
+
 	power_supply_for_each_extension(reg, psy)
 		if (power_supply_ext_has_property(reg->ext, psp))
 			return true;
@@ -1329,11 +1350,14 @@ EXPORT_SYMBOL_GPL(power_supply_powers);
 
 static int power_supply_update_groups(struct power_supply *psy)
 {
-	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	psy->update_groups = true;
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
 
-	ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
 	power_supply_changed(psy);
-	return ret;
+	return 0;
 }
 
 int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
@@ -1342,6 +1366,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
 	struct power_supply_ext_registration *reg;
 	size_t i;
 
+	guard(rwsem_write)(&psy->extensions_sem);
+
 	for (i = 0; i < ext->num_properties; i++) {
 		if (power_supply_has_property(psy, ext->properties[i]))
 			return -EEXIST;
@@ -1362,6 +1388,8 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
 {
 	struct power_supply_ext_registration *reg;
 
+	guard(rwsem_write)(&psy->extensions_sem);
+
 	power_supply_for_each_extension(reg, psy) {
 		if (reg->ext == ext) {
 			list_del(&reg->list_head);
@@ -1405,6 +1433,7 @@ static int power_supply_read_temp(struct thermal_zone_device *tzd,
 
 	WARN_ON(tzd == NULL);
 	psy = thermal_zone_device_priv(tzd);
+	guard(rwsem_read)(&psy->extensions_sem);
 	ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
 	if (ret)
 		return ret;
@@ -1427,7 +1456,7 @@ static int psy_register_thermal(struct power_supply *psy)
 		return 0;
 
 	/* Register battery zone device psy reports temperature */
-	if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
+	if (psy_has_property_no_ext(psy, POWER_SUPPLY_PROP_TEMP)) {
 		/* Prefer our hwmon device and avoid duplicates */
 		struct thermal_zone_params tzp = {
 			.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1534,7 +1563,9 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	init_rwsem(&psy->extensions_sem);
 	INIT_LIST_HEAD(&psy->extensions);
+
 	rc = device_add(dev);
 	if (rc)
 		goto device_add_failed;
@@ -1547,6 +1578,8 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	rc = power_supply_create_triggers(psy);
 	if (rc)
 		goto create_triggers_failed;
diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
index f4a7e566bea1..09a6f3fe2f85 100644
--- a/drivers/power/supply/power_supply_leds.c
+++ b/drivers/power/supply/power_supply_leds.c
@@ -195,6 +195,8 @@ static void power_supply_remove_gen_triggers(struct power_supply *psy)
 
 void power_supply_update_leds(struct power_supply *psy)
 {
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	if (psy->desc->type == POWER_SUPPLY_TYPE_BATTERY)
 		power_supply_update_bat_leds(psy);
 	else
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index e7c306afd846..b56e0bd424f5 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -371,6 +371,8 @@ static ssize_t power_supply_store_property(struct device *dev,
 
 	value.intval = ret;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	ret = power_supply_set_property(psy, psp, &value);
 	if (ret < 0)
 		return ret;
@@ -392,6 +394,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	if (attrno == POWER_SUPPLY_PROP_TYPE)
 		return mode;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	if (power_supply_has_property(psy, attrno)) {
 		if (power_supply_property_is_writeable(psy, attrno) > 0)
 			mode |= S_IWUSR;
@@ -497,6 +501,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	if (ret)
 		goto out;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
 		ret = add_prop_uevent(dev, env, j, prop_buf);
 		if (ret)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 51788faf1d66..87bc50698649 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 #include <linux/leds.h>
+#include <linux/rwsem.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
@@ -322,10 +323,12 @@ struct power_supply {
 	struct delayed_work deferred_register_work;
 	spinlock_t changed_lock;
 	bool changed;
+	bool update_groups;
 	bool initialized;
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	struct rw_semaphore extensions_sem; /* protects "extensions" */
 	struct list_head extensions;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;

-- 
2.46.0

Powered by blists - more mailing lists