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

[David Lechner <dlechner@baylibre.com>]

On 9/6/24 10:43 AM, Uwe Kleine-König wrote:
> With this change each pwmchip defining the new-style waveform callbacks
> 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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/core.c       | 267 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/pwm.h      |   3 +
>  include/uapi/linux/pwm.h |  24 ++++
>  3 files changed, 292 insertions(+), 2 deletions(-)
>  create mode 100644 include/uapi/linux/pwm.h
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c7f39f9f4bcf..16615a4673ef 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>
>  
> @@ -1594,6 +1596,254 @@ 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;
> +}

Maybe a potential problem here with multiple requests.

Suppose an applications does:

// main thread
fd = open("/dev/pwm0", ...);

// start some threads

// thread A

ioctl(fd, PWM_IOCTL_REQUEST, 0);
// in kernel, pwm_device_request() is called and
// cdata->pwm[0] is assigned

// does some stuff - OK

	// thread B

	ioctl(fd, PWM_IOCTL_REQUEST, 0);
	// succeeds since cdata->pwm[0] is assigned

	// does some stuff - messes up thread A

// does some stuff - messes up thread B

	// cleans up after itself
	ioctl(fd, PWM_IOCTL_FREE, 0);
	// pwm_put() is called and
	// cdata->pwm[0] is set to NULL
	
// does some stuff - kernel has to call pwm_device_request()
// again, which may fail? e.g. if sysfs stole it in the meantime

// cleans up after itself
ioctl(fd, PWM_IOCTL_FREE, 0);

Maybe we should be more strict and only allow one requester at a time?

Or is it a documentation problem that this isn't the intended way to
use these ioctls?

Or maybe we just don't need the REQUEST and FREE ioctls?

To me, the most intuitive is that REQUEST would gain exclusive
access until FREE is called and other ioctls should fail if
you don't have exclusive access.

> +
> +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_REQUEST:
> +		{
> +			unsigned int hwpwm;
> +
> +			ret = get_user(hwpwm, (unsigned int __user *)arg);
> +			if (ret)
> +				return ret;
> +
> +			return pwm_cdev_request(cdata, hwpwm);
> +		}
> +		break;
> +
> +	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);
> +		}
> +		break;
> +
> +	case PWM_IOCTL_ROUNDWF:
> +		{
> +			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.__pad != 0)
> +				return -EINVAL;
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			wf = (struct pwm_waveform) {
> +				.period_length_ns = cwf.period_length_ns,
> +				.duty_length_ns = cwf.duty_length_ns,
> +				.duty_offset_ns = cwf.duty_offset_ns,
> +			};
> +
> +			ret = pwm_round_waveform_might_sleep(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf = (struct pwmchip_waveform) {
> +				.hwpwm = cwf.hwpwm,
> +				.period_length_ns = wf.period_length_ns,
> +				.duty_length_ns = wf.duty_length_ns,
> +				.duty_offset_ns = wf.duty_offset_ns,
> +			};
> +
> +			return copy_to_user((struct pwmchip_waveform __user *)arg,
> +					    &cwf, sizeof(cwf));
> +		}
> +		break;
> +
> +	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;
> +
> +			if (cwf.__pad != 0) {
> +				pr_warn("huh\n");

This is what I will say when I see this in the kernel log. :-p

> +				return -EINVAL;
> +			}
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			ret = pwm_get_waveform_might_sleep(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf.period_length_ns = wf.period_length_ns;
> +			cwf.duty_length_ns = wf.duty_length_ns;
> +			cwf.duty_offset_ns = wf.duty_offset_ns;
> +
> +			return copy_to_user((struct pwmchip_waveform __user *)arg,
> +					    &cwf, sizeof(cwf));
> +		}
> +		break;