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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]