linux-kernel - Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

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: <20190205222522.GB1372@mithrandir>
Date:   Tue, 5 Feb 2019 23:25:23 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Fabrice Gasnier <fabrice.gasnier@...com>, jic23@...nel.org,
        robh+dt@...nel.org, mark.rutland@....com, alexandre.torgue@...com,
        mcoquelin.stm32@...il.com, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        vilhelm.gray@...il.com, linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

On Tue, Feb 05, 2019 at 09:47:32PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
> > Add suspend/resume PM sleep ops. When going to low power, disable
> > active PWM channel. Active PWM channel is resumed, by calling
> > pwm_apply_state(). This is inspired by Thierry's comment in [1].
> > Don't touch inactive channels, as it may be used by other LPTimer MFD
> > child driver.
> > [1]https://lkml.org/lkml/2017/12/5/175
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
> > ---
> >  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
> > index 0059b24c..0c40d48 100644
> > --- a/drivers/pwm/pwm-stm32-lp.c
> > +++ b/drivers/pwm/pwm-stm32-lp.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/mfd/stm32-lptimer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> >  
> > @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
> >  	struct pwm_chip chip;
> >  	struct clk *clk;
> >  	struct regmap *regmap;
> > +	struct pwm_state suspend;
> > +	bool suspended;
> >  };
> >  
> >  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
> > @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
> >  	return pwmchip_remove(&priv->chip);
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int stm32_pwm_lp_suspend(struct device *dev)
> > +{
> > +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
> > +
> > +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
> > +	priv->suspended = priv->suspend.enabled;
> > +
> > +	/* safe to call pwm_disable() for already disabled pwm */
> > +	pwm_disable(&priv->chip.pwms[0]);
> > +
> > +	return pinctrl_pm_select_sleep_state(dev);
> 
> IMHO a PWM should not stop if the PWM user didn't call pwm_disable() (or
> pwm_apply_state() with .enabled = false).
> 
> I don't understand all the PM details, but I think there is no defined
> order in which devices are signalled to suspend. If so the PWM might be
> stopped before its consumer. Then the PWM changes state without the
> consumer being aware of that.
> 
> I understand Thierry's position in the link you provided in the commit
> log consistant with my position here.

Agreed, we should let users of the PWM take care of resuming the PWM in
the state and at the point in time that they require so. PWM users will
also likely do a pwm_disable() during their suspend implementation, so
doing this again in a PWM ->suspend() is not necessary, even if perhaps
harmless.

So this leaves only the pinctrl_pm_select_sleep_state() call. That seems
fine, but I'm not sure that that's currently guaranteed to work. I think
we may need to implement device link support in the PWM framework to
guarantee the proper suspend/resume sequencing. Otherwise you may end up
in a situation where the PWM is actually suspended before the user and
glitch the pins before the user has a chance to disable the PWM.

I think it'd be fine to merge the driver change here first before we add
device link support if this works for you. Just mentioning the issue
here in case you ever run into it.

Thierry

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