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

[Uwe Kleine-König]

Hello David,

first of all thanks for your time and valuable feedback!

On Tue, Apr 08, 2025 at 11:20:19AM -0500, David Lechner wrote:
> On 4/8/25 9:23 AM, Uwe Kleine-König wrote:
> > +	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)
> > +				return -EINVAL;
> 
> Since this is get-only (argument is purly output), should we not check this
> to allow userspace to be able to pass an unintialized struct without error?

No, cwf.hwpwm is an input. So I think it's reasonable to assume cwf is
properly initialized.

> > +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> > +			if (IS_ERR(pwm))
> > +				return PTR_ERR(pwm);
> > +
> > +			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;
> 
> Odd to use different style for setting struct here compared to the other cases.
> (I prefer this one since it is less lines of code to read and less indent.)

Note there is a semantic difference:

	cwf = (struct pwmchip_waveform) {
		.period_length_ns = wf.period_length_ns,
		.duty_length_ns = wf.duty_length_ns,
		.duty_offset_ns = wf.duty_offset_ns,
	};

initializes all unspecified members (here e.g. hwpwm) to zero. I used
that idiom for ROUNDWF ioctl with

		.hwpwm = cwf.hwpwm,

I guess I'll update to that variant here, too.

> > +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> > +			if (IS_ERR(pwm))
> > +				return PTR_ERR(pwm);
> > +
> > +			return pwm_set_waveform_might_sleep(pwm, &wf,
> > +							    cmd == PWM_IOCTL_SETEXACTWF);
> 
> For PWM_IOCTL_SETROUNDEDWF case, should we be copying the modifed waveform back
> to userspace so that it can know what rounding what actually applied without having
> to call PWM_IOCTL_GETWF?

Hmm, for pwm_set_waveform_might_sleep() and also pwm_apply_might_sleep()
the argument isn't modified. So while this might save an ioctl for
GETWF, but you might have to rewrite your state instead.

> > @@ -2115,7 +2376,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);
> 
> if (chip->id >= 256 && chip->ops->write_waveform)
> dev_warn("too many PWM devices, chardev will not be created for ...") ?

I would be surprised to hit that, but I guess it's wise to do that
before it happens for the first time.

> > +/*
> > + * Modifies the passed wf according to hardware constraints. All parameters are
> > + * rounded down to the next possible value, unless there is no such value, then
> 
> Technically, isn't 0 a possible value (at least for duty length/offset)?

Yes, but not all hardware's support duty_length == 0 or duty_offset ==
0. For those that do, it's expected that 1 is rounded down to 0 (unless
they support 1, too). period_length isn't supposed to be round down to
0.

> So maybe more clear to say that if the requested value is non-zero then the
> value will be rounded down unless the result would be zero in which case
> the resulting value will the be smallest possible non-zero value.

Yes, this applies only to period however.

All your remarks that I removed will be addressed in the next revision.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature