From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754375AbbJPS1t (ORCPT ); Fri, 16 Oct 2015 14:27:49 -0400 Received: from mail-gw2-out.broadcom.com ([216.31.210.63]:60852 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752860AbbJPS1r (ORCPT ); Fri, 16 Oct 2015 14:27:47 -0400 X-IronPort-AV: E=Sophos;i="5.17,689,1437462000"; d="scan'208";a="77867090" Message-ID: <5621419E.1050806@broadcom.com> Date: Fri, 16 Oct 2015 11:27:42 -0700 From: Jonathan Richardson User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Thierry Reding CC: Tim Kryger , Dmitry Torokhov , Anatol Pomazau , Arun Ramamurthy , Scott Branden , bcm-kernel-feedback-list , , Subject: Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable References: <1434403262-24198-1-git-send-email-jonathar@broadcom.com> <1434403262-24198-3-git-send-email-jonathar@broadcom.com> <557F5E33.2050706@broadcom.com> <20150817143108.GD6891@ulmo.nvidia.com> In-Reply-To: <20150817143108.GD6891@ulmo.nvidia.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15-08-17 07:31 AM, Thierry Reding wrote: > 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 >>> Reviewed-by: Dmitry Torokhov >>> Signed-off-by: Jonathan Richardson >>> --- >>> 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); > > Thierry, Sounds good to me. I'll send a patch out for this hopefully today. I don't see a need to complicate debugfs with obscure functionality so the patch will just add polarity as you have shown it here and the enable routine as the previous patchset. Jon From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Richardson Subject: Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable Date: Fri, 16 Oct 2015 11:27:42 -0700 Message-ID: <5621419E.1050806@broadcom.com> References: <1434403262-24198-1-git-send-email-jonathar@broadcom.com> <1434403262-24198-3-git-send-email-jonathar@broadcom.com> <557F5E33.2050706@broadcom.com> <20150817143108.GD6891@ulmo.nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150817143108.GD6891@ulmo.nvidia.com> Sender: linux-kernel-owner@vger.kernel.org To: Thierry Reding Cc: Tim Kryger , Dmitry Torokhov , Anatol Pomazau , Arun Ramamurthy , Scott Branden , bcm-kernel-feedback-list , linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org List-Id: linux-pwm@vger.kernel.org On 15-08-17 07:31 AM, Thierry Reding wrote: > 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 >>> Reviewed-by: Dmitry Torokhov >>> Signed-off-by: Jonathan Richardson >>> --- >>> 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); > > Thierry, Sounds good to me. I'll send a patch out for this hopefully today. I don't see a need to complicate debugfs with obscure functionality so the patch will just add polarity as you have shown it here and the enable routine as the previous patchset. Jon