Re: [RFC] pwm: core: unsigned or signed ints for pwm_config

[Thierry Reding]

On Tue, Sep 29, 2015 at 09:19:27AM +0200, Olliver Schinagl wrote:
> Hey Thierry, list
> 
> I'm going over the pwm core and notice that in the pwm header, duty_ns and
> period_ns is internally stored as an unsigned int.
> 
> struct pwm_device {
>     const char *label;
>     unsigned long flags;
>     unsigned int hwpwm;
>     unsigned int pwm;
>     struct pwm_chip *chip;
>     void *chip_data;
> 
>     unsigned int period;
>     unsigned int duty_cycle;
>     enum pwm_polarity polarity;
> };
> 
> However, pwm_config takes signed ints
> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
> 
> So digging a little deeper in the PWM core, I see that pwm_config dissallows
> negative ints, so having them unsigned has no benefit (and technically is
> illegal)
>     if (!pwm  || duty_ns < 0|| period_ns= 0 || duty_ns > period_ns)
>         return -EINVAL;
> 
> and because (after the check) we cram the signed int into an unsigned one:
> 
>     pwm->duty_cycle = duty_ns;
>     pwm->period = period_ns;
> 
> This could end up badly when using any unsigned int larger then INT_MAX and
> thus ending up with a negative duty/period.

I don't think this is problematic because we're rejecting negative input
values and store the non-negative ones in an unsigned int, so we can
never store anything that would overflow the internal representation.

> I haven't checked deeper if this
> is accounted for later, but would it be worth my time to convert all ints to
> unsigned ints? Since negative period and duty cycles are really not possible
> anyway?

The reason for storing them as unsigned internally is precisely because
they can never be negative. The reason why pwm_config() has plain ints
is historic. It's always been on my TODO list to convert them over to a
unsigned variant, but never high priority enough. It's also problematic
because doing so needs to modify a public API and hence requires
auditing all consumers and providers to make sure nothing breaks.

I'm not sure if it's worth spending this effort now. Boris Brezillon
posted patches a few weeks ago to introduce an "atomic" API and that's
going to require updating all users anyway. The new API also uses the
correct types, so any effort should probably go into testing and
migrating to the new API.

Thierry

Attachment: signature.asc
Description: PGP signature