[PATCH 0/8] pwm: Add support for character devices

[PATCH 0/8] pwm: Add support for character devices

[Uwe Kleine-König]

Hello,

After the necessary changes to the lowlevel drivers got in for v6.9-rc1
here come some changes to the core to implement /dev/pwmchipX character
devices.

In my tests on an ARM STM32MP1 programming a PWM using the character
device is ~4 times faster than just changing duty_cycle via the sysfs
API. It also has the advantage that (similar to pwm_apply_*) the target
state is provided to the kernel with a single call, instead of having to
program the individual settings one after another via sysfs (in the
right order to not cross states not supported by the driver). 

Note the representation of a PWM waveform is different here compared to
the in-kernel representation. A PWM waveform is represented using:

	period
	duty_cycle
	duty_offset

A disabled PWM is represented by period = 0. For an inversed wave use:

	duty_offset = duty_cycle
	duty_cycle = period - duty_cycle;

. However there are some difficulties yet that make it hard to provide a
consistent API to userspace and so for now duty_offset isn't (fully)
supported yet. That needs some more consideration and can be added
later.

A userspace lib together with some simple test programs making use of
this new API can be found at

	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git

.

The start of the series is some cleanup and preparation. The lifetime
and locking patches are needed to not crash the kernel when a character
device is open while a lowlevel driver goes away.

I look forward to feedback,
Uwe

Uwe Kleine-König (8):
  pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()
  pwm: Give some sysfs related variables and functions better names
  pwm: Move contents of sysfs.c into core.c
  pwm: Ensure a struct pwm has the same lifetime as its pwm_chip
  pwm: Add a struct device to struct pwm_chip
  pwm: Make pwmchip_[sg]et_drvdata() a wrapper around dev_set_drvdata()
  pwm: Add more locking
  pwm: Add support for pwmchip devices for faster and easier userspace
    access

 drivers/pwm/Kconfig      |   4 -
 drivers/pwm/Makefile     |   1 -
 drivers/pwm/core.c       | 988 +++++++++++++++++++++++++++++++++++++--
 drivers/pwm/sysfs.c      | 545 ---------------------
 include/linux/pwm.h      |  51 +-
 include/uapi/linux/pwm.h |  23 +
 6 files changed, 993 insertions(+), 619 deletions(-)
 delete mode 100644 drivers/pwm/sysfs.c
 create mode 100644 include/uapi/linux/pwm.h

base-commit: dd6c6d57ab61d496f6ff7d6ca38611062af142a1
-- 
2.43.0

[PATCH 1/8] pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()

[Uwe Kleine-König]

Memory holding a struct device must not be freed before the reference
count drops to zero. So a struct pwm_chip must not live in memory
freed by a driver on unbind. All in-tree drivers were fixed accordingly,
but as out-of-tree drivers, that were not adapted, still compile fine,
catch these in pwmchip_add().

Link: https://lore.kernel.org/r/35f5b229c98f78b2f6ce2397259a4a936be477c0.1707900770.git.u.kleine-koenig@pengutronix.de
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 10 ++++++++++
 include/linux/pwm.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d70f793ce4b3..fe83333c466a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -481,6 +481,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 
 	chip->dev = parent;
 	chip->npwm = npwm;
+	chip->uses_pwmchip_alloc = true;
 
 	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
 
@@ -561,6 +562,15 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	if (!chip || !pwmchip_parent(chip) || !chip->ops || !chip->npwm)
 		return -EINVAL;
 
+	/*
+	 * a struct pwm_chip must be allocated using (devm_)pwmchip_alloc,
+	 * otherwise the embedded struct device might disappear too early
+	 * resulting in memory corruption.
+	 * Catch drivers that were not converted appropriately.
+	 */
+	if (!chip->uses_pwmchip_alloc)
+		return -EINVAL;
+
 	if (!pwm_ops_check(chip))
 		return -EINVAL;
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 4a6568dfdf3f..94a642a88817 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -272,6 +272,7 @@ struct pwm_ops {
  * @npwm: number of PWMs controlled by this chip
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @atomic: can the driver's ->apply() be called in atomic context
+ * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
  * @driver_data: Private pointer for driver specific info
  * @pwms: array of PWM devices allocated by the framework
  */
@@ -287,6 +288,7 @@ struct pwm_chip {
 	bool atomic;
 
 	/* only used internally by the PWM framework */
+	bool uses_pwmchip_alloc;
 	void *driver_data;
 	struct pwm_device *pwms;
 };
-- 
2.43.0

[PATCH 2/8] pwm: Give some sysfs related variables and functions better names

[Uwe Kleine-König]

The code handling the sysfs API uses "child" and "parent" to refer to
the devices corresponding to a struct pwm or a struct pwm_chip
respectively.

Other parts of the pwm core use "parent" to refer to the parent device of
a struct pwm_chip.

So rename "child" to "pwm_dev" and "parent" to "pwmchip_dev" which
better explains the semantic. Also two functions are changed to match
the new names:

        child_to_pwm_export() -> pwmexport_from_dev()
        child_to_pwm_device() -> pwm_from_dev()

(which have the additional advantage to start with "pwm" which gives the
right scope). Additionally introduce a wrapper for dev_get_drvdata() to
convert a pwmchip_dev to the respective pwm_chip.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/sysfs.c | 161 +++++++++++++++++++++++---------------------
 1 file changed, 83 insertions(+), 78 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 3f434a771fb5..1a1b1f6dd98f 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -15,29 +15,34 @@
 #include <linux/pwm.h>
 
 struct pwm_export {
-	struct device child;
+	struct device pwm_dev;
 	struct pwm_device *pwm;
 	struct mutex lock;
 	struct pwm_state suspend;
 };
 
-static struct pwm_export *child_to_pwm_export(struct device *child)
+static inline struct pwm_chip *pwmchip_from_dev(struct device *pwmchip_dev)
 {
-	return container_of(child, struct pwm_export, child);
+	return dev_get_drvdata(pwmchip_dev);
 }
 
-static struct pwm_device *child_to_pwm_device(struct device *child)
+static inline struct pwm_export *pwmexport_from_dev(struct device *pwm_dev)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	return container_of(pwm_dev, struct pwm_export, pwm_dev);
+}
+
+static inline struct pwm_device *pwm_from_dev(struct device *pwm_dev)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 
 	return export->pwm;
 }
 
-static ssize_t period_show(struct device *child,
+static ssize_t period_show(struct device *pwm_dev,
 			   struct device_attribute *attr,
 			   char *buf)
 {
-	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
@@ -45,11 +50,11 @@ static ssize_t period_show(struct device *child,
 	return sysfs_emit(buf, "%llu\n", state.period);
 }
 
-static ssize_t period_store(struct device *child,
+static ssize_t period_store(struct device *pwm_dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t size)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 	struct pwm_device *pwm = export->pwm;
 	struct pwm_state state;
 	u64 val;
@@ -68,11 +73,11 @@ static ssize_t period_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t duty_cycle_show(struct device *child,
+static ssize_t duty_cycle_show(struct device *pwm_dev,
 			       struct device_attribute *attr,
 			       char *buf)
 {
-	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
@@ -80,11 +85,11 @@ static ssize_t duty_cycle_show(struct device *child,
 	return sysfs_emit(buf, "%llu\n", state.duty_cycle);
 }
 
-static ssize_t duty_cycle_store(struct device *child,
+static ssize_t duty_cycle_store(struct device *pwm_dev,
 				struct device_attribute *attr,
 				const char *buf, size_t size)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 	struct pwm_device *pwm = export->pwm;
 	struct pwm_state state;
 	u64 val;
@@ -103,11 +108,11 @@ static ssize_t duty_cycle_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t enable_show(struct device *child,
+static ssize_t enable_show(struct device *pwm_dev,
 			   struct device_attribute *attr,
 			   char *buf)
 {
-	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
@@ -115,11 +120,11 @@ static ssize_t enable_show(struct device *child,
 	return sysfs_emit(buf, "%d\n", state.enabled);
 }
 
-static ssize_t enable_store(struct device *child,
+static ssize_t enable_store(struct device *pwm_dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t size)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 	struct pwm_device *pwm = export->pwm;
 	struct pwm_state state;
 	int val, ret;
@@ -151,11 +156,11 @@ static ssize_t enable_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t polarity_show(struct device *child,
+static ssize_t polarity_show(struct device *pwm_dev,
 			     struct device_attribute *attr,
 			     char *buf)
 {
-	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
 	const char *polarity = "unknown";
 	struct pwm_state state;
 
@@ -174,11 +179,11 @@ static ssize_t polarity_show(struct device *child,
 	return sysfs_emit(buf, "%s\n", polarity);
 }
 
-static ssize_t polarity_store(struct device *child,
+static ssize_t polarity_store(struct device *pwm_dev,
 			      struct device_attribute *attr,
 			      const char *buf, size_t size)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 	struct pwm_device *pwm = export->pwm;
 	enum pwm_polarity polarity;
 	struct pwm_state state;
@@ -200,11 +205,11 @@ static ssize_t polarity_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t capture_show(struct device *child,
+static ssize_t capture_show(struct device *pwm_dev,
 			    struct device_attribute *attr,
 			    char *buf)
 {
-	struct pwm_device *pwm = child_to_pwm_device(child);
+	struct pwm_device *pwm = pwm_from_dev(pwm_dev);
 	struct pwm_capture result;
 	int ret;
 
@@ -231,14 +236,14 @@ static struct attribute *pwm_attrs[] = {
 };
 ATTRIBUTE_GROUPS(pwm);
 
-static void pwm_export_release(struct device *child)
+static void pwm_export_release(struct device *pwm_dev)
 {
-	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
 
 	kfree(export);
 }
 
-static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
+static int pwm_export_child(struct device *pwmchip_dev, struct pwm_device *pwm)
 {
 	struct pwm_export *export;
 	char *pwm_prop[2];
@@ -256,62 +261,62 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 	export->pwm = pwm;
 	mutex_init(&export->lock);
 
-	export->child.release = pwm_export_release;
-	export->child.parent = parent;
-	export->child.devt = MKDEV(0, 0);
-	export->child.groups = pwm_groups;
-	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+	export->pwm_dev.release = pwm_export_release;
+	export->pwm_dev.parent = pwmchip_dev;
+	export->pwm_dev.devt = MKDEV(0, 0);
+	export->pwm_dev.groups = pwm_groups;
+	dev_set_name(&export->pwm_dev, "pwm%u", pwm->hwpwm);
 
-	ret = device_register(&export->child);
+	ret = device_register(&export->pwm_dev);
 	if (ret) {
 		clear_bit(PWMF_EXPORTED, &pwm->flags);
-		put_device(&export->child);
+		put_device(&export->pwm_dev);
 		export = NULL;
 		return ret;
 	}
 	pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
 	pwm_prop[1] = NULL;
-	kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
+	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
 	kfree(pwm_prop[0]);
 
 	return 0;
 }
 
-static int pwm_unexport_match(struct device *child, void *data)
+static int pwm_unexport_match(struct device *pwm_dev, void *data)
 {
-	return child_to_pwm_device(child) == data;
+	return pwm_from_dev(pwm_dev) == data;
 }
 
-static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
+static int pwm_unexport_child(struct device *pwmchip_dev, struct pwm_device *pwm)
 {
-	struct device *child;
+	struct device *pwm_dev;
 	char *pwm_prop[2];
 
 	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
 		return -ENODEV;
 
-	child = device_find_child(parent, pwm, pwm_unexport_match);
-	if (!child)
+	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
+	if (!pwm_dev)
 		return -ENODEV;
 
 	pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
 	pwm_prop[1] = NULL;
-	kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
+	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
 	kfree(pwm_prop[0]);
 
 	/* for device_find_child() */
-	put_device(child);
-	device_unregister(child);
+	put_device(pwm_dev);
+	device_unregister(pwm_dev);
 	pwm_put(pwm);
 
 	return 0;
 }
 
-static ssize_t export_store(struct device *parent,
+static ssize_t export_store(struct device *pwmchip_dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
-	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 	struct pwm_device *pwm;
 	unsigned int hwpwm;
 	int ret;
@@ -327,7 +332,7 @@ static ssize_t export_store(struct device *parent,
 	if (IS_ERR(pwm))
 		return PTR_ERR(pwm);
 
-	ret = pwm_export_child(parent, pwm);
+	ret = pwm_export_child(pwmchip_dev, pwm);
 	if (ret < 0)
 		pwm_put(pwm);
 
@@ -335,11 +340,11 @@ static ssize_t export_store(struct device *parent,
 }
 static DEVICE_ATTR_WO(export);
 
-static ssize_t unexport_store(struct device *parent,
+static ssize_t unexport_store(struct device *pwmchip_dev,
 			      struct device_attribute *attr,
 			      const char *buf, size_t len)
 {
-	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 	unsigned int hwpwm;
 	int ret;
 
@@ -350,16 +355,16 @@ static ssize_t unexport_store(struct device *parent,
 	if (hwpwm >= chip->npwm)
 		return -ENODEV;
 
-	ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]);
+	ret = pwm_unexport_child(pwmchip_dev, &chip->pwms[hwpwm]);
 
 	return ret ? : len;
 }
 static DEVICE_ATTR_WO(unexport);
 
-static ssize_t npwm_show(struct device *parent, struct device_attribute *attr,
+static ssize_t npwm_show(struct device *pwmchip_dev, struct device_attribute *attr,
 			 char *buf)
 {
-	const struct pwm_chip *chip = dev_get_drvdata(parent);
+	const struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 
 	return sysfs_emit(buf, "%u\n", chip->npwm);
 }
@@ -374,22 +379,22 @@ static struct attribute *pwm_chip_attrs[] = {
 ATTRIBUTE_GROUPS(pwm_chip);
 
 /* takes export->lock on success */
-static struct pwm_export *pwm_class_get_state(struct device *parent,
+static struct pwm_export *pwm_class_get_state(struct device *pwmchip_dev,
 					      struct pwm_device *pwm,
 					      struct pwm_state *state)
 {
-	struct device *child;
+	struct device *pwm_dev;
 	struct pwm_export *export;
 
 	if (!test_bit(PWMF_EXPORTED, &pwm->flags))
 		return NULL;
 
-	child = device_find_child(parent, pwm, pwm_unexport_match);
-	if (!child)
+	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
+	if (!pwm_dev)
 		return NULL;
 
-	export = child_to_pwm_export(child);
-	put_device(child);	/* for device_find_child() */
+	export = pwmexport_from_dev(pwm_dev);
+	put_device(pwm_dev);	/* for device_find_child() */
 
 	mutex_lock(&export->lock);
 	pwm_get_state(pwm, state);
@@ -409,9 +414,9 @@ static int pwm_class_apply_state(struct pwm_export *export,
 	return ret;
 }
 
-static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
+static int pwm_class_resume_npwm(struct device *pwmchip_dev, unsigned int npwm)
 {
-	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 	unsigned int i;
 	int ret = 0;
 
@@ -420,7 +425,7 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 		struct pwm_state state;
 		struct pwm_export *export;
 
-		export = pwm_class_get_state(parent, pwm, &state);
+		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
 		if (!export)
 			continue;
 
@@ -440,9 +445,9 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 	return ret;
 }
 
-static int pwm_class_suspend(struct device *parent)
+static int pwm_class_suspend(struct device *pwmchip_dev)
 {
-	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 	unsigned int i;
 	int ret = 0;
 
@@ -451,7 +456,7 @@ static int pwm_class_suspend(struct device *parent)
 		struct pwm_state state;
 		struct pwm_export *export;
 
-		export = pwm_class_get_state(parent, pwm, &state);
+		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
 		if (!export)
 			continue;
 
@@ -473,7 +478,7 @@ static int pwm_class_suspend(struct device *parent)
 			 * roll back the PWM devices that were disabled by
 			 * this suspend function.
 			 */
-			pwm_class_resume_npwm(parent, i);
+			pwm_class_resume_npwm(pwmchip_dev, i);
 			break;
 		}
 	}
@@ -481,11 +486,11 @@ static int pwm_class_suspend(struct device *parent)
 	return ret;
 }
 
-static int pwm_class_resume(struct device *parent)
+static int pwm_class_resume(struct device *pwmchip_dev)
 {
-	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 
-	return pwm_class_resume_npwm(parent, chip->npwm);
+	return pwm_class_resume_npwm(pwmchip_dev, chip->npwm);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume);
@@ -496,22 +501,22 @@ static struct class pwm_class = {
 	.pm = pm_sleep_ptr(&pwm_class_pm_ops),
 };
 
-static int pwmchip_sysfs_match(struct device *parent, const void *data)
+static int pwmchip_sysfs_match(struct device *pwmchip_dev, const void *data)
 {
-	return dev_get_drvdata(parent) == data;
+	return pwmchip_from_dev(pwmchip_dev) == data;
 }
 
 void pwmchip_sysfs_export(struct pwm_chip *chip)
 {
-	struct device *parent;
+	struct device *pwmchip_dev;
 
 	/*
 	 * If device_create() fails the pwm_chip is still usable by
 	 * the kernel it's just not exported.
 	 */
-	parent = device_create(&pwm_class, pwmchip_parent(chip), MKDEV(0, 0), chip,
+	pwmchip_dev = device_create(&pwm_class, pwmchip_parent(chip), MKDEV(0, 0), chip,
 			       "pwmchip%d", chip->id);
-	if (IS_ERR(parent)) {
+	if (IS_ERR(pwmchip_dev)) {
 		dev_warn(pwmchip_parent(chip),
 			 "device_create failed for pwm_chip sysfs export\n");
 	}
@@ -519,23 +524,23 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
 
 void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 {
-	struct device *parent;
+	struct device *pwmchip_dev;
 	unsigned int i;
 
-	parent = class_find_device(&pwm_class, NULL, chip,
+	pwmchip_dev = class_find_device(&pwm_class, NULL, chip,
 				   pwmchip_sysfs_match);
-	if (!parent)
+	if (!pwmchip_dev)
 		return;
 
 	for (i = 0; i < chip->npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (test_bit(PWMF_EXPORTED, &pwm->flags))
-			pwm_unexport_child(parent, pwm);
+			pwm_unexport_child(pwmchip_dev, pwm);
 	}
 
-	put_device(parent);
-	device_unregister(parent);
+	put_device(pwmchip_dev);
+	device_unregister(pwmchip_dev);
 }
 
 static int __init pwm_sysfs_init(void)
-- 
2.43.0

[PATCH 3/8] pwm: Move contents of sysfs.c into core.c

[Uwe Kleine-König]

With the upcoming restructuring having all in a single file simplifies
things a bit. The relevant and somewhat visible changes are:

 - Some dropped prototypes from include/linux/pwm.h that were only
   necessary that core.c has a declaration of the symbols defined in
   sysfs.c. The respective functions are static now.

 - The pwm class now also exists if CONFIG_SYSFS isn't enabled. Having
   CONFIG_SYSFS is not very relevant today, but even without it the
   class and device stuff still provides lifetime tracking.

 - Both files had an initcall, these are merged into a single one now.
   Instead of a big #ifdef block for CONFIG_DEBUG_FS, a single
   if (IS_ENABLED(CONFIG_DEBUG_FS)) is used now. This increases compile
   coverage a bit and is a tad nicer on the eyes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/Kconfig  |   4 -
 drivers/pwm/Makefile |   1 -
 drivers/pwm/core.c   | 540 +++++++++++++++++++++++++++++++++++++++++-
 drivers/pwm/sysfs.c  | 550 -------------------------------------------
 include/linux/pwm.h  |  13 -
 5 files changed, 534 insertions(+), 574 deletions(-)
 delete mode 100644 drivers/pwm/sysfs.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..1dd7921194f5 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -29,10 +29,6 @@ menuconfig PWM
 
 if PWM
 
-config PWM_SYSFS
-	bool
-	default y if SYSFS
-
 config PWM_DEBUG
 	bool "PWM lowlevel drivers additional checks and debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..90913519f11a 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PWM)		+= core.o
-obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index fe83333c466a..69af8123455a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -454,6 +454,535 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
 }
 EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
 
+struct pwm_export {
+	struct device pwm_dev;
+	struct pwm_device *pwm;
+	struct mutex lock;
+	struct pwm_state suspend;
+};
+
+static inline struct pwm_chip *pwmchip_from_dev(struct device *pwmchip_dev)
+{
+	return dev_get_drvdata(pwmchip_dev);
+}
+
+static inline struct pwm_export *pwmexport_from_dev(struct device *pwm_dev)
+{
+	return container_of(pwm_dev, struct pwm_export, pwm_dev);
+}
+
+static inline struct pwm_device *pwm_from_dev(struct device *pwm_dev)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+
+	return export->pwm;
+}
+
+static ssize_t period_show(struct device *pwm_dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sysfs_emit(buf, "%llu\n", state.period);
+}
+
+static ssize_t period_store(struct device *pwm_dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t size)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	u64 val;
+	int ret;
+
+	ret = kstrtou64(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.period = val;
+	ret = pwm_apply_might_sleep(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
+static ssize_t duty_cycle_show(struct device *pwm_dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sysfs_emit(buf, "%llu\n", state.duty_cycle);
+}
+
+static ssize_t duty_cycle_store(struct device *pwm_dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	u64 val;
+	int ret;
+
+	ret = kstrtou64(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.duty_cycle = val;
+	ret = pwm_apply_might_sleep(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
+static ssize_t enable_show(struct device *pwm_dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sysfs_emit(buf, "%d\n", state.enabled);
+}
+
+static ssize_t enable_store(struct device *pwm_dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t size)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+
+	pwm_get_state(pwm, &state);
+
+	switch (val) {
+	case 0:
+		state.enabled = false;
+		break;
+	case 1:
+		state.enabled = true;
+		break;
+	default:
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	ret = pwm_apply_might_sleep(pwm, &state);
+
+unlock:
+	mutex_unlock(&export->lock);
+	return ret ? : size;
+}
+
+static ssize_t polarity_show(struct device *pwm_dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	const char *polarity = "unknown";
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	switch (state.polarity) {
+	case PWM_POLARITY_NORMAL:
+		polarity = "normal";
+		break;
+
+	case PWM_POLARITY_INVERSED:
+		polarity = "inversed";
+		break;
+	}
+
+	return sysfs_emit(buf, "%s\n", polarity);
+}
+
+static ssize_t polarity_store(struct device *pwm_dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+	struct pwm_device *pwm = export->pwm;
+	enum pwm_polarity polarity;
+	struct pwm_state state;
+	int ret;
+
+	if (sysfs_streq(buf, "normal"))
+		polarity = PWM_POLARITY_NORMAL;
+	else if (sysfs_streq(buf, "inversed"))
+		polarity = PWM_POLARITY_INVERSED;
+	else
+		return -EINVAL;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.polarity = polarity;
+	ret = pwm_apply_might_sleep(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
+static ssize_t capture_show(struct device *pwm_dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	struct pwm_capture result;
+	int ret;
+
+	ret = pwm_capture(pwm, &result, jiffies_to_msecs(HZ));
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u %u\n", result.period, result.duty_cycle);
+}
+
+static DEVICE_ATTR_RW(period);
+static DEVICE_ATTR_RW(duty_cycle);
+static DEVICE_ATTR_RW(enable);
+static DEVICE_ATTR_RW(polarity);
+static DEVICE_ATTR_RO(capture);
+
+static struct attribute *pwm_attrs[] = {
+	&dev_attr_period.attr,
+	&dev_attr_duty_cycle.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_polarity.attr,
+	&dev_attr_capture.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(pwm);
+
+static void pwm_export_release(struct device *pwm_dev)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+
+	kfree(export);
+}
+
+static int pwm_export_child(struct device *pwmchip_dev, struct pwm_device *pwm)
+{
+	struct pwm_export *export;
+	char *pwm_prop[2];
+	int ret;
+
+	if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
+		return -EBUSY;
+
+	export = kzalloc(sizeof(*export), GFP_KERNEL);
+	if (!export) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		return -ENOMEM;
+	}
+
+	export->pwm = pwm;
+	mutex_init(&export->lock);
+
+	export->pwm_dev.release = pwm_export_release;
+	export->pwm_dev.parent = pwmchip_dev;
+	export->pwm_dev.devt = MKDEV(0, 0);
+	export->pwm_dev.groups = pwm_groups;
+	dev_set_name(&export->pwm_dev, "pwm%u", pwm->hwpwm);
+
+	ret = device_register(&export->pwm_dev);
+	if (ret) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		put_device(&export->pwm_dev);
+		export = NULL;
+		return ret;
+	}
+	pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
+	pwm_prop[1] = NULL;
+	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
+	kfree(pwm_prop[0]);
+
+	return 0;
+}
+
+static int pwm_unexport_match(struct device *pwm_dev, void *data)
+{
+	return pwm_from_dev(pwm_dev) == data;
+}
+
+static int pwm_unexport_child(struct device *pwmchip_dev, struct pwm_device *pwm)
+{
+	struct device *pwm_dev;
+	char *pwm_prop[2];
+
+	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
+		return -ENODEV;
+
+	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
+	if (!pwm_dev)
+		return -ENODEV;
+
+	pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
+	pwm_prop[1] = NULL;
+	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
+	kfree(pwm_prop[0]);
+
+	/* for device_find_child() */
+	put_device(pwm_dev);
+	device_unregister(pwm_dev);
+	pwm_put(pwm);
+
+	return 0;
+}
+
+static ssize_t export_store(struct device *pwmchip_dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t len)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+	struct pwm_device *pwm;
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	pwm = pwm_request_from_chip(chip, hwpwm, "sysfs");
+	if (IS_ERR(pwm))
+		return PTR_ERR(pwm);
+
+	ret = pwm_export_child(pwmchip_dev, pwm);
+	if (ret < 0)
+		pwm_put(pwm);
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_WO(export);
+
+static ssize_t unexport_store(struct device *pwmchip_dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t len)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	ret = pwm_unexport_child(pwmchip_dev, &chip->pwms[hwpwm]);
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_WO(unexport);
+
+static ssize_t npwm_show(struct device *pwmchip_dev, struct device_attribute *attr,
+			 char *buf)
+{
+	const struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+
+	return sysfs_emit(buf, "%u\n", chip->npwm);
+}
+static DEVICE_ATTR_RO(npwm);
+
+static struct attribute *pwm_chip_attrs[] = {
+	&dev_attr_export.attr,
+	&dev_attr_unexport.attr,
+	&dev_attr_npwm.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(pwm_chip);
+
+/* takes export->lock on success */
+static struct pwm_export *pwm_class_get_state(struct device *pwmchip_dev,
+					      struct pwm_device *pwm,
+					      struct pwm_state *state)
+{
+	struct device *pwm_dev;
+	struct pwm_export *export;
+
+	if (!test_bit(PWMF_EXPORTED, &pwm->flags))
+		return NULL;
+
+	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
+	if (!pwm_dev)
+		return NULL;
+
+	export = pwmexport_from_dev(pwm_dev);
+	put_device(pwm_dev);	/* for device_find_child() */
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, state);
+
+	return export;
+}
+
+static int pwm_class_apply_state(struct pwm_export *export,
+				 struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	int ret = pwm_apply_might_sleep(pwm, state);
+
+	/* release lock taken in pwm_class_get_state */
+	mutex_unlock(&export->lock);
+
+	return ret;
+}
+
+static int pwm_class_resume_npwm(struct device *pwmchip_dev, unsigned int npwm)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+		struct pwm_state state;
+		struct pwm_export *export;
+
+		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
+		if (!export)
+			continue;
+
+		/* If pwmchip was not enabled before suspend, do nothing. */
+		if (!export->suspend.enabled) {
+			/* release lock taken in pwm_class_get_state */
+			mutex_unlock(&export->lock);
+			continue;
+		}
+
+		state.enabled = export->suspend.enabled;
+		ret = pwm_class_apply_state(export, pwm, &state);
+		if (ret < 0)
+			break;
+	}
+
+	return ret;
+}
+
+static int pwm_class_suspend(struct device *pwmchip_dev)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+		struct pwm_state state;
+		struct pwm_export *export;
+
+		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
+		if (!export)
+			continue;
+
+		/*
+		 * If pwmchip was not enabled before suspend, save
+		 * state for resume time and do nothing else.
+		 */
+		export->suspend = state;
+		if (!state.enabled) {
+			/* release lock taken in pwm_class_get_state */
+			mutex_unlock(&export->lock);
+			continue;
+		}
+
+		state.enabled = false;
+		ret = pwm_class_apply_state(export, pwm, &state);
+		if (ret < 0) {
+			/*
+			 * roll back the PWM devices that were disabled by
+			 * this suspend function.
+			 */
+			pwm_class_resume_npwm(pwmchip_dev, i);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int pwm_class_resume(struct device *pwmchip_dev)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+
+	return pwm_class_resume_npwm(pwmchip_dev, chip->npwm);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume);
+
+static struct class pwm_class = {
+	.name = "pwm",
+	.dev_groups = pwm_chip_groups,
+	.pm = pm_sleep_ptr(&pwm_class_pm_ops),
+};
+
+static int pwmchip_sysfs_match(struct device *pwmchip_dev, const void *data)
+{
+	return pwmchip_from_dev(pwmchip_dev) == data;
+}
+
+static void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+	struct device *pwmchip_dev;
+
+	/*
+	 * If device_create() fails the pwm_chip is still usable by
+	 * the kernel it's just not exported.
+	 */
+	pwmchip_dev = device_create(&pwm_class, pwmchip_parent(chip), MKDEV(0, 0), chip,
+			       "pwmchip%d", chip->id);
+	if (IS_ERR(pwmchip_dev)) {
+		dev_warn(pwmchip_parent(chip),
+			 "device_create failed for pwm_chip sysfs export\n");
+	}
+}
+
+static void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+	struct device *pwmchip_dev;
+	unsigned int i;
+
+	pwmchip_dev = class_find_device(&pwm_class, NULL, chip,
+					pwmchip_sysfs_match);
+	if (!pwmchip_dev)
+		return;
+
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+
+		if (test_bit(PWMF_EXPORTED, &pwm->flags))
+			pwm_unexport_child(pwmchip_dev, pwm);
+	}
+
+	put_device(pwmchip_dev);
+	device_unregister(pwmchip_dev);
+}
+
 #define PWMCHIP_ALIGN ARCH_DMA_MINALIGN
 
 static void *pwmchip_priv(struct pwm_chip *chip)
@@ -1086,7 +1615,6 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
 
-#ifdef CONFIG_DEBUG_FS
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
 	unsigned int i;
@@ -1171,11 +1699,11 @@ static const struct seq_operations pwm_debugfs_sops = {
 
 DEFINE_SEQ_ATTRIBUTE(pwm_debugfs);
 
-static int __init pwm_debugfs_init(void)
+static int __init pwm_init(void)
 {
-	debugfs_create_file("pwm", 0444, NULL, NULL, &pwm_debugfs_fops);
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		debugfs_create_file("pwm", 0444, NULL, NULL, &pwm_debugfs_fops);
 
-	return 0;
+	return class_register(&pwm_class);
 }
-subsys_initcall(pwm_debugfs_init);
-#endif /* CONFIG_DEBUG_FS */
+subsys_initcall(pwm_init);
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
deleted file mode 100644
index 1a1b1f6dd98f..000000000000
--- a/drivers/pwm/sysfs.c
+++ /dev/null
@@ -1,550 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * A simple sysfs interface for the generic PWM framework
- *
- * Copyright (C) 2013 H Hartley Sweeten <hsweeten@visionengravers.com>
- *
- * Based on previous work by Lars Poeschel <poeschel@lemonage.de>
- */
-
-#include <linux/device.h>
-#include <linux/mutex.h>
-#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/kdev_t.h>
-#include <linux/pwm.h>
-
-struct pwm_export {
-	struct device pwm_dev;
-	struct pwm_device *pwm;
-	struct mutex lock;
-	struct pwm_state suspend;
-};
-
-static inline struct pwm_chip *pwmchip_from_dev(struct device *pwmchip_dev)
-{
-	return dev_get_drvdata(pwmchip_dev);
-}
-
-static inline struct pwm_export *pwmexport_from_dev(struct device *pwm_dev)
-{
-	return container_of(pwm_dev, struct pwm_export, pwm_dev);
-}
-
-static inline struct pwm_device *pwm_from_dev(struct device *pwm_dev)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-
-	return export->pwm;
-}
-
-static ssize_t period_show(struct device *pwm_dev,
-			   struct device_attribute *attr,
-			   char *buf)
-{
-	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
-	struct pwm_state state;
-
-	pwm_get_state(pwm, &state);
-
-	return sysfs_emit(buf, "%llu\n", state.period);
-}
-
-static ssize_t period_store(struct device *pwm_dev,
-			    struct device_attribute *attr,
-			    const char *buf, size_t size)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-	struct pwm_device *pwm = export->pwm;
-	struct pwm_state state;
-	u64 val;
-	int ret;
-
-	ret = kstrtou64(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&export->lock);
-	pwm_get_state(pwm, &state);
-	state.period = val;
-	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
-
-	return ret ? : size;
-}
-
-static ssize_t duty_cycle_show(struct device *pwm_dev,
-			       struct device_attribute *attr,
-			       char *buf)
-{
-	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
-	struct pwm_state state;
-
-	pwm_get_state(pwm, &state);
-
-	return sysfs_emit(buf, "%llu\n", state.duty_cycle);
-}
-
-static ssize_t duty_cycle_store(struct device *pwm_dev,
-				struct device_attribute *attr,
-				const char *buf, size_t size)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-	struct pwm_device *pwm = export->pwm;
-	struct pwm_state state;
-	u64 val;
-	int ret;
-
-	ret = kstrtou64(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&export->lock);
-	pwm_get_state(pwm, &state);
-	state.duty_cycle = val;
-	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
-
-	return ret ? : size;
-}
-
-static ssize_t enable_show(struct device *pwm_dev,
-			   struct device_attribute *attr,
-			   char *buf)
-{
-	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
-	struct pwm_state state;
-
-	pwm_get_state(pwm, &state);
-
-	return sysfs_emit(buf, "%d\n", state.enabled);
-}
-
-static ssize_t enable_store(struct device *pwm_dev,
-			    struct device_attribute *attr,
-			    const char *buf, size_t size)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-	struct pwm_device *pwm = export->pwm;
-	struct pwm_state state;
-	int val, ret;
-
-	ret = kstrtoint(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&export->lock);
-
-	pwm_get_state(pwm, &state);
-
-	switch (val) {
-	case 0:
-		state.enabled = false;
-		break;
-	case 1:
-		state.enabled = true;
-		break;
-	default:
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	ret = pwm_apply_might_sleep(pwm, &state);
-
-unlock:
-	mutex_unlock(&export->lock);
-	return ret ? : size;
-}
-
-static ssize_t polarity_show(struct device *pwm_dev,
-			     struct device_attribute *attr,
-			     char *buf)
-{
-	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
-	const char *polarity = "unknown";
-	struct pwm_state state;
-
-	pwm_get_state(pwm, &state);
-
-	switch (state.polarity) {
-	case PWM_POLARITY_NORMAL:
-		polarity = "normal";
-		break;
-
-	case PWM_POLARITY_INVERSED:
-		polarity = "inversed";
-		break;
-	}
-
-	return sysfs_emit(buf, "%s\n", polarity);
-}
-
-static ssize_t polarity_store(struct device *pwm_dev,
-			      struct device_attribute *attr,
-			      const char *buf, size_t size)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-	struct pwm_device *pwm = export->pwm;
-	enum pwm_polarity polarity;
-	struct pwm_state state;
-	int ret;
-
-	if (sysfs_streq(buf, "normal"))
-		polarity = PWM_POLARITY_NORMAL;
-	else if (sysfs_streq(buf, "inversed"))
-		polarity = PWM_POLARITY_INVERSED;
-	else
-		return -EINVAL;
-
-	mutex_lock(&export->lock);
-	pwm_get_state(pwm, &state);
-	state.polarity = polarity;
-	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
-
-	return ret ? : size;
-}
-
-static ssize_t capture_show(struct device *pwm_dev,
-			    struct device_attribute *attr,
-			    char *buf)
-{
-	struct pwm_device *pwm = pwm_from_dev(pwm_dev);
-	struct pwm_capture result;
-	int ret;
-
-	ret = pwm_capture(pwm, &result, jiffies_to_msecs(HZ));
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%u %u\n", result.period, result.duty_cycle);
-}
-
-static DEVICE_ATTR_RW(period);
-static DEVICE_ATTR_RW(duty_cycle);
-static DEVICE_ATTR_RW(enable);
-static DEVICE_ATTR_RW(polarity);
-static DEVICE_ATTR_RO(capture);
-
-static struct attribute *pwm_attrs[] = {
-	&dev_attr_period.attr,
-	&dev_attr_duty_cycle.attr,
-	&dev_attr_enable.attr,
-	&dev_attr_polarity.attr,
-	&dev_attr_capture.attr,
-	NULL
-};
-ATTRIBUTE_GROUPS(pwm);
-
-static void pwm_export_release(struct device *pwm_dev)
-{
-	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
-
-	kfree(export);
-}
-
-static int pwm_export_child(struct device *pwmchip_dev, struct pwm_device *pwm)
-{
-	struct pwm_export *export;
-	char *pwm_prop[2];
-	int ret;
-
-	if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
-		return -EBUSY;
-
-	export = kzalloc(sizeof(*export), GFP_KERNEL);
-	if (!export) {
-		clear_bit(PWMF_EXPORTED, &pwm->flags);
-		return -ENOMEM;
-	}
-
-	export->pwm = pwm;
-	mutex_init(&export->lock);
-
-	export->pwm_dev.release = pwm_export_release;
-	export->pwm_dev.parent = pwmchip_dev;
-	export->pwm_dev.devt = MKDEV(0, 0);
-	export->pwm_dev.groups = pwm_groups;
-	dev_set_name(&export->pwm_dev, "pwm%u", pwm->hwpwm);
-
-	ret = device_register(&export->pwm_dev);
-	if (ret) {
-		clear_bit(PWMF_EXPORTED, &pwm->flags);
-		put_device(&export->pwm_dev);
-		export = NULL;
-		return ret;
-	}
-	pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
-	pwm_prop[1] = NULL;
-	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
-	kfree(pwm_prop[0]);
-
-	return 0;
-}
-
-static int pwm_unexport_match(struct device *pwm_dev, void *data)
-{
-	return pwm_from_dev(pwm_dev) == data;
-}
-
-static int pwm_unexport_child(struct device *pwmchip_dev, struct pwm_device *pwm)
-{
-	struct device *pwm_dev;
-	char *pwm_prop[2];
-
-	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
-		return -ENODEV;
-
-	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
-	if (!pwm_dev)
-		return -ENODEV;
-
-	pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
-	pwm_prop[1] = NULL;
-	kobject_uevent_env(&pwmchip_dev->kobj, KOBJ_CHANGE, pwm_prop);
-	kfree(pwm_prop[0]);
-
-	/* for device_find_child() */
-	put_device(pwm_dev);
-	device_unregister(pwm_dev);
-	pwm_put(pwm);
-
-	return 0;
-}
-
-static ssize_t export_store(struct device *pwmchip_dev,
-			    struct device_attribute *attr,
-			    const char *buf, size_t len)
-{
-	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-	struct pwm_device *pwm;
-	unsigned int hwpwm;
-	int ret;
-
-	ret = kstrtouint(buf, 0, &hwpwm);
-	if (ret < 0)
-		return ret;
-
-	if (hwpwm >= chip->npwm)
-		return -ENODEV;
-
-	pwm = pwm_request_from_chip(chip, hwpwm, "sysfs");
-	if (IS_ERR(pwm))
-		return PTR_ERR(pwm);
-
-	ret = pwm_export_child(pwmchip_dev, pwm);
-	if (ret < 0)
-		pwm_put(pwm);
-
-	return ret ? : len;
-}
-static DEVICE_ATTR_WO(export);
-
-static ssize_t unexport_store(struct device *pwmchip_dev,
-			      struct device_attribute *attr,
-			      const char *buf, size_t len)
-{
-	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-	unsigned int hwpwm;
-	int ret;
-
-	ret = kstrtouint(buf, 0, &hwpwm);
-	if (ret < 0)
-		return ret;
-
-	if (hwpwm >= chip->npwm)
-		return -ENODEV;
-
-	ret = pwm_unexport_child(pwmchip_dev, &chip->pwms[hwpwm]);
-
-	return ret ? : len;
-}
-static DEVICE_ATTR_WO(unexport);
-
-static ssize_t npwm_show(struct device *pwmchip_dev, struct device_attribute *attr,
-			 char *buf)
-{
-	const struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-
-	return sysfs_emit(buf, "%u\n", chip->npwm);
-}
-static DEVICE_ATTR_RO(npwm);
-
-static struct attribute *pwm_chip_attrs[] = {
-	&dev_attr_export.attr,
-	&dev_attr_unexport.attr,
-	&dev_attr_npwm.attr,
-	NULL,
-};
-ATTRIBUTE_GROUPS(pwm_chip);
-
-/* takes export->lock on success */
-static struct pwm_export *pwm_class_get_state(struct device *pwmchip_dev,
-					      struct pwm_device *pwm,
-					      struct pwm_state *state)
-{
-	struct device *pwm_dev;
-	struct pwm_export *export;
-
-	if (!test_bit(PWMF_EXPORTED, &pwm->flags))
-		return NULL;
-
-	pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
-	if (!pwm_dev)
-		return NULL;
-
-	export = pwmexport_from_dev(pwm_dev);
-	put_device(pwm_dev);	/* for device_find_child() */
-
-	mutex_lock(&export->lock);
-	pwm_get_state(pwm, state);
-
-	return export;
-}
-
-static int pwm_class_apply_state(struct pwm_export *export,
-				 struct pwm_device *pwm,
-				 struct pwm_state *state)
-{
-	int ret = pwm_apply_might_sleep(pwm, state);
-
-	/* release lock taken in pwm_class_get_state */
-	mutex_unlock(&export->lock);
-
-	return ret;
-}
-
-static int pwm_class_resume_npwm(struct device *pwmchip_dev, unsigned int npwm)
-{
-	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-	unsigned int i;
-	int ret = 0;
-
-	for (i = 0; i < npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-		struct pwm_state state;
-		struct pwm_export *export;
-
-		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
-		if (!export)
-			continue;
-
-		/* If pwmchip was not enabled before suspend, do nothing. */
-		if (!export->suspend.enabled) {
-			/* release lock taken in pwm_class_get_state */
-			mutex_unlock(&export->lock);
-			continue;
-		}
-
-		state.enabled = export->suspend.enabled;
-		ret = pwm_class_apply_state(export, pwm, &state);
-		if (ret < 0)
-			break;
-	}
-
-	return ret;
-}
-
-static int pwm_class_suspend(struct device *pwmchip_dev)
-{
-	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-	unsigned int i;
-	int ret = 0;
-
-	for (i = 0; i < chip->npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-		struct pwm_state state;
-		struct pwm_export *export;
-
-		export = pwm_class_get_state(pwmchip_dev, pwm, &state);
-		if (!export)
-			continue;
-
-		/*
-		 * If pwmchip was not enabled before suspend, save
-		 * state for resume time and do nothing else.
-		 */
-		export->suspend = state;
-		if (!state.enabled) {
-			/* release lock taken in pwm_class_get_state */
-			mutex_unlock(&export->lock);
-			continue;
-		}
-
-		state.enabled = false;
-		ret = pwm_class_apply_state(export, pwm, &state);
-		if (ret < 0) {
-			/*
-			 * roll back the PWM devices that were disabled by
-			 * this suspend function.
-			 */
-			pwm_class_resume_npwm(pwmchip_dev, i);
-			break;
-		}
-	}
-
-	return ret;
-}
-
-static int pwm_class_resume(struct device *pwmchip_dev)
-{
-	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
-
-	return pwm_class_resume_npwm(pwmchip_dev, chip->npwm);
-}
-
-static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume);
-
-static struct class pwm_class = {
-	.name = "pwm",
-	.dev_groups = pwm_chip_groups,
-	.pm = pm_sleep_ptr(&pwm_class_pm_ops),
-};
-
-static int pwmchip_sysfs_match(struct device *pwmchip_dev, const void *data)
-{
-	return pwmchip_from_dev(pwmchip_dev) == data;
-}
-
-void pwmchip_sysfs_export(struct pwm_chip *chip)
-{
-	struct device *pwmchip_dev;
-
-	/*
-	 * If device_create() fails the pwm_chip is still usable by
-	 * the kernel it's just not exported.
-	 */
-	pwmchip_dev = device_create(&pwm_class, pwmchip_parent(chip), MKDEV(0, 0), chip,
-			       "pwmchip%d", chip->id);
-	if (IS_ERR(pwmchip_dev)) {
-		dev_warn(pwmchip_parent(chip),
-			 "device_create failed for pwm_chip sysfs export\n");
-	}
-}
-
-void pwmchip_sysfs_unexport(struct pwm_chip *chip)
-{
-	struct device *pwmchip_dev;
-	unsigned int i;
-
-	pwmchip_dev = class_find_device(&pwm_class, NULL, chip,
-				   pwmchip_sysfs_match);
-	if (!pwmchip_dev)
-		return;
-
-	for (i = 0; i < chip->npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-
-		if (test_bit(PWMF_EXPORTED, &pwm->flags))
-			pwm_unexport_child(pwmchip_dev, pwm);
-	}
-
-	put_device(pwmchip_dev);
-	device_unregister(pwmchip_dev);
-}
-
-static int __init pwm_sysfs_init(void)
-{
-	return class_register(&pwm_class);
-}
-subsys_initcall(pwm_sysfs_init);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 94a642a88817..17e45d8413ed 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -630,17 +630,4 @@ static inline void pwm_remove_table(struct pwm_lookup *table, size_t num)
 }
 #endif
 
-#ifdef CONFIG_PWM_SYSFS
-void pwmchip_sysfs_export(struct pwm_chip *chip);
-void pwmchip_sysfs_unexport(struct pwm_chip *chip);
-#else
-static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
-{
-}
-
-static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip)
-{
-}
-#endif /* CONFIG_PWM_SYSFS */
-
 #endif /* __LINUX_PWM_H */
-- 
2.43.0

[PATCH 4/8] pwm: Ensure a struct pwm has the same lifetime as its pwm_chip

[Uwe Kleine-König]

It's required to not free the memory underlying a requested PWM
while a consumer still has a reference to it. While currently a pwm_chip
doesn't life long enough in all cases, linking the struct pwm to the
pwm_chip results in the right lifetime as soon as the pwmchip is living
long enough. This happens with the following commits.

Note this is a breaking change for all pwm drivers that don't use
pwmchip_alloc().

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> # for struct_size() and __counted_by()
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 27 ++++++++++-----------------
 include/linux/pwm.h |  2 +-
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 69af8123455a..8ed4c93cd0ce 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -987,7 +987,7 @@ static void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 
 static void *pwmchip_priv(struct pwm_chip *chip)
 {
-	return (void *)chip + ALIGN(sizeof(*chip), PWMCHIP_ALIGN);
+	return (void *)chip + ALIGN(struct_size(chip, pwms, chip->npwm), PWMCHIP_ALIGN);
 }
 
 /* This is the counterpart to pwmchip_alloc() */
@@ -1001,8 +1001,10 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 {
 	struct pwm_chip *chip;
 	size_t alloc_size;
+	unsigned int i;
 
-	alloc_size = size_add(ALIGN(sizeof(*chip), PWMCHIP_ALIGN), sizeof_priv);
+	alloc_size = size_add(ALIGN(struct_size(chip, pwms, npwm), PWMCHIP_ALIGN),
+			      sizeof_priv);
 
 	chip = kzalloc(alloc_size, GFP_KERNEL);
 	if (!chip)
@@ -1014,6 +1016,12 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 
 	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
 
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+		pwm->chip = chip;
+		pwm->hwpwm = i;
+	}
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(pwmchip_alloc);
@@ -1085,7 +1093,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
  */
 int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 {
-	unsigned int i;
 	int ret;
 
 	if (!chip || !pwmchip_parent(chip) || !chip->ops || !chip->npwm)
@@ -1105,28 +1112,16 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 
 	chip->owner = owner;
 
-	chip->pwms = kcalloc(chip->npwm, sizeof(*chip->pwms), GFP_KERNEL);
-	if (!chip->pwms)
-		return -ENOMEM;
-
 	mutex_lock(&pwm_lock);
 
 	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		mutex_unlock(&pwm_lock);
-		kfree(chip->pwms);
 		return ret;
 	}
 
 	chip->id = ret;
 
-	for (i = 0; i < chip->npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-
-		pwm->chip = chip;
-		pwm->hwpwm = i;
-	}
-
 	mutex_unlock(&pwm_lock);
 
 	if (IS_ENABLED(CONFIG_OF))
@@ -1156,8 +1151,6 @@ void pwmchip_remove(struct pwm_chip *chip)
 	idr_remove(&pwm_chips, chip->id);
 
 	mutex_unlock(&pwm_lock);
-
-	kfree(chip->pwms);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 17e45d8413ed..78b9061572ff 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -290,7 +290,7 @@ struct pwm_chip {
 	/* only used internally by the PWM framework */
 	bool uses_pwmchip_alloc;
 	void *driver_data;
-	struct pwm_device *pwms;
+	struct pwm_device pwms[] __counted_by(npwm);
 };
 
 static inline struct device *pwmchip_parent(const struct pwm_chip *chip)
-- 
2.43.0

[PATCH 5/8] pwm: Add a struct device to struct pwm_chip

[Uwe Kleine-König]

This replaces the formerly dynamically allocated struct device. This
allows to additionally use it to track the lifetime of the struct
pwm_chip. Otherwise the new struct device provides the same sysfs API as
was provided by the dynamic device before.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 93 +++++++++++++++++++++++++--------------------
 include/linux/pwm.h |  5 ++-
 2 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 8ed4c93cd0ce..09ff6ef0b634 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -343,9 +343,16 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	if (!try_module_get(chip->owner))
 		return -ENODEV;
 
+	if (!get_device(&chip->dev)) {
+		err = -ENODEV;
+		goto err_get_device;
+	}
+
 	if (ops->request) {
 		err = ops->request(chip, pwm);
 		if (err) {
+			put_device(&chip->dev);
+err_get_device:
 			module_put(chip->owner);
 			return err;
 		}
@@ -463,7 +470,7 @@ struct pwm_export {
 
 static inline struct pwm_chip *pwmchip_from_dev(struct device *pwmchip_dev)
 {
-	return dev_get_drvdata(pwmchip_dev);
+	return container_of(pwmchip_dev, struct pwm_chip, dev);
 }
 
 static inline struct pwm_export *pwmexport_from_dev(struct device *pwm_dev)
@@ -941,46 +948,16 @@ static struct class pwm_class = {
 	.pm = pm_sleep_ptr(&pwm_class_pm_ops),
 };
 
-static int pwmchip_sysfs_match(struct device *pwmchip_dev, const void *data)
-{
-	return pwmchip_from_dev(pwmchip_dev) == data;
-}
-
-static void pwmchip_sysfs_export(struct pwm_chip *chip)
-{
-	struct device *pwmchip_dev;
-
-	/*
-	 * If device_create() fails the pwm_chip is still usable by
-	 * the kernel it's just not exported.
-	 */
-	pwmchip_dev = device_create(&pwm_class, pwmchip_parent(chip), MKDEV(0, 0), chip,
-			       "pwmchip%d", chip->id);
-	if (IS_ERR(pwmchip_dev)) {
-		dev_warn(pwmchip_parent(chip),
-			 "device_create failed for pwm_chip sysfs export\n");
-	}
-}
-
 static void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 {
-	struct device *pwmchip_dev;
 	unsigned int i;
 
-	pwmchip_dev = class_find_device(&pwm_class, NULL, chip,
-					pwmchip_sysfs_match);
-	if (!pwmchip_dev)
-		return;
-
 	for (i = 0; i < chip->npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (test_bit(PWMF_EXPORTED, &pwm->flags))
-			pwm_unexport_child(pwmchip_dev, pwm);
+			pwm_unexport_child(&chip->dev, pwm);
 	}
-
-	put_device(pwmchip_dev);
-	device_unregister(pwmchip_dev);
 }
 
 #define PWMCHIP_ALIGN ARCH_DMA_MINALIGN
@@ -993,13 +970,21 @@ static void *pwmchip_priv(struct pwm_chip *chip)
 /* This is the counterpart to pwmchip_alloc() */
 void pwmchip_put(struct pwm_chip *chip)
 {
-	kfree(chip);
+	put_device(&chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_put);
 
+static void pwmchip_release(struct device *pwmchip_dev)
+{
+	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
+
+	kfree(chip);
+}
+
 struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv)
 {
 	struct pwm_chip *chip;
+	struct device *pwmchip_dev;
 	size_t alloc_size;
 	unsigned int i;
 
@@ -1010,10 +995,15 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 	if (!chip)
 		return ERR_PTR(-ENOMEM);
 
-	chip->dev = parent;
 	chip->npwm = npwm;
 	chip->uses_pwmchip_alloc = true;
 
+	pwmchip_dev = &chip->dev;
+	device_initialize(pwmchip_dev);
+	pwmchip_dev->class = &pwm_class;
+	pwmchip_dev->parent = parent;
+	pwmchip_dev->release = pwmchip_release;
+
 	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
 
 	for (i = 0; i < chip->npwm; i++) {
@@ -1115,21 +1105,34 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	mutex_lock(&pwm_lock);
 
 	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
-	if (ret < 0) {
-		mutex_unlock(&pwm_lock);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_idr_alloc;
 
 	chip->id = ret;
 
-	mutex_unlock(&pwm_lock);
+	dev_set_name(&chip->dev, "pwmchip%u", chip->id);
 
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
-	pwmchip_sysfs_export(chip);
+	ret = device_add(&chip->dev);
+	if (ret)
+		goto err_device_add;
+
+	mutex_unlock(&pwm_lock);
 
 	return 0;
+
+err_device_add:
+	if (IS_ENABLED(CONFIG_OF))
+		of_pwmchip_remove(chip);
+
+	idr_remove(&pwm_chips, chip->id);
+err_idr_alloc:
+
+	mutex_unlock(&pwm_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__pwmchip_add);
 
@@ -1151,6 +1154,8 @@ void pwmchip_remove(struct pwm_chip *chip)
 	idr_remove(&pwm_chips, chip->id);
 
 	mutex_unlock(&pwm_lock);
+
+	device_del(&chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
@@ -1520,6 +1525,8 @@ EXPORT_SYMBOL_GPL(pwm_get);
  */
 void pwm_put(struct pwm_device *pwm)
 {
+	struct pwm_chip *chip = pwm->chip;
+
 	if (!pwm)
 		return;
 
@@ -1530,12 +1537,14 @@ void pwm_put(struct pwm_device *pwm)
 		goto out;
 	}
 
-	if (pwm->chip->ops->free)
+	if (chip->ops->free)
 		pwm->chip->ops->free(pwm->chip, pwm);
 
 	pwm->label = NULL;
 
-	module_put(pwm->chip->owner);
+	put_device(&chip->dev);
+
+	module_put(chip->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 78b9061572ff..495e23761f34 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
@@ -277,7 +278,7 @@ struct pwm_ops {
  * @pwms: array of PWM devices allocated by the framework
  */
 struct pwm_chip {
-	struct device *dev;
+	struct device dev;
 	const struct pwm_ops *ops;
 	struct module *owner;
 	unsigned int id;
@@ -295,7 +296,7 @@ struct pwm_chip {
 
 static inline struct device *pwmchip_parent(const struct pwm_chip *chip)
 {
-	return chip->dev;
+	return chip->dev.parent;
 }
 
 static inline void *pwmchip_get_drvdata(struct pwm_chip *chip)
-- 
2.43.0

Re: [PATCH 5/8] pwm: Add a struct device to struct pwm_chip

[David Lechner]

On 3/17/24 5:40 AM, Uwe Kleine-König wrote:

...

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 8ed4c93cd0ce..09ff6ef0b634 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c

...

> @@ -1520,6 +1525,8 @@ EXPORT_SYMBOL_GPL(pwm_get);
>   */
>  void pwm_put(struct pwm_device *pwm)
>  {
> +	struct pwm_chip *chip = pwm->chip;
> +

Should this be moved after the !pwm check to avoid potential null
dereference or is it safe to remove the !pwm check now?

>  	if (!pwm)
>  		return;
>  

Re: [PATCH 5/8] pwm: Add a struct device to struct pwm_chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Thu, Mar 28, 2024 at 01:44:45PM -0500, David Lechner wrote:
> On 3/17/24 5:40 AM, Uwe Kleine-König wrote:
> 
> ...
> 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 8ed4c93cd0ce..09ff6ef0b634 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> 
> ...
> 
> > @@ -1520,6 +1525,8 @@ EXPORT_SYMBOL_GPL(pwm_get);
> >   */
> >  void pwm_put(struct pwm_device *pwm)
> >  {
> > +	struct pwm_chip *chip = pwm->chip;
> > +
> 
> Should this be moved after the !pwm check to avoid potential null
> dereference or is it safe to remove the !pwm check now?

This is indeed on oversight. Thanks for finding and reporting it. I just
sent a patch to address this by delaying the assignment until after the
check below.

See https://lore.kernel.org/r/20240329101648.544155-2-u.kleine-koenig@pengutronix.de

Best regards
Uwe
 
> >  	if (!pwm)
> >  		return;
> >  

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 6/8] pwm: Make pwmchip_[sg]et_drvdata() a wrapper around dev_set_drvdata()

[Uwe Kleine-König]

Now that a pwm_chip has a dedicated struct device, pwmchip_set_drvdata()
and pwmchip_get_drvdata() can be made thin wrappers around
dev_set_drvdata() and dev_get_drvdata() respectively and the previously
needed pointer can be dropped from struct pwm_chip.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/pwm.h | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 495e23761f34..60b92c2c75ef 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -274,7 +274,6 @@ struct pwm_ops {
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @atomic: can the driver's ->apply() be called in atomic context
  * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
- * @driver_data: Private pointer for driver specific info
  * @pwms: array of PWM devices allocated by the framework
  */
 struct pwm_chip {
@@ -290,7 +289,6 @@ struct pwm_chip {
 
 	/* only used internally by the PWM framework */
 	bool uses_pwmchip_alloc;
-	void *driver_data;
 	struct pwm_device pwms[] __counted_by(npwm);
 };
 
@@ -301,20 +299,12 @@ static inline struct device *pwmchip_parent(const struct pwm_chip *chip)
 
 static inline void *pwmchip_get_drvdata(struct pwm_chip *chip)
 {
-	/*
-	 * After pwm_chip got a dedicated struct device, this can be replaced by
-	 * dev_get_drvdata(&chip->dev);
-	 */
-	return chip->driver_data;
+	return dev_get_drvdata(&chip->dev);
 }
 
 static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
 {
-	/*
-	 * After pwm_chip got a dedicated struct device, this can be replaced by
-	 * dev_set_drvdata(&chip->dev, data);
-	 */
-	chip->driver_data = data;
+	dev_set_drvdata(&chip->dev, data);
 }
 
 #if IS_ENABLED(CONFIG_PWM)
-- 
2.43.0

[PATCH 7/8] pwm: Add more locking

[Uwe Kleine-König]

This ensures that a pwm_chip that has no corresponding driver isn't used
and that a driver doesn't go away while a callback is still running.

In the presence of device links this isn't necessary yet (so this is no
fix) but for pwm character device support this is needed.

To not serialize all pwm_apply_state() calls, this introduces a per chip
lock. An additional complication is that for atomic chips a mutex cannot
be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
held while calling an operation for a sleeping chip. So depending on the
chip being atomic or not a spinlock or a mutex is used.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 98 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/pwm.h | 13 ++++++
 2 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 09ff6ef0b634..2e08fcfbe138 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -29,6 +29,22 @@ static DEFINE_MUTEX(pwm_lock);
 
 static DEFINE_IDR(pwm_chips);
 
+static void pwmchip_lock(struct pwm_chip *chip)
+{
+	if (chip->atomic)
+		spin_lock(&chip->atomic_lock);
+	else
+		mutex_lock(&chip->nonatomic_lock);
+}
+
+static void pwmchip_unlock(struct pwm_chip *chip)
+{
+	if (chip->atomic)
+		spin_unlock(&chip->atomic_lock);
+	else
+		mutex_unlock(&chip->nonatomic_lock);
+}
+
 static void pwm_apply_debug(struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
@@ -183,6 +199,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	int err;
+	struct pwm_chip *chip = pwm->chip;
 
 	/*
 	 * Some lowlevel driver's implementations of .apply() make use of
@@ -193,7 +210,13 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 	 */
 	might_sleep();
 
-	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
+	pwmchip_lock(chip);
+	if (!chip->operational) {
+		pwmchip_unlock(chip);
+		return -ENODEV;
+	}
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
 		/*
 		 * Catch any drivers that have been marked as atomic but
 		 * that will sleep anyway.
@@ -205,6 +228,8 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 		err = __pwm_apply(pwm, state);
 	}
 
+	pwmchip_unlock(chip);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
@@ -291,16 +316,26 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
 int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 		unsigned long timeout)
 {
+	struct pwm_chip *chip;
 	int err;
 
-	if (!pwm || !pwm->chip->ops)
+	if (!pwm || !(chip = pwm->chip)->ops)
 		return -EINVAL;
 
-	if (!pwm->chip->ops->capture)
+	if (!chip->ops->capture)
 		return -ENOSYS;
 
 	mutex_lock(&pwm_lock);
-	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
+
+	pwmchip_lock(chip);
+
+	if (chip->operational)
+		err = chip->ops->capture(pwm->chip, pwm, result, timeout);
+	else
+		err = -ENODEV;
+
+	pwmchip_unlock(chip);
+
 	mutex_unlock(&pwm_lock);
 
 	return err;
@@ -340,6 +375,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	if (test_bit(PWMF_REQUESTED, &pwm->flags))
 		return -EBUSY;
 
+	/*
+	 * This function is called while holding pwm_lock. As .operational only
+	 * changes while holding this lock, checking it here without holding the
+	 * chip lock is fine.
+	 */
+	if (!chip->operational)
+		return -ENODEV;
+
 	if (!try_module_get(chip->owner))
 		return -ENODEV;
 
@@ -368,7 +411,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
+		pwmchip_lock(chip);
+
 		err = ops->get_state(chip, pwm, &state);
+
+		pwmchip_unlock(chip);
+
 		trace_pwm_get(pwm, &state, err);
 
 		if (!err)
@@ -997,6 +1045,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 
 	chip->npwm = npwm;
 	chip->uses_pwmchip_alloc = true;
+	chip->operational = false;
 
 	pwmchip_dev = &chip->dev;
 	device_initialize(pwmchip_dev);
@@ -1102,6 +1151,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 
 	chip->owner = owner;
 
+	if (chip->atomic)
+		spin_lock_init(&chip->atomic_lock);
+	else
+		mutex_init(&chip->nonatomic_lock);
+
 	mutex_lock(&pwm_lock);
 
 	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
@@ -1115,6 +1169,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
+	pwmchip_lock(chip);
+	chip->operational = true;
+	pwmchip_unlock(chip);
+
 	ret = device_add(&chip->dev);
 	if (ret)
 		goto err_device_add;
@@ -1124,6 +1182,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	return 0;
 
 err_device_add:
+	pwmchip_lock(chip);
+	chip->operational = false;
+	pwmchip_unlock(chip);
+
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
@@ -1145,12 +1207,27 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
 void pwmchip_remove(struct pwm_chip *chip)
 {
 	pwmchip_sysfs_unexport(chip);
+	unsigned int i;
+
+	mutex_lock(&pwm_lock);
+
+	pwmchip_lock(chip);
+	chip->operational = false;
+	pwmchip_unlock(chip);
+
+	for (i = 0; i < chip->npwm; ++i) {
+		struct pwm_device *pwm = &chip->pwms[i];
+
+		if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+			dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
+			if (pwm->chip->ops->free)
+				pwm->chip->ops->free(pwm->chip, pwm);
+		}
+	}
 
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
-	mutex_lock(&pwm_lock);
-
 	idr_remove(&pwm_chips, chip->id);
 
 	mutex_unlock(&pwm_lock);
@@ -1532,12 +1609,17 @@ void pwm_put(struct pwm_device *pwm)
 
 	mutex_lock(&pwm_lock);
 
-	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+	/*
+	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
+	 * don't warn in this case. This can only happen if a consumer called
+	 * pwm_put() twice.
+	 */
+	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
 		pr_warn("PWM device already freed\n");
 		goto out;
 	}
 
-	if (chip->ops->free)
+	if (chip->operational && chip->ops->free)
 		pwm->chip->ops->free(pwm->chip, pwm);
 
 	pwm->label = NULL;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 60b92c2c75ef..9c84e0ba81a4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -274,6 +274,9 @@ struct pwm_ops {
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @atomic: can the driver's ->apply() be called in atomic context
  * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
+ * @operational: signals if the chip can be used (or is already deregistered)
+ * @nonatomic_lock: mutex for nonatomic chips
+ * @atomic_lock: mutex for atomic chips
  * @pwms: array of PWM devices allocated by the framework
  */
 struct pwm_chip {
@@ -289,6 +292,16 @@ struct pwm_chip {
 
 	/* only used internally by the PWM framework */
 	bool uses_pwmchip_alloc;
+	bool operational;
+	union {
+		/*
+		 * depending on the chip being atomic or not either the mutex or
+		 * the spinlock is used. It protects .operational and
+		 * synchronizes calls to the .ops->apply and .ops->get_state()
+		 */
+		struct mutex nonatomic_lock;
+		struct spinlock atomic_lock;
+	};
 	struct pwm_device pwms[] __counted_by(npwm);
 };
 
-- 
2.43.0

Re: [PATCH 7/8] pwm: Add more locking

[Thorsten Scherer]

Hello Uwe,

On Sun, Mar 17, 2024 at 11:40:38AM +0100, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
> 
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
> 
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be

Nitpick: s/sleem/sleep/

> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c  | 98 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pwm.h | 13 ++++++
>  2 files changed, 103 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 09ff6ef0b634..2e08fcfbe138 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -29,6 +29,22 @@ static DEFINE_MUTEX(pwm_lock);
>  
>  static DEFINE_IDR(pwm_chips);
>  
> +static void pwmchip_lock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_lock(&chip->atomic_lock);
> +	else
> +		mutex_lock(&chip->nonatomic_lock);
> +}
> +
> +static void pwmchip_unlock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_unlock(&chip->atomic_lock);
> +	else
> +		mutex_unlock(&chip->nonatomic_lock);
> +}
> +
>  static void pwm_apply_debug(struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
> @@ -183,6 +199,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>  {
>  	int err;
> +	struct pwm_chip *chip = pwm->chip;
>  
>  	/*
>  	 * Some lowlevel driver's implementations of .apply() make use of
> @@ -193,7 +210,13 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>  	 */
>  	might_sleep();
>  
> -	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
> +	pwmchip_lock(chip);
> +	if (!chip->operational) {
> +		pwmchip_unlock(chip);
> +		return -ENODEV;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
>  		/*
>  		 * Catch any drivers that have been marked as atomic but
>  		 * that will sleep anyway.
> @@ -205,6 +228,8 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>  		err = __pwm_apply(pwm, state);
>  	}
>  
> +	pwmchip_unlock(chip);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
> @@ -291,16 +316,26 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
>  int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>  		unsigned long timeout)
>  {
> +	struct pwm_chip *chip;
>  	int err;
>  
> -	if (!pwm || !pwm->chip->ops)
> +	if (!pwm || !(chip = pwm->chip)->ops)
>  		return -EINVAL;
>  
> -	if (!pwm->chip->ops->capture)
> +	if (!chip->ops->capture)
>  		return -ENOSYS;
>  
>  	mutex_lock(&pwm_lock);
> -	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +
> +	pwmchip_lock(chip);
> +
> +	if (chip->operational)
> +		err = chip->ops->capture(pwm->chip, pwm, result, timeout);
> +	else
> +		err = -ENODEV;
> +
> +	pwmchip_unlock(chip);
> +
>  	mutex_unlock(&pwm_lock);
>  
>  	return err;
> @@ -340,6 +375,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  	if (test_bit(PWMF_REQUESTED, &pwm->flags))
>  		return -EBUSY;
>  
> +	/*
> +	 * This function is called while holding pwm_lock. As .operational only
> +	 * changes while holding this lock, checking it here without holding the
> +	 * chip lock is fine.
> +	 */
> +	if (!chip->operational)
> +		return -ENODEV;
> +
>  	if (!try_module_get(chip->owner))
>  		return -ENODEV;
>  
> @@ -368,7 +411,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  		 */
>  		struct pwm_state state = { 0, };
>  
> +		pwmchip_lock(chip);
> +
>  		err = ops->get_state(chip, pwm, &state);
> +
> +		pwmchip_unlock(chip);
> +
>  		trace_pwm_get(pwm, &state, err);
>  
>  		if (!err)
> @@ -997,6 +1045,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
>  
>  	chip->npwm = npwm;
>  	chip->uses_pwmchip_alloc = true;
> +	chip->operational = false;
>  
>  	pwmchip_dev = &chip->dev;
>  	device_initialize(pwmchip_dev);
> @@ -1102,6 +1151,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  
>  	chip->owner = owner;
>  
> +	if (chip->atomic)
> +		spin_lock_init(&chip->atomic_lock);
> +	else
> +		mutex_init(&chip->nonatomic_lock);
> +
>  	mutex_lock(&pwm_lock);
>  
>  	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
> @@ -1115,6 +1169,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_add(chip);
>  
> +	pwmchip_lock(chip);
> +	chip->operational = true;
> +	pwmchip_unlock(chip);
> +
>  	ret = device_add(&chip->dev);
>  	if (ret)
>  		goto err_device_add;
> @@ -1124,6 +1182,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	return 0;
>  
>  err_device_add:
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_remove(chip);
>  
> @@ -1145,12 +1207,27 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
>  void pwmchip_remove(struct pwm_chip *chip)
>  {
>  	pwmchip_sysfs_unexport(chip);
> +	unsigned int i;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
> +	for (i = 0; i < chip->npwm; ++i) {
> +		struct pwm_device *pwm = &chip->pwms[i];
> +
> +		if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +			dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
> +			if (pwm->chip->ops->free)
> +				pwm->chip->ops->free(pwm->chip, pwm);
> +		}
> +	}
>  
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_remove(chip);
>  
> -	mutex_lock(&pwm_lock);
> -
>  	idr_remove(&pwm_chips, chip->id);
>  
>  	mutex_unlock(&pwm_lock);
> @@ -1532,12 +1609,17 @@ void pwm_put(struct pwm_device *pwm)
>  
>  	mutex_lock(&pwm_lock);
>  
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>  		pr_warn("PWM device already freed\n");
>  		goto out;
>  	}
>  
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>  		pwm->chip->ops->free(pwm->chip, pwm);
>  
>  	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 60b92c2c75ef..9c84e0ba81a4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -274,6 +274,9 @@ struct pwm_ops {
>   * @of_xlate: request a PWM device given a device tree PWM specifier
>   * @atomic: can the driver's ->apply() be called in atomic context
>   * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>   * @pwms: array of PWM devices allocated by the framework
>   */
>  struct pwm_chip {
> @@ -289,6 +292,16 @@ struct pwm_chip {
>  
>  	/* only used internally by the PWM framework */
>  	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()
> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>  	struct pwm_device pwms[] __counted_by(npwm);
>  };
>  
> -- 
> 2.43.0
> 
> 

Best regards
Thorsten

Re: [PATCH 7/8] pwm: Add more locking

[Marek Szyprowski]

Hi Uwe,

On 17.03.2024 11:40, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
>
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
>
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>


This patch landed some time ago in linux-next as commit a740f7879609 
("pwm: Add more locking"). Recently I've finally found that $subject 
patch is responsible for the following lock dep splat observed for some 
time during day-to-day linux-next testing:

======================================================
WARNING: possible circular locking dependency detected
6.9.0-rc1+ #14790 Tainted: G         C
------------------------------------------------------
kworker/u24:4/59 is trying to acquire lock:
ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
pwm_apply_might_sleep+0x90/0xd8

but task is already holding lock:
ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (prepare_lock){+.+.}-{3:3}:
        lock_acquire+0x68/0x84
        __mutex_lock+0xa0/0x840
        mutex_lock_nested+0x24/0x30
        clk_prepare_lock+0x4c/0xa8
        clk_round_rate+0x38/0xd0
        meson_pwm_apply+0x128/0x220 [pwm_meson]
        __pwm_apply+0x64/0x1f8
        pwm_apply_might_sleep+0x58/0xd8
        pwm_regulator_set_voltage+0xbc/0x12c
        _regulator_do_set_voltage+0x110/0x688
        regulator_set_voltage_rdev+0x64/0x25c
        regulator_do_balance_voltage+0x20c/0x47c
        regulator_balance_voltage+0x50/0x9c
        regulator_set_voltage_unlocked+0xa4/0x128
        regulator_set_voltage+0x50/0xec
        _opp_config_regulator_single+0x4c/0x130
        _set_opp+0xfc/0x544
        dev_pm_opp_set_rate+0x190/0x284
        set_target+0x34/0x40
        __cpufreq_driver_target+0x170/0x290
        cpufreq_online+0x814/0xa3c
        cpufreq_add_dev+0x80/0x98
        subsys_interface_register+0xfc/0x118
        cpufreq_register_driver+0x150/0x234
        dt_cpufreq_probe+0x150/0x480
        platform_probe+0x68/0xd8
        really_probe+0xbc/0x2a0
        __driver_probe_device+0x78/0x12c
        driver_probe_device+0xdc/0x164
        __device_attach_driver+0xb8/0x138
        bus_for_each_drv+0x84/0xe0
        __device_attach+0xa8/0x1b0
        device_initial_probe+0x14/0x20
        bus_probe_device+0xb0/0xb4
        deferred_probe_work_func+0x8c/0xc8
        process_one_work+0x220/0x634
        worker_thread+0x268/0x3a8
        kthread+0x124/0x128
        ret_from_fork+0x10/0x20

-> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
        __lock_acquire+0x1318/0x20c4
        lock_acquire.part.0+0xc8/0x20c
        lock_acquire+0x68/0x84
        __mutex_lock+0xa0/0x840
        mutex_lock_nested+0x24/0x30
        pwm_apply_might_sleep+0x90/0xd8
        clk_pwm_prepare+0x54/0x84
        clk_core_prepare+0xc8/0x2f8
        clk_prepare+0x28/0x44
        mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
        mmc_pwrseq_pre_power_on+0x24/0x34
        mmc_power_up.part.0+0x20/0x16c
        mmc_start_host+0xa0/0xac
        mmc_add_host+0x84/0xf0
        meson_mmc_probe+0x318/0x3e8
        platform_probe+0x68/0xd8
        really_probe+0xbc/0x2a0
        __driver_probe_device+0x78/0x12c
        driver_probe_device+0xdc/0x164
        __device_attach_driver+0xb8/0x138
        bus_for_each_drv+0x84/0xe0
        __device_attach_async_helper+0xb0/0xd4
        async_run_entry_fn+0x34/0xe0
        process_one_work+0x220/0x634
        worker_thread+0x268/0x3a8
        kthread+0x124/0x128
        ret_from_fork+0x10/0x20

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(prepare_lock);
                                lock(&chip->nonatomic_lock);
                                lock(prepare_lock);
   lock(&chip->nonatomic_lock);

  *** DEADLOCK ***

4 locks held by kworker/u24:4/59:
  #0: ffff000000220d48 ((wq_completion)async){+.+.}-{0:0}, at: 
process_one_work+0x1a0/0x634
  #1: ffff80008461bde0 ((work_completion)(&entry->work)){+.+.}-{0:0}, 
at: process_one_work+0x1c8/0x634
  #2: ffff0000015bf0f8 (&dev->mutex){....}-{3:3}, at: 
__device_attach_async_helper+0x3c/0xd4
  #3: ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: 
clk_prepare_lock+0x4c/0xa8

stack backtrace:
CPU: 1 PID: 59 Comm: kworker/u24:4 Tainted: G         C 6.9.0-rc1+ #14790
Hardware name: Khadas VIM3 (DT)
Workqueue: async async_run_entry_fn
Call trace:
  dump_backtrace+0x98/0xf0
  show_stack+0x18/0x24
  dump_stack_lvl+0x90/0xd0
  dump_stack+0x18/0x24
  print_circular_bug+0x290/0x370
  check_noncircular+0x15c/0x170
  __lock_acquire+0x1318/0x20c4
  lock_acquire.part.0+0xc8/0x20c
  lock_acquire+0x68/0x84
  __mutex_lock+0xa0/0x840
  mutex_lock_nested+0x24/0x30
  pwm_apply_might_sleep+0x90/0xd8
  clk_pwm_prepare+0x54/0x84
  clk_core_prepare+0xc8/0x2f8
  clk_prepare+0x28/0x44
  mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
  mmc_pwrseq_pre_power_on+0x24/0x34
  mmc_power_up.part.0+0x20/0x16c
  mmc_start_host+0xa0/0xac
  mmc_add_host+0x84/0xf0
  meson_mmc_probe+0x318/0x3e8
  platform_probe+0x68/0xd8
  really_probe+0xbc/0x2a0
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0xdc/0x164
  __device_attach_driver+0xb8/0x138
  bus_for_each_drv+0x84/0xe0
  __device_attach_async_helper+0xb0/0xd4
  async_run_entry_fn+0x34/0xe0
  process_one_work+0x220/0x634
  worker_thread+0x268/0x3a8
  kthread+0x124/0x128
  ret_from_fork+0x10/0x20


I'm not really sure if this warning shows a real problem or just some 
functions are missing lock dep annotations. Quite a lots of subsystems 
are involved in it (clocks, regulators and pwms) and this issue is fully 
reproducible here during system boot on Khadas VIM3 ARM64-based board.


> ---
>   drivers/pwm/core.c  | 98 +++++++++++++++++++++++++++++++++++++++++----
>   include/linux/pwm.h | 13 ++++++
>   2 files changed, 103 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 09ff6ef0b634..2e08fcfbe138 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -29,6 +29,22 @@ static DEFINE_MUTEX(pwm_lock);
>   
>   static DEFINE_IDR(pwm_chips);
>   
> +static void pwmchip_lock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_lock(&chip->atomic_lock);
> +	else
> +		mutex_lock(&chip->nonatomic_lock);
> +}
> +
> +static void pwmchip_unlock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_unlock(&chip->atomic_lock);
> +	else
> +		mutex_unlock(&chip->nonatomic_lock);
> +}
> +
>   static void pwm_apply_debug(struct pwm_device *pwm,
>   			    const struct pwm_state *state)
>   {
> @@ -183,6 +199,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>   int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   {
>   	int err;
> +	struct pwm_chip *chip = pwm->chip;
>   
>   	/*
>   	 * Some lowlevel driver's implementations of .apply() make use of
> @@ -193,7 +210,13 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   	 */
>   	might_sleep();
>   
> -	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
> +	pwmchip_lock(chip);
> +	if (!chip->operational) {
> +		pwmchip_unlock(chip);
> +		return -ENODEV;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
>   		/*
>   		 * Catch any drivers that have been marked as atomic but
>   		 * that will sleep anyway.
> @@ -205,6 +228,8 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   		err = __pwm_apply(pwm, state);
>   	}
>   
> +	pwmchip_unlock(chip);
> +
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
> @@ -291,16 +316,26 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
>   int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>   		unsigned long timeout)
>   {
> +	struct pwm_chip *chip;
>   	int err;
>   
> -	if (!pwm || !pwm->chip->ops)
> +	if (!pwm || !(chip = pwm->chip)->ops)
>   		return -EINVAL;
>   
> -	if (!pwm->chip->ops->capture)
> +	if (!chip->ops->capture)
>   		return -ENOSYS;
>   
>   	mutex_lock(&pwm_lock);
> -	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +
> +	pwmchip_lock(chip);
> +
> +	if (chip->operational)
> +		err = chip->ops->capture(pwm->chip, pwm, result, timeout);
> +	else
> +		err = -ENODEV;
> +
> +	pwmchip_unlock(chip);
> +
>   	mutex_unlock(&pwm_lock);
>   
>   	return err;
> @@ -340,6 +375,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   	if (test_bit(PWMF_REQUESTED, &pwm->flags))
>   		return -EBUSY;
>   
> +	/*
> +	 * This function is called while holding pwm_lock. As .operational only
> +	 * changes while holding this lock, checking it here without holding the
> +	 * chip lock is fine.
> +	 */
> +	if (!chip->operational)
> +		return -ENODEV;
> +
>   	if (!try_module_get(chip->owner))
>   		return -ENODEV;
>   
> @@ -368,7 +411,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   		 */
>   		struct pwm_state state = { 0, };
>   
> +		pwmchip_lock(chip);
> +
>   		err = ops->get_state(chip, pwm, &state);
> +
> +		pwmchip_unlock(chip);
> +
>   		trace_pwm_get(pwm, &state, err);
>   
>   		if (!err)
> @@ -997,6 +1045,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
>   
>   	chip->npwm = npwm;
>   	chip->uses_pwmchip_alloc = true;
> +	chip->operational = false;
>   
>   	pwmchip_dev = &chip->dev;
>   	device_initialize(pwmchip_dev);
> @@ -1102,6 +1151,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   
>   	chip->owner = owner;
>   
> +	if (chip->atomic)
> +		spin_lock_init(&chip->atomic_lock);
> +	else
> +		mutex_init(&chip->nonatomic_lock);
> +
>   	mutex_lock(&pwm_lock);
>   
>   	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
> @@ -1115,6 +1169,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_add(chip);
>   
> +	pwmchip_lock(chip);
> +	chip->operational = true;
> +	pwmchip_unlock(chip);
> +
>   	ret = device_add(&chip->dev);
>   	if (ret)
>   		goto err_device_add;
> @@ -1124,6 +1182,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	return 0;
>   
>   err_device_add:
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> @@ -1145,12 +1207,27 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
>   void pwmchip_remove(struct pwm_chip *chip)
>   {
>   	pwmchip_sysfs_unexport(chip);
> +	unsigned int i;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
> +	for (i = 0; i < chip->npwm; ++i) {
> +		struct pwm_device *pwm = &chip->pwms[i];
> +
> +		if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +			dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
> +			if (pwm->chip->ops->free)
> +				pwm->chip->ops->free(pwm->chip, pwm);
> +		}
> +	}
>   
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> -	mutex_lock(&pwm_lock);
> -
>   	idr_remove(&pwm_chips, chip->id);
>   
>   	mutex_unlock(&pwm_lock);
> @@ -1532,12 +1609,17 @@ void pwm_put(struct pwm_device *pwm)
>   
>   	mutex_lock(&pwm_lock);
>   
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>   		pr_warn("PWM device already freed\n");
>   		goto out;
>   	}
>   
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>   		pwm->chip->ops->free(pwm->chip, pwm);
>   
>   	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 60b92c2c75ef..9c84e0ba81a4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -274,6 +274,9 @@ struct pwm_ops {
>    * @of_xlate: request a PWM device given a device tree PWM specifier
>    * @atomic: can the driver's ->apply() be called in atomic context
>    * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>    * @pwms: array of PWM devices allocated by the framework
>    */
>   struct pwm_chip {
> @@ -289,6 +292,16 @@ struct pwm_chip {
>   
>   	/* only used internally by the PWM framework */
>   	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()
> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>   	struct pwm_device pwms[] __counted_by(npwm);
>   };
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 7/8] pwm: Add more locking

[Marek Szyprowski]

Hi Uwe,

On 17.03.2024 11:40, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
>
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
>
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>


This patch landed some time ago in linux-next as commit a740f7879609 
("pwm: Add more locking"). Recently I've finally found that $subject 
patch is responsible for the following lock dep splat observed for some 
time during day-to-day linux-next testing:

======================================================
WARNING: possible circular locking dependency detected
6.9.0-rc1+ #14790 Tainted: G         C
------------------------------------------------------
kworker/u24:4/59 is trying to acquire lock:
ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
pwm_apply_might_sleep+0x90/0xd8

but task is already holding lock:
ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (prepare_lock){+.+.}-{3:3}:
        lock_acquire+0x68/0x84
        __mutex_lock+0xa0/0x840
        mutex_lock_nested+0x24/0x30
        clk_prepare_lock+0x4c/0xa8
        clk_round_rate+0x38/0xd0
        meson_pwm_apply+0x128/0x220 [pwm_meson]
        __pwm_apply+0x64/0x1f8
        pwm_apply_might_sleep+0x58/0xd8
        pwm_regulator_set_voltage+0xbc/0x12c
        _regulator_do_set_voltage+0x110/0x688
        regulator_set_voltage_rdev+0x64/0x25c
        regulator_do_balance_voltage+0x20c/0x47c
        regulator_balance_voltage+0x50/0x9c
        regulator_set_voltage_unlocked+0xa4/0x128
        regulator_set_voltage+0x50/0xec
        _opp_config_regulator_single+0x4c/0x130
        _set_opp+0xfc/0x544
        dev_pm_opp_set_rate+0x190/0x284
        set_target+0x34/0x40
        __cpufreq_driver_target+0x170/0x290
        cpufreq_online+0x814/0xa3c
        cpufreq_add_dev+0x80/0x98
        subsys_interface_register+0xfc/0x118
        cpufreq_register_driver+0x150/0x234
        dt_cpufreq_probe+0x150/0x480
        platform_probe+0x68/0xd8
        really_probe+0xbc/0x2a0
        __driver_probe_device+0x78/0x12c
        driver_probe_device+0xdc/0x164
        __device_attach_driver+0xb8/0x138
        bus_for_each_drv+0x84/0xe0
        __device_attach+0xa8/0x1b0
        device_initial_probe+0x14/0x20
        bus_probe_device+0xb0/0xb4
        deferred_probe_work_func+0x8c/0xc8
        process_one_work+0x220/0x634
        worker_thread+0x268/0x3a8
        kthread+0x124/0x128
        ret_from_fork+0x10/0x20

-> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
        __lock_acquire+0x1318/0x20c4
        lock_acquire.part.0+0xc8/0x20c
        lock_acquire+0x68/0x84
        __mutex_lock+0xa0/0x840
        mutex_lock_nested+0x24/0x30
        pwm_apply_might_sleep+0x90/0xd8
        clk_pwm_prepare+0x54/0x84
        clk_core_prepare+0xc8/0x2f8
        clk_prepare+0x28/0x44
        mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
        mmc_pwrseq_pre_power_on+0x24/0x34
        mmc_power_up.part.0+0x20/0x16c
        mmc_start_host+0xa0/0xac
        mmc_add_host+0x84/0xf0
        meson_mmc_probe+0x318/0x3e8
        platform_probe+0x68/0xd8
        really_probe+0xbc/0x2a0
        __driver_probe_device+0x78/0x12c
        driver_probe_device+0xdc/0x164
        __device_attach_driver+0xb8/0x138
        bus_for_each_drv+0x84/0xe0
        __device_attach_async_helper+0xb0/0xd4
        async_run_entry_fn+0x34/0xe0
        process_one_work+0x220/0x634
        worker_thread+0x268/0x3a8
        kthread+0x124/0x128
        ret_from_fork+0x10/0x20

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(prepare_lock);
                                lock(&chip->nonatomic_lock);
                                lock(prepare_lock);
   lock(&chip->nonatomic_lock);

  *** DEADLOCK ***

4 locks held by kworker/u24:4/59:
  #0: ffff000000220d48 ((wq_completion)async){+.+.}-{0:0}, at: 
process_one_work+0x1a0/0x634
  #1: ffff80008461bde0 ((work_completion)(&entry->work)){+.+.}-{0:0}, 
at: process_one_work+0x1c8/0x634
  #2: ffff0000015bf0f8 (&dev->mutex){....}-{3:3}, at: 
__device_attach_async_helper+0x3c/0xd4
  #3: ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: 
clk_prepare_lock+0x4c/0xa8

stack backtrace:
CPU: 1 PID: 59 Comm: kworker/u24:4 Tainted: G         C 6.9.0-rc1+ #14790
Hardware name: Khadas VIM3 (DT)
Workqueue: async async_run_entry_fn
Call trace:
  dump_backtrace+0x98/0xf0
  show_stack+0x18/0x24
  dump_stack_lvl+0x90/0xd0
  dump_stack+0x18/0x24
  print_circular_bug+0x290/0x370
  check_noncircular+0x15c/0x170
  __lock_acquire+0x1318/0x20c4
  lock_acquire.part.0+0xc8/0x20c
  lock_acquire+0x68/0x84
  __mutex_lock+0xa0/0x840
  mutex_lock_nested+0x24/0x30
  pwm_apply_might_sleep+0x90/0xd8
  clk_pwm_prepare+0x54/0x84
  clk_core_prepare+0xc8/0x2f8
  clk_prepare+0x28/0x44
  mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
  mmc_pwrseq_pre_power_on+0x24/0x34
  mmc_power_up.part.0+0x20/0x16c
  mmc_start_host+0xa0/0xac
  mmc_add_host+0x84/0xf0
  meson_mmc_probe+0x318/0x3e8
  platform_probe+0x68/0xd8
  really_probe+0xbc/0x2a0
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0xdc/0x164
  __device_attach_driver+0xb8/0x138
  bus_for_each_drv+0x84/0xe0
  __device_attach_async_helper+0xb0/0xd4
  async_run_entry_fn+0x34/0xe0
  process_one_work+0x220/0x634
  worker_thread+0x268/0x3a8
  kthread+0x124/0x128
  ret_from_fork+0x10/0x20


I'm not really sure if this warning shows a real problem or just some 
functions are missing lock dep annotations. Quite a lots of subsystems 
are involved in it (clocks, regulators and pwms) and this issue is fully 
reproducible here during system boot on Khadas VIM3 ARM64-based board.


> ---
>   drivers/pwm/core.c  | 98 +++++++++++++++++++++++++++++++++++++++++----
>   include/linux/pwm.h | 13 ++++++
>   2 files changed, 103 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 09ff6ef0b634..2e08fcfbe138 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -29,6 +29,22 @@ static DEFINE_MUTEX(pwm_lock);
>   
>   static DEFINE_IDR(pwm_chips);
>   
> +static void pwmchip_lock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_lock(&chip->atomic_lock);
> +	else
> +		mutex_lock(&chip->nonatomic_lock);
> +}
> +
> +static void pwmchip_unlock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_unlock(&chip->atomic_lock);
> +	else
> +		mutex_unlock(&chip->nonatomic_lock);
> +}
> +
>   static void pwm_apply_debug(struct pwm_device *pwm,
>   			    const struct pwm_state *state)
>   {
> @@ -183,6 +199,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>   int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   {
>   	int err;
> +	struct pwm_chip *chip = pwm->chip;
>   
>   	/*
>   	 * Some lowlevel driver's implementations of .apply() make use of
> @@ -193,7 +210,13 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   	 */
>   	might_sleep();
>   
> -	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
> +	pwmchip_lock(chip);
> +	if (!chip->operational) {
> +		pwmchip_unlock(chip);
> +		return -ENODEV;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
>   		/*
>   		 * Catch any drivers that have been marked as atomic but
>   		 * that will sleep anyway.
> @@ -205,6 +228,8 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   		err = __pwm_apply(pwm, state);
>   	}
>   
> +	pwmchip_unlock(chip);
> +
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
> @@ -291,16 +316,26 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
>   int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>   		unsigned long timeout)
>   {
> +	struct pwm_chip *chip;
>   	int err;
>   
> -	if (!pwm || !pwm->chip->ops)
> +	if (!pwm || !(chip = pwm->chip)->ops)
>   		return -EINVAL;
>   
> -	if (!pwm->chip->ops->capture)
> +	if (!chip->ops->capture)
>   		return -ENOSYS;
>   
>   	mutex_lock(&pwm_lock);
> -	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +
> +	pwmchip_lock(chip);
> +
> +	if (chip->operational)
> +		err = chip->ops->capture(pwm->chip, pwm, result, timeout);
> +	else
> +		err = -ENODEV;
> +
> +	pwmchip_unlock(chip);
> +
>   	mutex_unlock(&pwm_lock);
>   
>   	return err;
> @@ -340,6 +375,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   	if (test_bit(PWMF_REQUESTED, &pwm->flags))
>   		return -EBUSY;
>   
> +	/*
> +	 * This function is called while holding pwm_lock. As .operational only
> +	 * changes while holding this lock, checking it here without holding the
> +	 * chip lock is fine.
> +	 */
> +	if (!chip->operational)
> +		return -ENODEV;
> +
>   	if (!try_module_get(chip->owner))
>   		return -ENODEV;
>   
> @@ -368,7 +411,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   		 */
>   		struct pwm_state state = { 0, };
>   
> +		pwmchip_lock(chip);
> +
>   		err = ops->get_state(chip, pwm, &state);
> +
> +		pwmchip_unlock(chip);
> +
>   		trace_pwm_get(pwm, &state, err);
>   
>   		if (!err)
> @@ -997,6 +1045,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
>   
>   	chip->npwm = npwm;
>   	chip->uses_pwmchip_alloc = true;
> +	chip->operational = false;
>   
>   	pwmchip_dev = &chip->dev;
>   	device_initialize(pwmchip_dev);
> @@ -1102,6 +1151,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   
>   	chip->owner = owner;
>   
> +	if (chip->atomic)
> +		spin_lock_init(&chip->atomic_lock);
> +	else
> +		mutex_init(&chip->nonatomic_lock);
> +
>   	mutex_lock(&pwm_lock);
>   
>   	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
> @@ -1115,6 +1169,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_add(chip);
>   
> +	pwmchip_lock(chip);
> +	chip->operational = true;
> +	pwmchip_unlock(chip);
> +
>   	ret = device_add(&chip->dev);
>   	if (ret)
>   		goto err_device_add;
> @@ -1124,6 +1182,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	return 0;
>   
>   err_device_add:
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> @@ -1145,12 +1207,27 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
>   void pwmchip_remove(struct pwm_chip *chip)
>   {
>   	pwmchip_sysfs_unexport(chip);
> +	unsigned int i;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
> +	for (i = 0; i < chip->npwm; ++i) {
> +		struct pwm_device *pwm = &chip->pwms[i];
> +
> +		if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +			dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
> +			if (pwm->chip->ops->free)
> +				pwm->chip->ops->free(pwm->chip, pwm);
> +		}
> +	}
>   
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> -	mutex_lock(&pwm_lock);
> -
>   	idr_remove(&pwm_chips, chip->id);
>   
>   	mutex_unlock(&pwm_lock);
> @@ -1532,12 +1609,17 @@ void pwm_put(struct pwm_device *pwm)
>   
>   	mutex_lock(&pwm_lock);
>   
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>   		pr_warn("PWM device already freed\n");
>   		goto out;
>   	}
>   
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>   		pwm->chip->ops->free(pwm->chip, pwm);
>   
>   	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 60b92c2c75ef..9c84e0ba81a4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -274,6 +274,9 @@ struct pwm_ops {
>    * @of_xlate: request a PWM device given a device tree PWM specifier
>    * @atomic: can the driver's ->apply() be called in atomic context
>    * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>    * @pwms: array of PWM devices allocated by the framework
>    */
>   struct pwm_chip {
> @@ -289,6 +292,16 @@ struct pwm_chip {
>   
>   	/* only used internally by the PWM framework */
>   	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()
> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>   	struct pwm_device pwms[] __counted_by(npwm);
>   };
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 7/8] pwm: Add more locking

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5973 bytes --]

Hello Marek,

[Cc += linux-clk@vger.kernel.org, Alexander Stein, linux-arm-kernel@lists.infradead.org]

On Thu, Apr 04, 2024 at 02:09:32PM +0200, Marek Szyprowski wrote:
> On 17.03.2024 11:40, Uwe Kleine-König wrote:
> > This ensures that a pwm_chip that has no corresponding driver isn't used
> > and that a driver doesn't go away while a callback is still running.
> >
> > In the presence of device links this isn't necessary yet (so this is no
> > fix) but for pwm character device support this is needed.
> >
> > To not serialize all pwm_apply_state() calls, this introduces a per chip
> > lock. An additional complication is that for atomic chips a mutex cannot
> > be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> > held while calling an operation for a sleeping chip. So depending on the
> > chip being atomic or not a spinlock or a mutex is used.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> 
> This patch landed some time ago in linux-next as commit a740f7879609 
> ("pwm: Add more locking"). Recently I've finally found that $subject 
> patch is responsible for the following lock dep splat observed for some 
> time during day-to-day linux-next testing:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.9.0-rc1+ #14790 Tainted: G         C
> ------------------------------------------------------
> kworker/u24:4/59 is trying to acquire lock:
> ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
> pwm_apply_might_sleep+0x90/0xd8
> 
> but task is already holding lock:
> ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (prepare_lock){+.+.}-{3:3}:
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         clk_prepare_lock+0x4c/0xa8
>         clk_round_rate+0x38/0xd0
>         meson_pwm_apply+0x128/0x220 [pwm_meson]
>         __pwm_apply+0x64/0x1f8
>         pwm_apply_might_sleep+0x58/0xd8
>         pwm_regulator_set_voltage+0xbc/0x12c
>         _regulator_do_set_voltage+0x110/0x688
>         regulator_set_voltage_rdev+0x64/0x25c
>         regulator_do_balance_voltage+0x20c/0x47c
>         regulator_balance_voltage+0x50/0x9c
>         regulator_set_voltage_unlocked+0xa4/0x128
>         regulator_set_voltage+0x50/0xec
>         _opp_config_regulator_single+0x4c/0x130
>         _set_opp+0xfc/0x544
>         dev_pm_opp_set_rate+0x190/0x284
>         set_target+0x34/0x40
>         __cpufreq_driver_target+0x170/0x290
>         cpufreq_online+0x814/0xa3c
>         cpufreq_add_dev+0x80/0x98
>         subsys_interface_register+0xfc/0x118
>         cpufreq_register_driver+0x150/0x234
>         dt_cpufreq_probe+0x150/0x480
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach+0xa8/0x1b0
>         device_initial_probe+0x14/0x20
>         bus_probe_device+0xb0/0xb4
>         deferred_probe_work_func+0x8c/0xc8
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the meson pwm driver calling clk_set_rate() in the .apply()
callback.

> -> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
>         __lock_acquire+0x1318/0x20c4
>         lock_acquire.part.0+0xc8/0x20c
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         pwm_apply_might_sleep+0x90/0xd8
>         clk_pwm_prepare+0x54/0x84
>         clk_core_prepare+0xc8/0x2f8
>         clk_prepare+0x28/0x44
>         mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
>         mmc_pwrseq_pre_power_on+0x24/0x34
>         mmc_power_up.part.0+0x20/0x16c
>         mmc_start_host+0xa0/0xac
>         mmc_add_host+0x84/0xf0
>         meson_mmc_probe+0x318/0x3e8
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach_async_helper+0xb0/0xd4
>         async_run_entry_fn+0x34/0xe0
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the clk_pwm driver that calls pwm_enable() in its .prepare()
callback. Looking at
arch/arm64/boot/dts/amlogic/meson-g12b-s922x-khadas-vim3.dts the
pwm-clock uses &pwm_ef which is a meson pwm.

This alone only works because clk_prepare_lock() is reentrant for a
single thread (see commit 533ddeb1e86f ("clk: allow reentrant calls into
the clk framework")). (Is this really safe? What does the prepare_lock
actually protect?)

And because of this the lockdep splat is a false positive (as is every
ABBA splat involving the clk prepare_lock).

I think to properly address this, the global prepare_lock should be
replaced by a per-clk lock. This would at least make

	https://lore.kernel.org/all/20231017135436.1241650-1-alexander.stein@ew.tq-group.com/

(which I think is racy and needs a call to clk_rate_exclusive_get())
unnecessary.

Having said that, I don't know how to prevent that lockdep splat with
neither dropping the blamed commit (which is needed for race free
operation in the pwm framework) nor spending hours to rework the locking
of the clk framework.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/8] pwm: Add more locking

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5973 bytes --]

Hello Marek,

[Cc += linux-clk@vger.kernel.org, Alexander Stein, linux-arm-kernel@lists.infradead.org]

On Thu, Apr 04, 2024 at 02:09:32PM +0200, Marek Szyprowski wrote:
> On 17.03.2024 11:40, Uwe Kleine-König wrote:
> > This ensures that a pwm_chip that has no corresponding driver isn't used
> > and that a driver doesn't go away while a callback is still running.
> >
> > In the presence of device links this isn't necessary yet (so this is no
> > fix) but for pwm character device support this is needed.
> >
> > To not serialize all pwm_apply_state() calls, this introduces a per chip
> > lock. An additional complication is that for atomic chips a mutex cannot
> > be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> > held while calling an operation for a sleeping chip. So depending on the
> > chip being atomic or not a spinlock or a mutex is used.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> 
> This patch landed some time ago in linux-next as commit a740f7879609 
> ("pwm: Add more locking"). Recently I've finally found that $subject 
> patch is responsible for the following lock dep splat observed for some 
> time during day-to-day linux-next testing:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.9.0-rc1+ #14790 Tainted: G         C
> ------------------------------------------------------
> kworker/u24:4/59 is trying to acquire lock:
> ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
> pwm_apply_might_sleep+0x90/0xd8
> 
> but task is already holding lock:
> ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (prepare_lock){+.+.}-{3:3}:
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         clk_prepare_lock+0x4c/0xa8
>         clk_round_rate+0x38/0xd0
>         meson_pwm_apply+0x128/0x220 [pwm_meson]
>         __pwm_apply+0x64/0x1f8
>         pwm_apply_might_sleep+0x58/0xd8
>         pwm_regulator_set_voltage+0xbc/0x12c
>         _regulator_do_set_voltage+0x110/0x688
>         regulator_set_voltage_rdev+0x64/0x25c
>         regulator_do_balance_voltage+0x20c/0x47c
>         regulator_balance_voltage+0x50/0x9c
>         regulator_set_voltage_unlocked+0xa4/0x128
>         regulator_set_voltage+0x50/0xec
>         _opp_config_regulator_single+0x4c/0x130
>         _set_opp+0xfc/0x544
>         dev_pm_opp_set_rate+0x190/0x284
>         set_target+0x34/0x40
>         __cpufreq_driver_target+0x170/0x290
>         cpufreq_online+0x814/0xa3c
>         cpufreq_add_dev+0x80/0x98
>         subsys_interface_register+0xfc/0x118
>         cpufreq_register_driver+0x150/0x234
>         dt_cpufreq_probe+0x150/0x480
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach+0xa8/0x1b0
>         device_initial_probe+0x14/0x20
>         bus_probe_device+0xb0/0xb4
>         deferred_probe_work_func+0x8c/0xc8
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the meson pwm driver calling clk_set_rate() in the .apply()
callback.

> -> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
>         __lock_acquire+0x1318/0x20c4
>         lock_acquire.part.0+0xc8/0x20c
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         pwm_apply_might_sleep+0x90/0xd8
>         clk_pwm_prepare+0x54/0x84
>         clk_core_prepare+0xc8/0x2f8
>         clk_prepare+0x28/0x44
>         mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
>         mmc_pwrseq_pre_power_on+0x24/0x34
>         mmc_power_up.part.0+0x20/0x16c
>         mmc_start_host+0xa0/0xac
>         mmc_add_host+0x84/0xf0
>         meson_mmc_probe+0x318/0x3e8
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach_async_helper+0xb0/0xd4
>         async_run_entry_fn+0x34/0xe0
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the clk_pwm driver that calls pwm_enable() in its .prepare()
callback. Looking at
arch/arm64/boot/dts/amlogic/meson-g12b-s922x-khadas-vim3.dts the
pwm-clock uses &pwm_ef which is a meson pwm.

This alone only works because clk_prepare_lock() is reentrant for a
single thread (see commit 533ddeb1e86f ("clk: allow reentrant calls into
the clk framework")). (Is this really safe? What does the prepare_lock
actually protect?)

And because of this the lockdep splat is a false positive (as is every
ABBA splat involving the clk prepare_lock).

I think to properly address this, the global prepare_lock should be
replaced by a per-clk lock. This would at least make

	https://lore.kernel.org/all/20231017135436.1241650-1-alexander.stein@ew.tq-group.com/

(which I think is racy and needs a call to clk_rate_exclusive_get())
unnecessary.

Having said that, I don't know how to prevent that lockdep splat with
neither dropping the blamed commit (which is needed for race free
operation in the pwm framework) nor spending hours to rework the locking
of the clk framework.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] pwm: Add more locking

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5973 bytes --]

Hello Marek,

[Cc += linux-clk@vger.kernel.org, Alexander Stein, linux-arm-kernel@lists.infradead.org]

On Thu, Apr 04, 2024 at 02:09:32PM +0200, Marek Szyprowski wrote:
> On 17.03.2024 11:40, Uwe Kleine-König wrote:
> > This ensures that a pwm_chip that has no corresponding driver isn't used
> > and that a driver doesn't go away while a callback is still running.
> >
> > In the presence of device links this isn't necessary yet (so this is no
> > fix) but for pwm character device support this is needed.
> >
> > To not serialize all pwm_apply_state() calls, this introduces a per chip
> > lock. An additional complication is that for atomic chips a mutex cannot
> > be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> > held while calling an operation for a sleeping chip. So depending on the
> > chip being atomic or not a spinlock or a mutex is used.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> 
> This patch landed some time ago in linux-next as commit a740f7879609 
> ("pwm: Add more locking"). Recently I've finally found that $subject 
> patch is responsible for the following lock dep splat observed for some 
> time during day-to-day linux-next testing:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.9.0-rc1+ #14790 Tainted: G         C
> ------------------------------------------------------
> kworker/u24:4/59 is trying to acquire lock:
> ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
> pwm_apply_might_sleep+0x90/0xd8
> 
> but task is already holding lock:
> ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (prepare_lock){+.+.}-{3:3}:
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         clk_prepare_lock+0x4c/0xa8
>         clk_round_rate+0x38/0xd0
>         meson_pwm_apply+0x128/0x220 [pwm_meson]
>         __pwm_apply+0x64/0x1f8
>         pwm_apply_might_sleep+0x58/0xd8
>         pwm_regulator_set_voltage+0xbc/0x12c
>         _regulator_do_set_voltage+0x110/0x688
>         regulator_set_voltage_rdev+0x64/0x25c
>         regulator_do_balance_voltage+0x20c/0x47c
>         regulator_balance_voltage+0x50/0x9c
>         regulator_set_voltage_unlocked+0xa4/0x128
>         regulator_set_voltage+0x50/0xec
>         _opp_config_regulator_single+0x4c/0x130
>         _set_opp+0xfc/0x544
>         dev_pm_opp_set_rate+0x190/0x284
>         set_target+0x34/0x40
>         __cpufreq_driver_target+0x170/0x290
>         cpufreq_online+0x814/0xa3c
>         cpufreq_add_dev+0x80/0x98
>         subsys_interface_register+0xfc/0x118
>         cpufreq_register_driver+0x150/0x234
>         dt_cpufreq_probe+0x150/0x480
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach+0xa8/0x1b0
>         device_initial_probe+0x14/0x20
>         bus_probe_device+0xb0/0xb4
>         deferred_probe_work_func+0x8c/0xc8
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the meson pwm driver calling clk_set_rate() in the .apply()
callback.

> -> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
>         __lock_acquire+0x1318/0x20c4
>         lock_acquire.part.0+0xc8/0x20c
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         pwm_apply_might_sleep+0x90/0xd8
>         clk_pwm_prepare+0x54/0x84
>         clk_core_prepare+0xc8/0x2f8
>         clk_prepare+0x28/0x44
>         mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
>         mmc_pwrseq_pre_power_on+0x24/0x34
>         mmc_power_up.part.0+0x20/0x16c
>         mmc_start_host+0xa0/0xac
>         mmc_add_host+0x84/0xf0
>         meson_mmc_probe+0x318/0x3e8
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach_async_helper+0xb0/0xd4
>         async_run_entry_fn+0x34/0xe0
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the clk_pwm driver that calls pwm_enable() in its .prepare()
callback. Looking at
arch/arm64/boot/dts/amlogic/meson-g12b-s922x-khadas-vim3.dts the
pwm-clock uses &pwm_ef which is a meson pwm.

This alone only works because clk_prepare_lock() is reentrant for a
single thread (see commit 533ddeb1e86f ("clk: allow reentrant calls into
the clk framework")). (Is this really safe? What does the prepare_lock
actually protect?)

And because of this the lockdep splat is a false positive (as is every
ABBA splat involving the clk prepare_lock).

I think to properly address this, the global prepare_lock should be
replaced by a per-clk lock. This would at least make

	https://lore.kernel.org/all/20231017135436.1241650-1-alexander.stein@ew.tq-group.com/

(which I think is racy and needs a call to clk_rate_exclusive_get())
unnecessary.

Having said that, I don't know how to prevent that lockdep splat with
neither dropping the blamed commit (which is needed for race free
operation in the pwm framework) nor spending hours to rework the locking
of the clk framework.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

With this change each pwmchip can be accessed from userspace via a
character device. Compared to the sysfs-API this is faster (on a
stm32mp157 applying a new configuration takes approx 25% only) and
allows to pass the whole configuration in a single ioctl.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c       | 310 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   2 +
 include/uapi/linux/pwm.h |  23 +++
 3 files changed, 322 insertions(+), 13 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2e08fcfbe138..7f41ab087b98 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -21,6 +21,8 @@
 
 #include <dt-bindings/pwm/pwm.h>
 
+#include <uapi/linux/pwm.h>
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/pwm.h>
 
@@ -249,6 +251,25 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
 
+static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	int ret = -EOPNOTSUPP;
+
+	if (ops->get_state) {
+		pwmchip_lock(chip);
+
+		ret = ops->get_state(chip, pwm, state);
+
+		pwmchip_unlock(chip);
+
+		trace_pwm_get(pwm, state, ret);
+	}
+
+	return ret;
+}
+
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
  * @pwm: PWM device
@@ -411,14 +432,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
-		pwmchip_lock(chip);
-
-		err = ops->get_state(chip, pwm, &state);
-
-		pwmchip_unlock(chip);
-
-		trace_pwm_get(pwm, &state, err);
-
+		err = pwm_get_state_hw(pwm, &state);
 		if (!err)
 			pwm->state = state;
 
@@ -1120,6 +1134,250 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
 	return true;
 }
 
+struct pwm_cdev_data {
+	struct pwm_chip *chip;
+	struct pwm_device *pwm[];
+};
+
+static int pwm_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pwm_chip *chip = container_of(inode->i_cdev, struct pwm_chip, cdev);
+	struct pwm_cdev_data *cdata;
+	int ret;
+
+	mutex_lock(&pwm_lock);
+
+	if (!chip->operational) {
+		ret = -ENXIO;
+		goto out_unlock;
+	}
+
+	cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
+	if (!cdata) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	cdata->chip = chip;
+
+	file->private_data = cdata;
+
+	ret = nonseekable_open(inode, file);
+
+out_unlock:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+
+static int pwm_cdev_release(struct inode *inode, struct file *file)
+{
+	struct pwm_cdev_data *cdata = file->private_data;
+	unsigned int i;
+
+	for (i = 0; i < cdata->chip->npwm; ++i) {
+		if (cdata->pwm[i])
+			pwm_put(cdata->pwm[i]);
+	}
+	kfree(cdata);
+
+	return 0;
+}
+
+static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (!cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = &chip->pwms[hwpwm];
+		int ret;
+
+		ret = pwm_device_request(pwm, "pwm-cdev");
+		if (ret < 0)
+			return ret;
+
+		cdata->pwm[hwpwm] = pwm;
+	}
+
+	return 0;
+}
+
+static int pwm_cdev_free(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = cdata->pwm[hwpwm];
+
+		pwm_put(pwm);
+
+		cdata->pwm[hwpwm] = NULL;
+	}
+
+	return 0;
+}
+
+static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	struct pwm_cdev_data *cdata = file->private_data;
+	struct pwm_chip *chip = cdata->chip;
+
+	mutex_lock(&pwm_lock);
+
+	if (!chip->operational) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	switch (cmd) {
+	case PWM_IOCTL_GET_NUM_PWMS:
+		ret = chip->npwm;
+		break;
+
+	case PWM_IOCTL_REQUEST:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_free(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_GET:
+		{
+			struct pwmchip_state cstate;
+			struct pwm_state state;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cstate, (struct pwmchip_state __user *)arg, sizeof(cstate));
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_request(cdata, cstate.hwpwm);
+			if (ret)
+				goto out_unlock;
+
+			pwm = cdata->pwm[cstate.hwpwm];
+
+			ret = pwm_get_state_hw(pwm, &state);
+			if (ret)
+				goto out_unlock;
+
+			if (state.enabled) {
+				cstate.period = state.period;
+				if (state.polarity == PWM_POLARITY_NORMAL) {
+					cstate.duty_offset = 0;
+					cstate.duty_cycle = state.duty_cycle;
+				} else {
+					cstate.duty_offset = state.duty_cycle;
+					cstate.duty_cycle = state.period - state.duty_cycle;
+				}
+			} else {
+				cstate.period = 0;
+				cstate.duty_cycle = 0;
+				cstate.duty_offset = 0;
+			}
+			ret = copy_to_user((struct pwmchip_state __user *)arg, &cstate, sizeof(cstate));
+		}
+		break;
+
+	case PWM_IOCTL_APPLY:
+		{
+			struct pwmchip_state cstate;
+			struct pwm_state state = { };
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cstate, (struct pwmchip_state __user *)arg, sizeof(cstate));
+			if (ret)
+				goto out_unlock;
+
+			if (cstate.period > 0 &&
+			    (cstate.duty_cycle > cstate.period ||
+			     cstate.duty_offset >= cstate.period)) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
+
+			/*
+			 * While the API provides a duty_offset member
+			 * to describe (among others) also inversed
+			 * polarity wave forms, the translation into the
+			 * traditional representation with a (binary) polarity
+			 * isn't trivial because the lowlevel drivers round
+			 * duty_cycle down when applying a setting and so in the
+			 * representation with duty_offset the rounding is
+			 * inconsistent. I have no idea what's the best way to
+			 * fix that, so to not commit to a solution yet, just
+			 * refuse requests with .duty_offset that would yield
+			 * inversed polarity for now.
+			 */
+			if (cstate.duty_cycle < cstate.period &&
+			    cstate.duty_offset + cstate.duty_cycle >= cstate.period) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
+
+			ret = pwm_cdev_request(cdata, cstate.hwpwm);
+			if (ret)
+				goto out_unlock;
+
+			pwm = cdata->pwm[cstate.hwpwm];
+
+			if (cstate.period > 0) {
+				state.enabled = true;
+				state.period = cstate.period;
+				state.polarity = PWM_POLARITY_NORMAL;
+				state.duty_cycle = cstate.duty_cycle;
+			} else {
+				state.enabled = false;
+			}
+
+			ret = pwm_apply_might_sleep(pwm, &state);
+		}
+		break;
+
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+out_unlock:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+
+static const struct file_operations pwm_cdev_fileops = {
+	.open = pwm_cdev_open,
+	.release = pwm_cdev_release,
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.unlocked_ioctl = pwm_cdev_ioctl,
+};
+
+static dev_t pwm_devt;
+
 /**
  * __pwmchip_add() - register a new PWM chip
  * @chip: the PWM chip to add
@@ -1173,7 +1431,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	chip->operational = true;
 	pwmchip_unlock(chip);
 
-	ret = device_add(&chip->dev);
+	if (chip->id < 256)
+		chip->dev.devt = MKDEV(MAJOR(pwm_devt), chip->id);
+
+	cdev_init(&chip->cdev, &pwm_cdev_fileops);
+	chip->cdev.owner = owner;
+
+	ret = cdev_device_add(&chip->cdev, &chip->dev);
 	if (ret)
 		goto err_device_add;
 
@@ -1232,7 +1496,7 @@ void pwmchip_remove(struct pwm_chip *chip)
 
 	mutex_unlock(&pwm_lock);
 
-	device_del(&chip->dev);
+	cdev_device_del(&chip->cdev, &chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
@@ -1785,9 +2049,29 @@ DEFINE_SEQ_ATTRIBUTE(pwm_debugfs);
 
 static int __init pwm_init(void)
 {
-	if (IS_ENABLED(CONFIG_DEBUG_FS))
-		debugfs_create_file("pwm", 0444, NULL, NULL, &pwm_debugfs_fops);
+	struct dentry *pwm_debugfs = NULL;
+	int ret;
 
-	return class_register(&pwm_class);
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		pwm_debugfs = debugfs_create_file("pwm", 0444, NULL, NULL,
+						  &pwm_debugfs_fops);
+
+	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
+	if (ret) {
+		pr_warn("Failed to initialize chrdev region for PWM usage\n");
+		goto err_alloc_chrdev;
+	}
+
+	ret = class_register(&pwm_class);
+	if (ret) {
+		pr_warn("Failed to initialize PWM class\n");
+
+		unregister_chrdev_region(pwm_devt, 256);
+err_alloc_chrdev:
+
+		debugfs_remove(pwm_debugfs);
+	}
+
+	return ret;
 }
 subsys_initcall(pwm_init);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9c84e0ba81a4..d5fed23b48f0 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
@@ -281,6 +282,7 @@ struct pwm_ops {
  */
 struct pwm_chip {
 	struct device dev;
+	struct cdev cdev;
 	const struct pwm_ops *ops;
 	struct module *owner;
 	unsigned int id;
diff --git a/include/uapi/linux/pwm.h b/include/uapi/linux/pwm.h
new file mode 100644
index 000000000000..ca765bfaa68d
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+
+#ifndef _UAPI_PWM_H_
+#define _UAPI_PWM_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct pwmchip_state {
+	unsigned int hwpwm;
+	__u64 period;
+	__u64 duty_cycle;
+	__u64 duty_offset;
+};
+
+#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
+#define PWM_IOCTL_REQUEST	_IOW(0x75, 1, unsigned int)
+#define PWM_IOCTL_FREE		_IOW(0x75, 2, unsigned int)
+/* reserve nr = 3 for rounding */
+#define PWM_IOCTL_GET		_IOWR(0x75, 4, struct pwmchip_state)
+#define PWM_IOCTL_APPLY		_IOW(0x75, 5, struct pwmchip_state)
+
+#endif /* _UAPI_PWM_H_ */
-- 
2.43.0

Re: [PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[John Ernberg]

Hi Uwe,

Seeing this patch set made me excited.

Did you consider or try a pwm_line model structure, that is connected to a file
descriptor, more like request_line from gpio chardevs?

We have been using gpio chardevs for a while and really benefitted from that
over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
application design more streamlined.

With this patch set I do not see how we can name the PWMs in the device tree
nor during request which is a big benefit with GPIO when we need to support
multiple hardwares. Nor can I see how we would inspect which pins are
allocated or their names when debugging.

Thanks! // John Ernberg

Re: [PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

Hallo,

On Mon, Apr 08, 2024 at 03:42:39PM +0000, John Ernberg wrote:
> Seeing this patch set made me excited.

\o/

> Did you consider or try a pwm_line model structure, that is connected to a file
> descriptor, more like request_line from gpio chardevs?

I'd be open for an extension that simplifies detection of which (chip,
hwpwm) pair corresponds to which usage. It could be fed from a device
tree like:

	&pwm0 {
		compatible = ...;
		reg = <...>;
		#pwm-cells = <3>;
		pwm-names = "motor-control", "backlight", "", "status-led";
	}

and provide a function in libpwm that allows to determine the chipid of
the chip corresponding to &pwm0 (or an open fd to that device) and 3
when "status-led" is searched for.

This could be done completely in userspace I think (by inspecting
sysfs), so I wonder if this could be a complete userspace
implementation. I like keeping the kernel API simple, i.e. let it only
work (as it is now) only using chipid and hwpwm. Hmm, maybe make pwm
names a kernel concept exposed to sysfs to not have to hardcode the dt
details into libpwm?!

> We have been using gpio chardevs for a while and really benefitted from that
> over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
> interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
> application design more streamlined.

libpwm can also fall back to sysfs when the character devices are
missing. If you want to compare the two implementation and create a best
of two merge from it, that would be great.

> With this patch set I do not see how we can name the PWMs in the device tree
> nor during request which is a big benefit with GPIO when we need to support
> multiple hardwares. Nor can I see how we would inspect which pins are
> allocated or their names when debugging.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[John Ernberg]

Hi Uwe,

On 4/9/24 9:49 AM, Uwe Kleine-König wrote:
> Hallo,
> 
> On Mon, Apr 08, 2024 at 03:42:39PM +0000, John Ernberg wrote:
>> Seeing this patch set made me excited.
> 
> \o/
> 
>> Did you consider or try a pwm_line model structure, that is connected to a file
>> descriptor, more like request_line from gpio chardevs?
> 
> I'd be open for an extension that simplifies detection of which (chip,
> hwpwm) pair corresponds to which usage. It could be fed from a device
> tree like:
> 
> 	&pwm0 {
> 		compatible = ...;
> 		reg = <...>;
> 		#pwm-cells = <3>;
> 		pwm-names = "motor-control", "backlight", "", "status-led";
> 	}
> 
> and provide a function in libpwm that allows to determine the chipid of
> the chip corresponding to &pwm0 (or an open fd to that device) and 3
> when "status-led" is searched for.
> 
> This could be done completely in userspace I think (by inspecting
> sysfs), so I wonder if this could be a complete userspace
> implementation. I like keeping the kernel API simple, i.e. let it only
> work (as it is now) only using chipid and hwpwm. Hmm, maybe make pwm
> names a kernel concept exposed to sysfs to not have to hardcode the dt
> details into libpwm?!

Sounds like it could be workable that way. Thanks!

> 
>> We have been using gpio chardevs for a while and really benefitted from that
>> over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
>> interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
>> application design more streamlined.
> 
> libpwm can also fall back to sysfs when the character devices are
> missing. If you want to compare the two implementation and create a best
> of two merge from it, that would be great.
> 
>> With this patch set I do not see how we can name the PWMs in the device tree
>> nor during request which is a big benefit with GPIO when we need to support
>> multiple hardwares. Nor can I see how we would inspect which pins are
>> allocated or their names when debugging.
> 
> Best regards
> Uwe
> 

Best regards // John Ernberg

Re: [PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Kent Gibson]

On Sun, Mar 17, 2024 at 11:40:39AM +0100, Uwe Kleine-König wrote:
> With this change each pwmchip can be accessed from userspace via a
> character device. Compared to the sysfs-API this is faster (on a
> stm32mp157 applying a new configuration takes approx 25% only) and
> allows to pass the whole configuration in a single ioctl.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> new file mode 100644
> index 000000000000..ca765bfaa68d
> --- /dev/null
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +
> +#ifndef _UAPI_PWM_H_
> +#define _UAPI_PWM_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +struct pwmchip_state {
> +	unsigned int hwpwm;
> +	__u64 period;
> +	__u64 duty_cycle;
> +	__u64 duty_offset;
> +};
> +

Sorry for being late to the party, but I only ran across this thread by
accident.

Have you considered the implications of sizing and alignment here[1]?

Change this to:

struct pwmchip_state {
	__u32 hwpwm;
    __u32 __pad;
	__u64 period;
	__u64 duty_cycle;
	__u64 duty_offset;
};

to clarify?

Cheers,
Kent.

[1] https://docs.kernel.org/driver-api/ioctl.html#bit-compat-mode (and
subsequent sections)

Re: [PATCH 8/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

On Tue, May 07, 2024 at 09:11:03AM +0800, Kent Gibson wrote:
> On Sun, Mar 17, 2024 at 11:40:39AM +0100, Uwe Kleine-König wrote:
> > With this change each pwmchip can be accessed from userspace via a
> > character device. Compared to the sysfs-API this is faster (on a
> > stm32mp157 applying a new configuration takes approx 25% only) and
> > allows to pass the whole configuration in a single ioctl.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> > new file mode 100644
> > index 000000000000..ca765bfaa68d
> > --- /dev/null
> > +++ b/include/uapi/linux/pwm.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> > +
> > +#ifndef _UAPI_PWM_H_
> > +#define _UAPI_PWM_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +struct pwmchip_state {
> > +	unsigned int hwpwm;
> > +	__u64 period;
> > +	__u64 duty_cycle;
> > +	__u64 duty_offset;
> > +};
> > +
> 
> Sorry for being late to the party, but I only ran across this thread by
> accident.
> 
> Have you considered the implications of sizing and alignment here[1]?
> 
> Change this to:
> 
> struct pwmchip_state {
> 	__u32 hwpwm;
>     __u32 __pad;
> 	__u64 period;
> 	__u64 duty_cycle;
> 	__u64 duty_offset;
> };
> 
> to clarify?

Good catch. There is some pending rework necessary for the inner
mechanism of the pwm framework that has to be done before the ioctl goes
into the mainline. So you're not (too) late with your concerns.

I'll take care of the explicit padding.

Thanks for your attention,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/8] pwm: Add support for character devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

Hello,

On Sun, Mar 17, 2024 at 11:40:31AM +0100, Uwe Kleine-König wrote:
> After the necessary changes to the lowlevel drivers got in for v6.9-rc1
> here come some changes to the core to implement /dev/pwmchipX character
> devices.
> 
> In my tests on an ARM STM32MP1 programming a PWM using the character
> device is ~4 times faster than just changing duty_cycle via the sysfs
> API. It also has the advantage that (similar to pwm_apply_*) the target
> state is provided to the kernel with a single call, instead of having to
> program the individual settings one after another via sysfs (in the
> right order to not cross states not supported by the driver). 
> 
> Note the representation of a PWM waveform is different here compared to
> the in-kernel representation. A PWM waveform is represented using:
> 
> 	period
> 	duty_cycle
> 	duty_offset
> 
> A disabled PWM is represented by period = 0. For an inversed wave use:
> 
> 	duty_offset = duty_cycle
> 	duty_cycle = period - duty_cycle;
> 
> . However there are some difficulties yet that make it hard to provide a
> consistent API to userspace and so for now duty_offset isn't (fully)
> supported yet. That needs some more consideration and can be added
> later.
> 
> A userspace lib together with some simple test programs making use of
> this new API can be found at
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git
> 
> .
> 
> The start of the series is some cleanup and preparation. The lifetime
> and locking patches are needed to not crash the kernel when a character
> device is open while a lowlevel driver goes away.

This series is already in next for some time, but I'm not sure that I
want to really send it to Linus in the next merge window as there are a
few issues with it:

 - A (false positive) lockdep warning reported by Marek Szyprowski.
   See https://lore.kernel.org/all/5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com

 - A speculation warning flagged by smatch that I don't understand
   completely yet (and failed to attract attention by people that know
   more about about it)
   See https://lore.kernel.org/all/1e3dc81d-dcd4-4b04-85b1-23937e2f0acd@moroto.mountain

 - I'm a bit unhappy about the rounding behaviour. Actually I'd like to
   only provide userspace access via the character device to drivers
   that adhere to the rounding rules for new drivers (that is: First
   pick the maximal period that isn't bigger than the requested period.
   Then for the chosen period pick the maximal duty_cycle that isn't
   bigger than the requested one) to give a consistent behaviour. This
   is further complicated by the fact that the character device exposes
   a more flexible API (involving a duty_offset instead of polarity) and
   the natural extension for the rounding rules with duty_offset is
   different than for inverted polarity configurations.

I currently consider introducing a new callback that in the long run
should replace .apply() and that properly implements the duty_offset
stuff. Then the character device could only be provided for the drivers
implementing .apply2().

I'm open for feedback, e.g. suggestions for a better name for .apply2().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/8] pwm: Add support for character devices

[David Lechner]

On Sat, Apr 13, 2024 at 4:22 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Sun, Mar 17, 2024 at 11:40:31AM +0100, Uwe Kleine-König wrote:
> > After the necessary changes to the lowlevel drivers got in for v6.9-rc1
> > here come some changes to the core to implement /dev/pwmchipX character
> > devices.
> >
> > In my tests on an ARM STM32MP1 programming a PWM using the character
> > device is ~4 times faster than just changing duty_cycle via the sysfs
> > API. It also has the advantage that (similar to pwm_apply_*) the target
> > state is provided to the kernel with a single call, instead of having to
> > program the individual settings one after another via sysfs (in the
> > right order to not cross states not supported by the driver).
> >
> > Note the representation of a PWM waveform is different here compared to
> > the in-kernel representation. A PWM waveform is represented using:
> >
> >       period
> >       duty_cycle
> >       duty_offset
> >
> > A disabled PWM is represented by period = 0. For an inversed wave use:
> >
> >       duty_offset = duty_cycle
> >       duty_cycle = period - duty_cycle;
> >
> > . However there are some difficulties yet that make it hard to provide a
> > consistent API to userspace and so for now duty_offset isn't (fully)
> > supported yet. That needs some more consideration and can be added
> > later.
> >
> > A userspace lib together with some simple test programs making use of
> > this new API can be found at
> >
> >       https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git
> >
> > .
> >
> > The start of the series is some cleanup and preparation. The lifetime
> > and locking patches are needed to not crash the kernel when a character
> > device is open while a lowlevel driver goes away.
>
> This series is already in next for some time, but I'm not sure that I
> want to really send it to Linus in the next merge window as there are a
> few issues with it:
>
>  - A (false positive) lockdep warning reported by Marek Szyprowski.
>    See https://lore.kernel.org/all/5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com
>
>  - A speculation warning flagged by smatch that I don't understand
>    completely yet (and failed to attract attention by people that know
>    more about about it)
>    See https://lore.kernel.org/all/1e3dc81d-dcd4-4b04-85b1-23937e2f0acd@moroto.mountain
>
>  - I'm a bit unhappy about the rounding behaviour. Actually I'd like to
>    only provide userspace access via the character device to drivers
>    that adhere to the rounding rules for new drivers (that is: First
>    pick the maximal period that isn't bigger than the requested period.
>    Then for the chosen period pick the maximal duty_cycle that isn't
>    bigger than the requested one) to give a consistent behaviour. This
>    is further complicated by the fact that the character device exposes
>    a more flexible API (involving a duty_offset instead of polarity) and
>    the natural extension for the rounding rules with duty_offset is
>    different than for inverted polarity configurations.
>
> I currently consider introducing a new callback that in the long run
> should replace .apply() and that properly implements the duty_offset
> stuff. Then the character device could only be provided for the drivers
> implementing .apply2().
>
> I'm open for feedback, e.g. suggestions for a better name for .apply2().
>

Waiting to merge this and giving this some more thought first does
seem like a wise idea.