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
signature.asc
Description: PGP signature
