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

["Nuno Sá" <noname.nuno@gmail.com>]

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á