[3/6] pwm: Add support for pwmchip devices for faster and easier userspace access

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 allowing atomic
application.

Thanks to Randy Dunlap for pointing out a missing kernel-doc
description.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c       | 367 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  24 +++
 3 files changed, 379 insertions(+), 15 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

On 2024-07-08 6:52 a.m., 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 allowing atomic
> application.
>
> Thanks to Randy Dunlap for pointing out a missing kernel-doc
> description.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/core.c       | 367 +++++++++++++++++++++++++++++++++++++--
>   include/linux/pwm.h      |   3 +
>   include/uapi/linux/pwm.h |  24 +++
>   3 files changed, 379 insertions(+), 15 deletions(-)
>   create mode 100644 include/uapi/linux/pwm.h
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 8e68481a7b33..d64c033c4cb2 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -23,6 +23,8 @@
>   
>   #include <dt-bindings/pwm/pwm.h>
>   
> +#include <uapi/linux/pwm.h>
> +
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/pwm.h>
>   
> @@ -95,6 +97,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
>   	}
>   }
>   
> +static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b)
> +{
> +	if (a->period_length > b->period_length)
> +		return 1;
> +
> +	if (a->period_length < b->period_length)
> +		return -1;
> +
> +	if (a->duty_length > b->duty_length)
> +		return 1;
> +
> +	if (a->duty_length < b->duty_length)
> +		return -1;
> +
> +	if (a->duty_offset > b->duty_offset)
> +		return 1;
> +
> +	if (a->duty_offset < b->duty_offset)
> +		return -1;
> +
> +	return 0;
> +}
> +
>   static int pwm_check_rounding(const struct pwm_waveform *wf,
>   			      const struct pwm_waveform *wf_rounded)
>   {
> @@ -115,6 +140,127 @@ static int pwm_check_rounding(const struct pwm_waveform *wf,
>   
>   #define WFHWSIZE 20
>   
> +static int pwm_get_waveform(struct pwm_device *pwm,
> +			    struct pwm_waveform *wf)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	char wfhw[WFHWSIZE];
> +	int err;
> +
> +	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	err = ops->read_waveform(chip, pwm, &wfhw);
> +	if (err)
> +		return err;
> +
> +	return ops->round_waveform_fromhw(chip, pwm, &wfhw, wf);
> +}
> +
> +/* Called with the pwmchip lock held */
> +static int __pwm_set_waveform(struct pwm_device *pwm,
> +			      const struct pwm_waveform *wf,
> +			      bool exact)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	char wfhw[WFHWSIZE];
> +	struct pwm_waveform wf_rounded;
> +	int err;
> +
> +	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> +	if (wf->period_length &&
> +	    (wf->duty_length > wf->period_length ||
> +	     wf->duty_offset >= wf->period_length))
> +		return -EINVAL;
> +
> +	err = ops->round_waveform_tohw(chip, pwm, wf, &wfhw);
> +	if (err)
> +		return err;
> +
> +	if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length) {
> +		err = ops->round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
> +		if (err)
> +			return err;
> +
> +		if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded))
> +			dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
> +				wf->duty_length, wf->period_length, wf->duty_offset,
> +				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
> +
> +		if (exact && pwmwfcmp(wf, &wf_rounded)) {
> +			dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n",
> +				wf->duty_length, wf->period_length, wf->duty_offset,
> +				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
> +
> +			return 1;
> +		}
> +	}
> +
> +	err = ops->write_waveform(chip, pwm, &wfhw);
> +	if (err)
> +		return err;
> +
> +	/* update .state */
> +	pwm_wf2state(wf, &pwm->state);
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length) {
> +		struct pwm_waveform wf_set;
> +
> +		err = ops->read_waveform(chip, pwm, &wfhw);
> +		if (err)
> +			/* maybe ignore? */
> +			return err;
> +
> +		err = ops->round_waveform_fromhw(chip, pwm, &wfhw, &wf_set);
> +		if (err)
> +			/* maybe ignore? */
> +			return err;
> +
> +		if (pwmwfcmp(&wf_set, &wf_rounded) != 0)
> +			dev_err(&chip->dev,
> +				"Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n",
> +				wf->duty_length, wf->period_length, wf->duty_offset,
> +				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset,
> +				wf_set.duty_length, wf_set.period_length, wf_set.duty_offset);
> +	}
> +	return 0;
> +}
> +
> +static int pwm_set_waveform_might_sleep(struct pwm_device *pwm,
> +					struct pwm_waveform *wf, bool exact)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	int err;
> +
> +	might_sleep();
> +
> +	guard(pwmchip)(chip);
> +
> +        if (!chip->operational)
> +                return -ENODEV;
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
> +		/*
> +		 * Catch any drivers that have been marked as atomic but
> +		 * that will sleep anyway.
> +		 */
> +		non_block_start();
> +		err = __pwm_set_waveform(pwm, wf, exact);
> +		non_block_end();
> +	} else {
> +		err = __pwm_set_waveform(pwm, wf, exact);
> +	}
> +
> +	return err;
> +}
> +
>   static void pwm_apply_debug(struct pwm_device *pwm,
>   			    const struct pwm_state *state)
>   {
> @@ -301,19 +447,6 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>   		if (err)
>   			return err;
>   
> -		if (IS_ENABLED(PWM_DEBUG)) {
> -			struct pwm_waveform wf_rounded;
> -
> -			err = ops->round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
> -			if (err)
> -				return err;
> -
> -			if (pwm_check_rounding(&wf, &wf_rounded))
> -				dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
> -					wf.duty_length, wf.period_length, wf.duty_offset,
> -					wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
> -		}
> -
>   		err = ops->write_waveform(chip, pwm, &wfhw);
>   		if (err)
>   			return err;
> @@ -1296,6 +1429,197 @@ 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;
> +
> +	guard(mutex)(&pwm_lock);
> +
> +	if (!chip->operational)
> +		return -ENXIO;
> +
> +	cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
> +	if (!cdata)
> +		return -ENOMEM;
> +
> +	cdata->chip = chip;
> +
> +	file->private_data = cdata;
> +
> +	return nonseekable_open(inode, file);
> +}
> +
> +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;
> +
> +	guard(mutex)(&pwm_lock);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case PWM_IOCTL_GET_NUM_PWMS:
> +		return chip->npwm;
> +
> +	case PWM_IOCTL_REQUEST:
> +		{
> +			unsigned int hwpwm;
> +
> +			ret = get_user(hwpwm, (unsigned int __user *)arg);
> +			if (ret)
> +				return ret;
> +
> +			return pwm_cdev_request(cdata, hwpwm);
> +		}
> +
> +	case PWM_IOCTL_FREE:
> +		{
> +			unsigned int hwpwm;
> +
> +			ret = get_user(hwpwm, (unsigned int __user *)arg);
> +			if (ret)
> +				return ret;
> +
> +			return pwm_cdev_free(cdata, hwpwm);
> +		}
> +
> +	case PWM_IOCTL_GETWF:
> +		{
> +			struct pwmchip_waveform cwf;
> +			struct pwm_waveform wf;
> +			struct pwm_device *pwm;
> +
> +			ret = copy_from_user(&cwf, (struct pwmchip_waveform __user *)arg, sizeof(cwf));
> +			if (ret)
> +				return -EFAULT;
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			ret = pwm_get_waveform(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf.period_length = wf.period_length;
> +			cwf.duty_length = wf.duty_length;
> +			cwf.duty_offset = wf.duty_offset;
> +
> +			return copy_to_user((struct pwmchip_waveform __user *)arg, &cwf, sizeof(cwf));
> +		}
> +		break;
> +
> +	case PWM_IOCTL_SETROUNDEDWF:
> +	case PWM_IOCTL_SETEXACTWF:
> +		{
> +			struct pwmchip_waveform cwf;
> +			struct pwm_waveform wf;
> +			struct pwm_device *pwm;
> +
> +			ret = copy_from_user(&cwf, (struct pwmchip_waveform __user *)arg, sizeof(cwf));
> +			if (ret)
> +				return -EFAULT;
> +
> +			if (cwf.period_length > 0 &&
> +			    (cwf.duty_length > cwf.period_length ||
> +			     cwf.duty_offset >= cwf.period_length))
> +				return -EINVAL;
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			wf = (struct pwm_waveform){
> +				.period_length = cwf.period_length,
> +				.duty_length = cwf.duty_length,
> +				.duty_offset = cwf.duty_offset,
> +			};
> +
> +			return pwm_set_waveform_might_sleep(pwm, &wf, cmd == PWM_IOCTL_SETEXACTWF);
> +		}
> +		break;
> +
> +	default:
> +		return -ENOTTY;
> +	}
> +}
> +
> +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
> @@ -1348,7 +1672,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	scoped_guard(pwmchip, chip)
>   		chip->operational = true;
>   
> -	ret = device_add(&chip->dev);
> +	if (chip->id < 256 && chip->ops->write_waveform)
> +		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;
>   
> @@ -1399,7 +1729,7 @@ void pwmchip_remove(struct pwm_chip *chip)
>   		idr_remove(&pwm_chips, chip->id);
>   	}
>   
> -	device_del(&chip->dev);
> +	cdev_device_del(&chip->cdev, &chip->dev);
>   }
>   EXPORT_SYMBOL_GPL(pwmchip_remove);
>   
> @@ -1943,9 +2273,16 @@ static int __init pwm_init(void)
>   {
>   	int ret;
>   
> +	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
> +	if (ret) {
> +		pr_warn("Failed to initialize chrdev region for PWM usage\n");
> +		return ret;
> +	}
> +
>   	ret = class_register(&pwm_class);
>   	if (ret) {
>   		pr_err("Failed to initialize PWM class (%pe)\n", ERR_PTR(ret));
> +		unregister_chrdev_region(pwm_devt, 256);
>   		return ret;
>   	}
>   
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index b5dff2a99038..3e503a28f5f7 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/module.h>
> @@ -303,6 +304,7 @@ struct pwm_ops {
>   /**
>    * struct pwm_chip - abstract a PWM controller
>    * @dev: device providing the PWMs
> + * @cdev: &struct cdev for this device
>    * @ops: callbacks for this PWM controller
>    * @owner: module providing this chip
>    * @id: unique number of this PWM chip
> @@ -317,6 +319,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..1ecf2e033b62
> --- /dev/null
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,24 @@
> +/* 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_waveform {
> +	unsigned int hwpwm;
> +	__u64 period_length;
> +	__u64 duty_length;
> +	__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)
> +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> +
> +#endif /* _UAPI_PWM_H_ */

On Mon, 2024-07-08 at 12:52 +0200, 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 allowing atomic
> application.
> 
> Thanks to Randy Dunlap for pointing out a missing kernel-doc
> description.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

I didn't looked very carefully at the patch but one thing did caught my
attention

...

> +
> +struct pwmchip_waveform {
> +	unsigned int hwpwm;
> +	__u64 period_length;
> +	__u64 duty_length;
> +	__u64 duty_offset;
> +};
> +

I do not think we should have holes in the struct given this is an userspace
interface.

One other thing is how likely is this struct to grow? If that is expected we
should probably think in adding some __reserved__ parameters or maybe to modify
the interface so we could make use of:

https://elixir.bootlin.com/linux/latest/source/include/linux/uaccess.h#L348

Like wrapping struct pwmchip_waveform in another struct with an extra member
forcing userspace to specify pwmchip_waveform size. But I agree it's a bit
awkward and ugly (but it could be hidden in libpwm).

Anyways, if this is not likely to change disregard all the compatibility
thingy...

- Nuno Sá

On Tue, Jul 09, 2024 at 11:37:13AM +0200, Nuno Sá wrote:
> On Mon, 2024-07-08 at 12:52 +0200, 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 allowing atomic
> > application.
> > 
> > Thanks to Randy Dunlap for pointing out a missing kernel-doc
> > description.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
> 
> I didn't looked very carefully at the patch but one thing did caught my
> attention
> 
> ...
> 
> > +
> > +struct pwmchip_waveform {
> > +	unsigned int hwpwm;
> > +	__u64 period_length;
> > +	__u64 duty_length;
> > +	__u64 duty_offset;
> > +};
> > +
> 
> I do not think we should have holes in the struct given this is an userspace
> interface.

Ack, will add explicit padding (and a check that it is zeroed).

> One other thing is how likely is this struct to grow?

I don't expect it to grow. Extensions I could imagine only concern
things like:

 - request the currently running period to be completed
 - block until the hardware is programmed

and these don't fit into pwmchip_waveform and would require a different
ioctl command and parameter struct anyhow.

> If that is expected we should probably think in adding some
> __reserved__ parameters or maybe to modify the interface so we could
> make use of:
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/uaccess.h#L348
> 
> Like wrapping struct pwmchip_waveform in another struct with an extra member
> forcing userspace to specify pwmchip_waveform size. But I agree it's a bit
> awkward and ugly (but it could be hidden in libpwm).

The size is already encoded in the ioctl request constants. So I think
we're set to use copy_struct_from_user() if my expectation about
pwmchip_waveform not growing turns out to be wrong.

Best regards and thanks for your feedback,
Uwe

On Fri, 2024-07-12 at 11:48 +0200, Uwe Kleine-König wrote:
> On Tue, Jul 09, 2024 at 11:37:13AM +0200, Nuno Sá wrote:
> > On Mon, 2024-07-08 at 12:52 +0200, 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 allowing atomic
> > > application.
> > > 
> > > Thanks to Randy Dunlap for pointing out a missing kernel-doc
> > > description.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > ---
> > 
> > I didn't looked very carefully at the patch but one thing did caught my
> > attention
> > 
> > ...
> > 
> > > +
> > > +struct pwmchip_waveform {
> > > +	unsigned int hwpwm;
> > > +	__u64 period_length;
> > > +	__u64 duty_length;
> > > +	__u64 duty_offset;
> > > +};
> > > +
> > 
> > I do not think we should have holes in the struct given this is an userspace
> > interface.
> 
> Ack, will add explicit padding (and a check that it is zeroed).
> 

Why not having the __u64 coming first :)? It also save you 4 bytes (yeah, should
not make a difference)

> > One other thing is how likely is this struct to grow?
> 
> I don't expect it to grow. Extensions I could imagine only concern
> things like:
> 
>  - request the currently running period to be completed
>  - block until the hardware is programmed
> 
> and these don't fit into pwmchip_waveform and would require a different
> ioctl command and parameter struct anyhow.
> 
> > If that is expected we should probably think in adding some
> > __reserved__ parameters or maybe to modify the interface so we could
> > make use of:
> > 
> > https://elixir.bootlin.com/linux/latest/source/include/linux/uaccess.h#L348
> > 
> > Like wrapping struct pwmchip_waveform in another struct with an extra member
> > forcing userspace to specify pwmchip_waveform size. But I agree it's a bit
> > awkward and ugly (but it could be hidden in libpwm).
> 
> The size is already encoded in the ioctl request constants. So I think
> we're set to use copy_struct_from_user() if my expectation about
> pwmchip_waveform not growing turns out to be wrong.
> 

Oh, indeed. I had to go and remember the IO* macros...

- Nuno Sá

On Fri, Jul 12, 2024 at 01:01:04PM +0200, Nuno Sá wrote:
> On Fri, 2024-07-12 at 11:48 +0200, Uwe Kleine-König wrote:
> > On Tue, Jul 09, 2024 at 11:37:13AM +0200, Nuno Sá wrote:
> > > On Mon, 2024-07-08 at 12:52 +0200, 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 allowing atomic
> > > > application.
> > > > 
> > > > Thanks to Randy Dunlap for pointing out a missing kernel-doc
> > > > description.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > > ---
> > > 
> > > I didn't looked very carefully at the patch but one thing did caught my
> > > attention
> > > 
> > > ...
> > > 
> > > > +
> > > > +struct pwmchip_waveform {
> > > > +	unsigned int hwpwm;
> > > > +	__u64 period_length;
> > > > +	__u64 duty_length;
> > > > +	__u64 duty_offset;
> > > > +};
> > > > +
> > > 
> > > I do not think we should have holes in the struct given this is an userspace
> > > interface.
> > 
> > Ack, will add explicit padding (and a check that it is zeroed).
> > 
> 
> Why not having the __u64 coming first :)? It also save you 4 bytes (yeah, should
> not make a difference)

Well no. First conceptually hwpwm should come first and second there is
https://www.kernel.org/doc/html/latest/process/botching-up-ioctls.html
which recommends:

	Align everything to the natural size and use explicit padding.
	32-bit platforms don’t necessarily align 64-bit values to 64-bit
	boundaries, but 64-bit platforms do. So we always need padding
	to the natural size to get this right.

> > > If that is expected we should probably think in adding some
> > > __reserved__ parameters or maybe to modify the interface so we could
> > > make use of:
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/uaccess.h#L348
> > > 
> > > Like wrapping struct pwmchip_waveform in another struct with an extra member
> > > forcing userspace to specify pwmchip_waveform size. But I agree it's a bit
> > > awkward and ugly (but it could be hidden in libpwm).
> > 
> > The size is already encoded in the ioctl request constants. So I think
> > we're set to use copy_struct_from_user() if my expectation about
> > pwmchip_waveform not growing turns out to be wrong.
> > 
> 
> Oh, indeed. I had to go and remember the IO* macros...

I did that recently and fixed a thing I considered wrong/unintuitive:
https://lore.kernel.org/all/20240712093524.905556-2-u.kleine-koenig@baylibre.com/

Best regards
Uwe

On Fri, 2024-07-12 at 18:28 +0200, Uwe Kleine-König wrote:
> On Fri, Jul 12, 2024 at 01:01:04PM +0200, Nuno Sá wrote:
> > On Fri, 2024-07-12 at 11:48 +0200, Uwe Kleine-König wrote:
> > > On Tue, Jul 09, 2024 at 11:37:13AM +0200, Nuno Sá wrote:
> > > > On Mon, 2024-07-08 at 12:52 +0200, 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 allowing atomic
> > > > > application.
> > > > > 
> > > > > Thanks to Randy Dunlap for pointing out a missing kernel-doc
> > > > > description.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > > > ---
> > > > 
> > > > I didn't looked very carefully at the patch but one thing did caught my
> > > > attention
> > > > 
> > > > ...
> > > > 
> > > > > +
> > > > > +struct pwmchip_waveform {
> > > > > +	unsigned int hwpwm;
> > > > > +	__u64 period_length;
> > > > > +	__u64 duty_length;
> > > > > +	__u64 duty_offset;
> > > > > +};
> > > > > +
> > > > 
> > > > I do not think we should have holes in the struct given this is an userspace
> > > > interface.
> > > 
> > > Ack, will add explicit padding (and a check that it is zeroed).
> > > 
> > 
> > Why not having the __u64 coming first :)? It also save you 4 bytes (yeah, should
> > not make a difference)
> 
> Well no. First conceptually hwpwm should come first and second there is
> https://www.kernel.org/doc/html/latest/process/botching-up-ioctls.html
> which recommends:
> 
> 	Align everything to the natural size and use explicit padding.
> 	32-bit platforms don’t necessarily align 64-bit values to 64-bit
> 	boundaries, but 64-bit platforms do. So we always need padding
> 	to the natural size to get this right.
> 

Right, my bad! The third point about struct sizes is also valid and would fail with
my suggestion.

Thanks!
- Nuno Sá

> > >