| 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
| ||
|
Date: Mon, 09 Jan 2017 19:14:43 -0800
From: Stefan Agner <stefan@...er.ch>
To: Boris Brezillon <boris.brezillon@...e-electrons.com>,
Lukasz Majewski <lukma@...x.de>
Cc: Lukasz Majewski <lukma@...x.de>,
Thierry Reding <thierry.reding@...il.com>,
Sascha Hauer <s.hauer@...gutronix.de>,
linux-pwm@...r.kernel.org,
Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
linux-kernel@...r.kernel.org,
Lothar Wassmann <LW@...o-electronics.de>,
kernel@...gutronix.de, Fabio Estevam <fabio.estevam@....com>,
Lukasz Majewski <l.majewski@...ess.pl>
Subject: Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX
PWMv2
Hi Lukasz,
Sry for the delay, I was traveling and had no access to hardware...
On 2017-01-05 01:53, Boris Brezillon wrote:
> On Thu, 5 Jan 2017 10:35:05 +0100
> Lukasz Majewski <lukma@...x.de> wrote:
>
>> On Thu, 5 Jan 2017 10:19:35 +0100
>> Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
>>
>> > On Thu, 5 Jan 2017 10:03:47 +0100
>> > Lukasz Majewski <lukma@...x.de> wrote:
>> >
>> > > On Thu, 5 Jan 2017 09:50:35 +0100
>> > > Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
>> > >
>> > > > On Thu, 5 Jan 2017 00:36:50 +0100
>> > > > Lukasz Majewski <lukma@...x.de> wrote:
>> > > >
>> > > > > This commit provides apply() callback implementation for i.MX's
>> > > > > PWMv2.
>> > > > >
>> > > > > Suggested-by: Stefan Agner <stefan@...er.ch>
>> > > > > Suggested-by: Boris Brezillon
>> > > > > <boris.brezillon@...e-electrons.com> Signed-off-by: Lukasz
>> > > > > Majewski <l.majewski@...ess.pl> Reviewed-by: Boris Brezillon
>> > > > > <boris.brezillon@...e-electrons.com> ---
>> > > > > Changes for v4:
>> > > > > - Avoid recalculation of PWM parameters when disabling PWM
>> > > > > signal
>> > > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
>> > > > > clk_disable_unprepare(imx->clk_per)
>> > > >
>> > > > I don't see that one, but I'm not sure this is actually needed.
>> > > > In the enabled path we enable the clk before accessing the
>> > > > registers, and in the disable path, assuming you change the code
>> > > > according to my suggestion, the clk should already be enabled
>> > > > when you write to MX3_PWMCR.
>> > > >
>> > > > >
>> > > > > Changes for v3:
>> > > > > - Remove ipg clock enable/disable functions
>> > > > >
>> > > > > Changes for v2:
>> > > > > - None
>> > > > > ---
>> > > > > drivers/pwm/pwm-imx.c | 67
>> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
>> > > > > changed, 67 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > > > > index 60cdc5c..134dd66 100644
>> > > > > --- a/drivers/pwm/pwm-imx.c
>> > > > > +++ b/drivers/pwm/pwm-imx.c
>> > > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
>> > > > > *chip, return ret;
>> > > > > }
>> > > > >
>> > > > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
>> > > > > pwm_device *pwm,
>> > > > > + struct pwm_state *state)
>> > > > > +{
>> > > > > + unsigned long period_cycles, duty_cycles, prescale;
>> > > > > + struct imx_chip *imx = to_imx_chip(chip);
>> > > > > + struct pwm_state cstate;
>> > > > > + unsigned long long c;
>> > > > > + int ret;
>> > > > > +
>> > > > > + pwm_get_state(pwm, &cstate);
>> > > > > +
>> > > > > + if (state->enabled) {
>> > > > > + c = clk_get_rate(imx->clk_per);
>> > > > > + c *= state->period;
>> > > > > +
>> > > > > + do_div(c, 1000000000);
>> > > > > + period_cycles = c;
>> > > > > +
>> > > > > + prescale = period_cycles / 0x10000 + 1;
>> > > > > +
>> > > > > + period_cycles /= prescale;
>> > > > > + c = (unsigned long long)period_cycles *
>> > > > > state->duty_cycle;
>> > > > > + do_div(c, state->period);
>> > > > > + duty_cycles = c;
>> > > > > +
>> > > > > + /*
>> > > > > + * according to imx pwm RM, the real period
>> > > > > value should be
>> > > > > + * PERIOD value in PWMPR plus 2.
>> > > > > + */
>> > > > > + if (period_cycles > 2)
>> > > > > + period_cycles -= 2;
>> > > > > + else
>> > > > > + period_cycles = 0;
>> > > > > +
>> > > >
>> > > > Starting here...
>> > > >
>> > > > > + ret = clk_prepare_enable(imx->clk_per);
>> > > > > + if (ret)
>> > > > > + return ret;
>> > > > > +
>> > > > > + /*
>> > > > > + * Wait for a free FIFO slot if the PWM is
>> > > > > already enabled, and
>> > > > > + * flush the FIFO if the PWM was disabled and
>> > > > > is about to be
>> > > > > + * enabled.
>> > > > > + */
>> > > > > + if (cstate.enabled)
>> > > > > + imx_pwm_wait_fifo_slot(chip, pwm);
>> > > > > + else if (state->enabled)
>> > > > > + imx_pwm_sw_reset(chip);
>> > > >
>> > > > ... till here, should be replaced by:
>> > > >
>> > > >
>> > > > /*
>> > > > * Wait for a free FIFO slot if the PWM is already
>> > > > enabled, and
>> > > > * flush the FIFO if the PWM was disabled and is
>> > > > about to be
>> > > > * enabled.
>> > > > */
>> > > > if (cstate.enabled) {
>> > > > imx_pwm_wait_fifo_slot(chip, pwm);
>> > > > } else {
>> > > > ret = clk_prepare_enable(imx->clk_per);
>> > > > if (ret)
>> > > > return ret;
>> > >
>> > > In the other mail you mentioned that the clock should be enabled
>> > > unconditionally to fix issue on iMX7 when we want to access
>> > > registers.
>> >
>> > When I said unconditionally, I meant call
>> > clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
>> > beginning of the function) and call clk_disable_(imx->clk_per) just
>> > before returning 0 at the end of the function. This way you're
>> > guaranteed that the clk is always enabled when you access registers in
>> > between. Of course, you still need the the clk_prepare_enable() and
>> > clk_disable_unprepare() in the enable and disable path to keep the clk
>> > enabled when the PWM is enabled, and to disable it when the clk is
>> > being disabled.
>>
>> Why you didn't said this when I pasted pseudo code to the other e-mail
>> and explicitly asked if this is correct? By doing that I wanted to
>> avoid situations like this (so I will have to repost this again and
>> again .....).
>
> Because I thought the unconditional clk_enable/disable was not part of
> your example and would be added later, and I must admit I didn't
> carefully read the code chunk. Anyway, it's not a big deal, you're only
> at v4.
>
>>
>>
>> >
>> > But, while reviewing your patch I realized this was actually unneeded
>> > (see the explanation in my previous review).
>> >
>> > >
>> > > Now it depends on cstate.enabled flag.
>> > >
>> > > So we end up with
>> > >
>> > > if (state.enabled && !cstate.enabled)
>> > > clk_preapre_enable();
>> >
>> > Yep, and that's correct.
>>
>> And following patch:
>> http://patchwork.ozlabs.org/patch/709510/
>>
>> address this issue.
>
> Yes, that was needed because the enable/disable path were not
> separated, and we were unconditionally writing to the IP registers
> even when the PWM was already disabled (which is probably the case
> generating the fault reported by Stefan). This is not the case anymore,
> but let's wait for Stefan to confirm this.
With v4 as is, the kernel crashes/hangs on i.MX 7.
The function imx_pwm_apply_v2 gets first called with state->enabled 0,
cstate->enabled 0. This branches to else and leads to a register access
with clocks disabled (and if that would succeed, also an unbalanced
disable?...)
With the proposed change plus the additional change in the else branch
it works for me:
@@ -192,19 +193,20 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
struct pwm_device *pwm,
else
period_cycles = 0;
- ret = clk_prepare_enable(imx->clk_per);
- if (ret)
- return ret;
-
/*
* Wait for a free FIFO slot if the PWM is already
enabled, and
* flush the FIFO if the PWM was disabled and is about
to be
* enabled.
*/
- if (cstate.enabled)
+ if (cstate.enabled) {
imx_pwm_wait_fifo_slot(chip, pwm);
- else if (state->enabled)
+ } else if (state->enabled) {
+ ret = clk_prepare_enable(imx->clk_per);
+ if (ret)
+ return ret;
+
imx_pwm_sw_reset(chip);
+ }
writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -218,7 +220,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
struct pwm_device *pwm,
cr |= MX3_PWMCR_POUTC;
writel(cr, imx->mmio_base + MX3_PWMCR);
- } else {
+ } else if (cstate.enabled) {
writel(0, imx->mmio_base + MX3_PWMCR);
clk_disable_unprepare(imx->clk_per);
This would not disable a disabled PWM anymore, I guess at normal use not
a problem. Only at bootup it could end up left on, but I guess if we
care about boot time transition we should implement get_state, but
something which we can do in a follow up patch.
>
> Also note that Stefan's patch is introducing an 'unbalanced clk
> prepapre/enable refcount' bug if ->apply() is called several times
> consecutively with the same state->enabled value.
Hm, agreed on that one.
--
Stefan
>
>>
>> >
>> > >
>> > > which was the reason of iMX7 failure reported by Stefan to v3.
>> >
>> > Stefan, do you confirm that? I don't see how this can possibly fail
>> > since the clk is either already enabled (cstate.enabled) or we enable
>> > it (!cstate.enable) before accessing registers.
>>
>> http://patchwork.ozlabs.org/patch/709510/
>>
>> >
>> > What's for sure is that your implementation is introducing possible
>> > unbalanced ref counting on the per clk.
>>
>> You mean when somebody call twice ->apply() with "state->enabled".
>
> Yep, when someone calls twice ->apply() with state->enabled set to true
> or twice with state->enabled set to false.
Powered by blists - more mailing lists