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

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

[Olliver Schinagl]

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

Olliver

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Olliver Schinagl]

Hey Thierry,

On 29-09-15 09:45, Thierry Reding wrote:
> 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.
Thanks for saving me from doing  alot of work herin ;)

Are Boris his patches merged in some dev tree of yours? I'm working on 
some pwm stuff too and would love to work 'with'.

Olliver
>
> Thierry

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2901 bytes --]

On Thu, Oct 01, 2015 at 08:46:43PM +0200, Olliver Schinagl wrote:
> Hey Thierry,
> 
> On 29-09-15 09:45, Thierry Reding wrote:
> >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.
> Thanks for saving me from doing  alot of work herin ;)
> 
> Are Boris his patches merged in some dev tree of yours? I'm working on some
> pwm stuff too and would love to work 'with'.

I'm hoping to get around to applying Boris' patches to my for-next
branch this week.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]