linux-kernel - Re: [PATCH v7] pwm: Add support for pwmchip devices for faster and easier userspace access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <twtne72vhwnwwyvld6ipwk6ilszgncgfqtbb53ldfl2hhvns7v@f6vmuburt47q>
Date: Thu, 17 Apr 2025 08:41:57 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: linux-pwm@...r.kernel.org, Kent Gibson <warthog618@...il.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] pwm: Add support for pwmchip devices for faster and
 easier userspace access

Hello David,

On Wed, Apr 16, 2025 at 11:29:50AM -0500, David Lechner wrote:
> the passed wf according to hardware constraints. All parameters are
> > + * rounded down to the next possible value, unless there is no such value, then
> > + * values are rounded up. Note that zero isn't considered for rounding down
> > + * period_length_ns.
> > + */
> > +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> 
> Should this return 1 if exact could not be met to match the other functions?

Currently only PWM_IOCTL_SETEXACTWF has that semantic. Adding that
information to PWM_IOCTL_SETROUNDEDWF has to involve calling
.read_waveform + .round_waveform_fromhw which makes it a bit more
expensive, maybe for little gain.

> > +/* Get the currently implemented waveform */
> > +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> > +
> > +/* Like PWM_IOCTL_ROUNDWF + PWM_IOCTL_SETEXACTWF in one go. */
> > +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> > +
> > +/*
> > + * Program the PWM to emit exactly the passed waveform, subject only to rounding
> > + * down each value less than 1 ns. Returns 0 on success, 1 if the waveform
> > + * cannot be implemented exactly, or other negative error codes.
> 
> It doesn't make sense to me that PWM_IOCTL_SETEXACTWF could return 1 meaning
> that the exact request could not be met. Isn't that the point of
> PWM_IOCTL_SETEXACTWF?

Yes it is. Returning 1 is an error indicator and then the HW state isn't
modified.

> To either do exactly as requested (with 1 ns precision)
> or return negative error code without changing the output state?

Returning 1 was my conclusion of our discussion that -EINVAL and -ERANGE
are both bad. The former because there could be other reasons for
-EINVAL during hw register writing and -ERANGE (also) because the
semantic doesn't fit nicely.

So I gave up finding a -ESOMETHING that has both the right semantic and
could be returned without being confused with a different problem during
hardware access. Inventing a new error code seems wrong to me, so only
the positive number range is left.

> It seems like only PWM_IOCTL_SETROUNDEDWF should be able to return 1. My natural
> expectation is that negative error would mean that the hardware output was not
> modified and non-negative value means that hardware output was modified.

I'm also not entirely happy with the semantics, but still I have no idea
for an improvement.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)