| 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
| ||
|
Message-ID: <DB3PR0402MB39162604F6C2D57FBD56E884F5420@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Thu, 21 Mar 2019 14:53:06 +0000
From: Anson Huang <anson.huang@....com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
CC: "thierry.reding@...il.com" <thierry.reding@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"stefan@...er.ch" <stefan@...er.ch>,
"otavio@...ystems.com.br" <otavio@...ystems.com.br>,
Leonard Crestez <leonard.crestez@....com>,
"schnitzeltony@...il.com" <schnitzeltony@...il.com>,
"jan.tuerk@...rion.com" <jan.tuerk@...rion.com>,
Robin Gong <yibin.gong@....com>,
"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@...gutronix.de]
> Sent: 2019年3月21日 21:42
> To: Anson Huang <anson.huang@....com>
> Cc: thierry.reding@...il.com; robh+dt@...nel.org; mark.rutland@....com;
> shawnguo@...nel.org; s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com; linux@...linux.org.uk; stefan@...er.ch;
> otavio@...ystems.com.br; Leonard Crestez <leonard.crestez@....com>;
> schnitzeltony@...il.com; jan.tuerk@...rion.com; Robin Gong
> <yibin.gong@....com>; linux-pwm@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 21, 2019 at 12:47:32PM +0000, Anson Huang wrote:
> > > On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > > > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > > > > + struct imx_tpm_pwm_param *p) {
> > > > > > + struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > + u32 val, saved_cmod, cur_prescale;
> > > > > > +
> > > > > > + /* make sure counter is disabled for programming prescale
> */
> > > > >
> > > > > @Thierry: What is your thought here? IMHO this should only be
> > > > > allowed if all affected PWMs are off.
> > > >
> > > > As we already make sure that ONLY when there is ONLY 1 user and
> > > > the requested period is different from the current period, then
> > > > this function will be called, so there is impossible that multiple
> > > > PWMs are active
> > > and the period is requested to be changed.
> > > > Am I right?
> > >
> > > This problem is not about two PWMs. If you reconfigure a running PWM
> > > the requirement is that the hardware completes a whole period with
> > > the old configuration and then immediately starts a new period with
> > > the new parameters. If you stop the counter, the last period with
> > > the old parameters is disturbed.
> >
> > So, I think simply return error if the counter is running and there is
> > a new PS change request, right?
>
> That's the general idea yes. If you cannot fulfil the request without violating
> the guarantees you have to adhere, refuse the request with an error.
Already add below check in next version:
val = readl(tpm->base + PWM_IMX_TPM_SC);
cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
if (cmod && cur_prescale != p->prescale)
return -EBUSY;
>
> > > > > > + */
> > > > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > > > +
> > > > > > + /* get channel status */
> > > > > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ?
> true :
> > > > > > +false; }
> > > > > > +
> > > > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > > > + struct pwm_device *pwm,
> > > > > > + struct pwm_state *state)
> > > > >
> > > > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this
> necessary?
> > > > > Please either call it without the mutex or annotate the function
> > > > > that the caller is supposed to hold it.
> > > >
> > > > OK, will make sure call it without mutex hold.
After further check, since this function will call get_state and it will access
shared registers, so mutex is necessary I think, so I will add annotate for this
function.
>
> Reading through the reference manual I noticed that there might be a
> stall: If you write two values to CnV the second write is ignored if the first
> wasn't latched yet. That might mean that you cannot release the mutex
> before the newly configured state is active. This is related to the request to
> not let .apply return before the configured state is active, but I didn't thought
> this to an end what the real consequences have to be.
The reference manual says the register is NOT updated until the current period finished
If counter is running, so I added below check for both period update and duty update, we
Can just wait the register value read matches what we write:
Period update:
writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
/* make sure MOD register is updated */
timeout = jiffies + msecs_to_jiffies(tpm->real_period /
NSEC_PER_MSEC + 1);
while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) {
if (time_after(jiffies, timeout))
return = -ETIME;
cpu_relax();
}
Duty update:
writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
/* make sure CnV register is updated */
timeout = jiffies + msecs_to_jiffies(tpm->real_period /
NSEC_PER_MSEC + 1);
while (readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) != val) {
if (time_after(jiffies, timeout))
return = -ETIME;
cpu_relax();
}
>
> > > > If counter is enabled, and for edge aligned PWM mode(EPWM), the
> > > > register is updated After written and TPM counter changes from MOD
> > > > to zero, same as period count update, HW will make sure the period
> finish..
> > >
> > > Looking into my concern again, it is actually the other way around:
> > > Assuming a single used PWM channel that runs at duty_cycle=500 +
> > > period=1000. Then pwm_imx_tpm_apply() is called with state-
> > > >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls
> > > pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
> > > PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV
> to
> > > the value corresponding to duty_cycle=700, it produces a waveform
> > > with
> > > duty_cycle=500 and period=800 which is bad. This is another
> > > limitation that can be worked around in software with some effort
> > > (which might or might not be worth to spend).
> >
> > I am sure that on i.MX7ULP platform we used for backlight ONLY, it
> > should NOT that matter if this case happen, unless the counter is
> > disabled, then the effort spend on this case will be huge, so I plan to leave
> it as what it is if you don't mind.
>
> That means you have to add this to the list of limitations.
OK, will try best to describe this scenario in limitation note;
>
> > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > > > > + struct pwm_device *pwm,
> > > > > > + struct pwm_state *state) {
> > > > > > + struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > + struct imx_tpm_pwm_param param;
> > > > > > + struct pwm_state real_state;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state,
> &real_state);
> > > > > > + if (ret)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + mutex_lock(&tpm->lock);
> > > > > > +
> > > > > > + /*
> > > > > > + * TPM counter is shared by multiple channels, so
> > > > > > + * prescale and period can NOT be modified when
> > > > > > + * there are multiple channels in use with different
> > > > > > + * period settings.
> > > > > > + */
> > > > > > + if (real_state.period != tpm->real_period) {
> > > > > > + if (tpm->user_count > 1) {
> > > > > > + ret = -EBUSY;
> > > > > > + goto exit;
> > > > > > + }
> > > > > > +
> > > > > > + pwm_imx_tpm_setup_period(chip, ¶m);
> > > > > > + tpm->real_period = real_state.period;
> > > > > > + }
> > > > > > +
> > > > > > + pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > > > > +
> > > > > > +exit:
> > > > > > + mutex_unlock(&tpm->lock);
> > > > >
> > > > > .apply is supposed to sleep until the newly configured state is active.
> > > > > This is missing here, right?
> > > >
> > > > NOT quite understand, you meant .apply could be sleep if mutex is
> > > > hold by other thread?
> > >
> > > No. .apply is supposed to only return when the new configuration is
> active.
> > > So if the PWM is running in its previous configuration, you setup
> > > the registers such that the new configuration gets active in the
> > > next period, you must not yet return to the caller until the new period
> started.
> >
> > That bring me back to previous question, we can add waiting for period
> > finish And then return from .apply, but we also need a timeout for the
> > wait, what should The timeout value be? 100mS? Or even several seconds?
>
> A sane value would be the duration of the previously configured period
> length as this is the theoretical upper limit for this delay.
I will just use tpm->real_period saved in driver data, for first time update, although
Tpm->real_period is 0, but the counter is disabled, so the register update is immediately,
From second time, the tpm->real_period will be previous period configured.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7Cae
> 5949f109c5401a2b5d08d6ae030d7f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636887725468509805&sdata=4qU1eJzlIle9v%2BHqEqwb
> Jm%2FkkAQlCH2LtsFDMaeSGHE%3D&reserved=0 |
Powered by blists - more mailing lists