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

> > >