From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752778AbcFVUlb (ORCPT ); Wed, 22 Jun 2016 16:41:31 -0400 Received: from down.free-electrons.com ([37.187.137.238]:50456 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751231AbcFVUl2 (ORCPT ); Wed, 22 Jun 2016 16:41:28 -0400 Date: Wed, 22 Jun 2016 22:41:14 +0200 From: Boris Brezillon To: Brian Norris Cc: Geert Uytterhoeven , Thierry Reding , Linux PWM List , "linux-kernel@vger.kernel.org" , Brian Norris , Doug Anderson , linux-renesas-soc@vger.kernel.org, Laurent Pinchart Subject: Re: [PATCH v2] pwm: improve args checking in pwm_apply_state() Message-ID: <20160622224114.697c0db5@bbrezillon> In-Reply-To: <20160622191658.GA106497@google.com> References: <1464367549-111530-1-git-send-email-briannorris@chromium.org> <20160621183730.GA130978@google.com> <20160622100422.5c34f975@bbrezillon> <20160622191658.GA106497@google.com> Organization: Free Electrons X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 22 Jun 2016 12:16:59 -0700 Brian Norris wrote: > On Wed, Jun 22, 2016 at 10:04:22AM +0200, Boris Brezillon wrote: > > From 0610f7e24976e176054bce20445ff42d8aea9513 Mon Sep 17 00:00:00 2001 > > From: Boris Brezillon > > Date: Wed, 22 Jun 2016 09:25:14 +0200 > > Subject: [PATCH] pwm: Fix pwm_apply_args() > > > > Commit 5ec803edcb70 ("pwm: Add core infrastructure to allow atomic > > updates"), implemented pwm_disable() as a wrapper around > > pwm_apply_state(), and then, commit ef2bf4997f7d ("pwm: Improve args > > checking in pwm_apply_state()") added missing checks on the ->period > > value in pwm_apply_state() to ensure we were not passing inappropriate > > values to the ->config() or ->apply() methods. > > > > The conjunction of these 2 commits led to a case where pwm_disable() > > was no longer succeeding, thus preventing the polarity setting done > > in pwm_apply_args(). > > > > Set a valid period in pwm_apply_args() to ensure polarity setting > > won't be rejected. > > > > Signed-off-by: Boris Brezillon > > Reported-by: Geert Uytterhoeven > > Suggested-by: Brian Norris > > Fixes: 5ec803edcb70 ("pwm: Add core infrastructure to allow atomic updates") > > --- > > include/linux/pwm.h | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > index 908b67c847cd..c038ae36b10e 100644 > > --- a/include/linux/pwm.h > > +++ b/include/linux/pwm.h > > @@ -464,6 +464,8 @@ static inline bool pwm_can_sleep(struct pwm_device *pwm) > > > > static inline void pwm_apply_args(struct pwm_device *pwm) > > { > > + struct pwm_state state = { }; > > + > > /* > > * PWM users calling pwm_apply_args() expect to have a fresh config > > * where the polarity and period are set according to pwm_args info. > > @@ -476,18 +478,20 @@ static inline void pwm_apply_args(struct pwm_device *pwm) > > * at startup (even if they are actually enabled), thus authorizing > > * polarity setting. > > * > > - * Instead of setting ->enabled to false, we call pwm_disable() > > - * before pwm_set_polarity() to ensure that everything is configured > > - * as expected, and the PWM is really disabled when the user request > > - * it. > > I was confused by this original text when reading it the first time. I > like the replacement text and implementation, as it seems to make more > sense. Well, it should have been done this way from the beginning, but pwm_apply_args() was introduced before the commit introducing the atomic APIs, and I forgot to update it when moving to the atomic approach :-/. > > > + * To fulfill this requirement, we apply a new state which disables > > + * the PWM device and set the reference period and polarity config. > > * > > * Note that PWM users requiring a smooth handover between the > > * bootloader and the kernel (like critical regulators controlled by > > * PWM devices) will have to switch to the atomic API and avoid calling > > * pwm_apply_args(). > > */ > > - pwm_disable(pwm); > > - pwm_set_polarity(pwm, pwm->args.polarity); > > Notably, you're dropping the 'if (!pwm) { }' safety checks that are part > of pwm_disable() and pwm_set_polarity(). But I don't think there should > be any users relying on that. Indeed. I can add it back here if you prefer, but honestly, PWM users that are not checking the value returned by pwm_get() should be considered buggy IMHO, and a NULL pointer exception is a good way to make people realize they are not properly using the API :). > > > + > > + state.enabled = false; > > + state.polarity = pwm->args.polarity; > > + state.period = pwm->args.period; > > + > > + pwm_apply_state(pwm, &state); > > } > > > > struct pwm_lookup { > > Reviewed-by: Brian Norris