Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1407717 > unrolled thread

[PATCH] pwm: don't allow duty cycle higher than period

Started byBrian Norris <briannorris@chromium.org>
First post2016-05-26 23:10 +0200
Last post2016-05-27 18:50 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] pwm: don't allow duty cycle higher than period Brian Norris <briannorris@chromium.org> - 2016-05-26 23:10 +0200
    Re: [PATCH] pwm: don't allow duty cycle higher than period Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 09:40 +0200
      Re: [PATCH] pwm: don't allow duty cycle higher than period Brian Norris <briannorris@chromium.org> - 2016-05-27 18:40 +0200
        Re: [PATCH] pwm: don't allow duty cycle higher than period Brian Norris <briannorris@chromium.org> - 2016-05-27 19:00 +0200
      Re: [PATCH] pwm: don't allow duty cycle higher than period Brian Norris <briannorris@chromium.org> - 2016-05-27 18:40 +0200
        Re: [PATCH] pwm: don't allow duty cycle higher than period Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 18:40 +0200
          Re: [PATCH] pwm: don't allow duty cycle higher than period Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 18:50 +0200

#1407717 — [PATCH] pwm: don't allow duty cycle higher than period

FromBrian Norris <briannorris@chromium.org>
Date2016-05-26 23:10 +0200
Subject[PATCH] pwm: don't allow duty cycle higher than period
Message-ID<rDaHw-2Xq-17@gated-at.bofh.it>
It doesn't make sense to allow the duty cycle to be larger than the
period. I can see this behavior by, e.g.:

  # echo 1 > /sys/class/pwm/pwmchip0/export
  # cat /sys/class/pwm/pwmchip0/pwm1/period
  100
  # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
  [... driver may or may not reject the value, or trigger some logic bug ...]

It's better to see:

  # echo 1 > /sys/class/pwm/pwmchip0/export
  # cat /sys/class/pwm/pwmchip0/pwm1/period
  100
  # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
  -bash: echo: write error: Invalid argument

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pwm/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dba3843c53b8..9246b60f894a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -463,6 +463,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 	if (!memcmp(state, &pwm->state, sizeof(*state)))
 		return 0;
 
+	if (state->duty_cycle > state->period)
+		return -EINVAL;
+
 	if (pwm->chip->ops->apply) {
 		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
 		if (err)
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1407876

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2016-05-27 09:40 +0200
Message-ID<rDkxc-uS-15@gated-at.bofh.it>
In reply to#1407717
Hi Brian,

On Thu, 26 May 2016 14:05:30 -0700
Brian Norris <briannorris@chromium.org> wrote:

> It doesn't make sense to allow the duty cycle to be larger than the
> period. I can see this behavior by, e.g.:
> 
>   # echo 1 > /sys/class/pwm/pwmchip0/export
>   # cat /sys/class/pwm/pwmchip0/pwm1/period
>   100
>   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
>   [... driver may or may not reject the value, or trigger some logic bug ...]
> 
> It's better to see:
> 
>   # echo 1 > /sys/class/pwm/pwmchip0/export
>   # cat /sys/class/pwm/pwmchip0/pwm1/period
>   100
>   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
>   -bash: echo: write error: Invalid argument
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/pwm/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dba3843c53b8..9246b60f894a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -463,6 +463,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  	if (!memcmp(state, &pwm->state, sizeof(*state)))
>  		return 0;
>  
> +	if (state->duty_cycle > state->period)
> +		return -EINVAL;
> +

Argh, I forgot to move the pwm_config() checks [1] into
pwm_apply_state() :-/.

I think we should check all the corner cases (see this diff [2]),
once done you can add my

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Thierry, can you include that in your material for 4.7-rc1?

Thanks,

Boris

[1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L443
[2]http://code.bulix.org/wtqja4-99473
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1408174

FromBrian Norris <briannorris@chromium.org>
Date2016-05-27 18:40 +0200
Message-ID<rDsXM-5NX-33@gated-at.bofh.it>
In reply to#1407876
On Fri, May 27, 2016 at 06:38:14PM +0200, Boris Brezillon wrote:
> On Fri, 27 May 2016 09:35:33 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> > On Fri, May 27, 2016 at 09:34:39AM +0200, Boris Brezillon wrote:
> > Now that you mention it, I think you've also dropped some signed
> > (negative value) checking in pwm_config(). I'll squash in your diff +
> > some pwm_config() fixes.
> 
> ->period and ->duty_cycle are unsigned now ;).

Not in pwm_config(). And we don't want to implitly make thos into large
unsigned values.

Brian

[toc] | [prev] | [next] | [standalone]

#1408186

FromBrian Norris <briannorris@chromium.org>
Date2016-05-27 19:00 +0200
Message-ID<rDth7-5UO-11@gated-at.bofh.it>
In reply to#1408174
On Fri, May 27, 2016 at 09:39:43AM -0700, Brian Norris wrote:
> On Fri, May 27, 2016 at 06:38:14PM +0200, Boris Brezillon wrote:
> > On Fri, 27 May 2016 09:35:33 -0700
> > Brian Norris <briannorris@chromium.org> wrote:
> > > On Fri, May 27, 2016 at 09:34:39AM +0200, Boris Brezillon wrote:
> > > Now that you mention it, I think you've also dropped some signed
> > > (negative value) checking in pwm_config(). I'll squash in your diff +
> > > some pwm_config() fixes.
> > 
> > ->period and ->duty_cycle are unsigned now ;).
> 
> Not in pwm_config(). And we don't want to implitly make thos into large

I really can't spell today... two typos in the same sentence!

> unsigned values.

Brian

[toc] | [prev] | [next] | [standalone]

#1408176

FromBrian Norris <briannorris@chromium.org>
Date2016-05-27 18:40 +0200
Message-ID<rDsXL-5NX-27@gated-at.bofh.it>
In reply to#1407876
Hi Boris,

On Fri, May 27, 2016 at 09:34:39AM +0200, Boris Brezillon wrote:
> On Thu, 26 May 2016 14:05:30 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > It doesn't make sense to allow the duty cycle to be larger than the
> > period. I can see this behavior by, e.g.:
> > 
> >   # echo 1 > /sys/class/pwm/pwmchip0/export
> >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> >   100
> >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> >   [... driver may or may not reject the value, or trigger some logic bug ...]
> > 
> > It's better to see:
> > 
> >   # echo 1 > /sys/class/pwm/pwmchip0/export
> >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> >   100
> >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> >   -bash: echo: write error: Invalid argument
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >  drivers/pwm/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dba3843c53b8..9246b60f894a 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -463,6 +463,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> >  	if (!memcmp(state, &pwm->state, sizeof(*state)))
> >  		return 0;
> >  
> > +	if (state->duty_cycle > state->period)
> > +		return -EINVAL;
> > +
> 
> Argh, I forgot to move the pwm_config() checks [1] into
> pwm_apply_state() :-/.

Oh, I didn't actually notice this was a regression.

> I think we should check all the corner cases (see this diff [2]),

Now that you mention it, I think you've also dropped some signed
(negative value) checking in pwm_config(). I'll squash in your diff +
some pwm_config() fixes.

> once done you can add my
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

I'll send v2 without your ack, since I'm going to add a tiny bit extra.
That'll give you a chance to ack the final (?) version.

> Thierry, can you include that in your material for 4.7-rc1?

That sounds like it would be a good idea, IMO. Thanks for noticing this
was a regression! :)

Regards,
Brian

> Thanks,
> 
> Boris
> 
> [1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L443
> [2]http://code.bulix.org/wtqja4-99473
> -- 
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1408179

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2016-05-27 18:40 +0200
Message-ID<rDsXL-5NX-25@gated-at.bofh.it>
In reply to#1408176
On Fri, 27 May 2016 09:35:33 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Hi Boris,
> 
> On Fri, May 27, 2016 at 09:34:39AM +0200, Boris Brezillon wrote:
> > On Thu, 26 May 2016 14:05:30 -0700
> > Brian Norris <briannorris@chromium.org> wrote:
> >   
> > > It doesn't make sense to allow the duty cycle to be larger than the
> > > period. I can see this behavior by, e.g.:
> > > 
> > >   # echo 1 > /sys/class/pwm/pwmchip0/export
> > >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> > >   100
> > >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> > >   [... driver may or may not reject the value, or trigger some logic bug ...]
> > > 
> > > It's better to see:
> > > 
> > >   # echo 1 > /sys/class/pwm/pwmchip0/export
> > >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> > >   100
> > >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> > >   -bash: echo: write error: Invalid argument
> > > 
> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > ---
> > >  drivers/pwm/core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index dba3843c53b8..9246b60f894a 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -463,6 +463,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> > >  	if (!memcmp(state, &pwm->state, sizeof(*state)))
> > >  		return 0;
> > >  
> > > +	if (state->duty_cycle > state->period)
> > > +		return -EINVAL;
> > > +  
> > 
> > Argh, I forgot to move the pwm_config() checks [1] into
> > pwm_apply_state() :-/.  
> 
> Oh, I didn't actually notice this was a regression.
> 
> > I think we should check all the corner cases (see this diff [2]),  
> 
> Now that you mention it, I think you've also dropped some signed
> (negative value) checking in pwm_config(). I'll squash in your diff +
> some pwm_config() fixes.

->period and ->duty_cycle are unsigned now ;).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1408185

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2016-05-27 18:50 +0200
Message-ID<rDt7s-5Rp-21@gated-at.bofh.it>
In reply to#1408179
On Fri, 27 May 2016 18:38:14 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 27 May 2016 09:35:33 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > Hi Boris,
> > 
> > On Fri, May 27, 2016 at 09:34:39AM +0200, Boris Brezillon wrote:  
> > > On Thu, 26 May 2016 14:05:30 -0700
> > > Brian Norris <briannorris@chromium.org> wrote:
> > >     
> > > > It doesn't make sense to allow the duty cycle to be larger than the
> > > > period. I can see this behavior by, e.g.:
> > > > 
> > > >   # echo 1 > /sys/class/pwm/pwmchip0/export
> > > >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> > > >   100
> > > >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> > > >   [... driver may or may not reject the value, or trigger some logic bug ...]
> > > > 
> > > > It's better to see:
> > > > 
> > > >   # echo 1 > /sys/class/pwm/pwmchip0/export
> > > >   # cat /sys/class/pwm/pwmchip0/pwm1/period
> > > >   100
> > > >   # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> > > >   -bash: echo: write error: Invalid argument
> > > > 
> > > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > > ---
> > > >  drivers/pwm/core.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > index dba3843c53b8..9246b60f894a 100644
> > > > --- a/drivers/pwm/core.c
> > > > +++ b/drivers/pwm/core.c
> > > > @@ -463,6 +463,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> > > >  	if (!memcmp(state, &pwm->state, sizeof(*state)))
> > > >  		return 0;
> > > >  
> > > > +	if (state->duty_cycle > state->period)
> > > > +		return -EINVAL;
> > > > +    
> > > 
> > > Argh, I forgot to move the pwm_config() checks [1] into
> > > pwm_apply_state() :-/.    
> > 
> > Oh, I didn't actually notice this was a regression.
> >   
> > > I think we should check all the corner cases (see this diff [2]),    
> > 
> > Now that you mention it, I think you've also dropped some signed
> > (negative value) checking in pwm_config(). I'll squash in your diff +
> > some pwm_config() fixes.  
> 
> ->period and ->duty_cycle are unsigned now ;).  
> 

Never mind, you're right (thought you were talking about checking the
->period and ->duty_cycle values).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web