linux-kernel - Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150817143108.GD6891@ulmo.nvidia.com>
Date:	Mon, 17 Aug 2015 16:31:09 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Jonathan Richardson <jonathar@...adcom.com>
Cc:	Tim Kryger <tim.kryger@...il.com>,
	Dmitry Torokhov <dtor@...gle.com>,
	Anatol Pomazau <anatol@...gle.com>,
	Arun Ramamurthy <arun.ramamurthy@...adcom.com>,
	Scott Branden <sbranden@...adcom.com>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed
 call to enable

On Mon, Jun 15, 2015 at 04:22:27PM -0700, Jonathan Richardson wrote:
> On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> > The pwm_enable function didn't clear the enabled bit if a call to a
> > clients enable function returned an error. The result was that the state
> > of the pwm core was wrong. Clearing the bit when enable returns an error
> > ensures the state is properly set.
> > 
> > Tested-by: Jonathan Richardson <jonathar@...adcom.com>
> > Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > Signed-off-by: Jonathan Richardson <jonathar@...adcom.com>
> > ---
> >  drivers/pwm/core.c  |   19 ++++++++++++++++---
> >  include/linux/pwm.h |    2 ++
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 76b0386..c255267 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >  		pwm->pwm = chip->base + i;
> >  		pwm->hwpwm = i;
> >  		pwm->polarity = polarity;
> > +		mutex_init(&pwm->lock);
> >  
> >  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> >  	}
> > @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> >   */
> >  int pwm_enable(struct pwm_device *pwm)
> >  {
> > -	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> > -		return pwm->chip->ops->enable(pwm->chip, pwm);
> > +	int err = 0;
> >  
> > -	return pwm ? 0 : -EINVAL;
> > +	if (!pwm)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&pwm->lock);
> > +
> > +	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> > +		err = pwm->chip->ops->enable(pwm->chip, pwm);
> > +		if (err)
> > +			clear_bit(PWMF_ENABLED, &pwm->flags);
> > +	}
> > +
> > +	mutex_unlock(&pwm->lock);
> > +
> > +	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(pwm_enable);
> 
> I meant to add the mutex check in disable also, but what about when
> PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?

I think for debugfs we're fine since there's no potential to race there.
It'll simply show the state of the PWM at the point where it was queried
even though that may change immediately after. I suppose we could keep
the lock across the body of the loop just to make sure debugfs will show
a consistent view of the PWM.

For pwm_disable() I don't think we need the lock, since the test_and_
clear_bit() is atomic and ->disable() cannot fail.

As for pwm_set_polarity(), I think it would need to be something like
the below:

---- >8 ----

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3ea3350..8488c7a19bf6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -473,16 +473,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
        if (!pwm->chip->ops->set_polarity)
                return -ENOSYS;
 
-       if (pwm_is_enabled(pwm))
-               return -EBUSY;
+       mutex_lock(&pwm->lock);
+
+       if (pwm_is_enabled(pwm)) {
+               err = -EBUSY;
+               goto unlock;
+       }
 
        err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
        if (err)
-               return err;
+               goto unlock;
 
        pwm->polarity = polarity;
 
-       return 0;
+unlock:
+       mutex_unlock(&pwm->lock);
+       return err;
 }
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)