[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 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

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.