| 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, 21 Oct 2019 11:57:12 +0200
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Daniel Thompson <daniel.thompson@...aro.org>,
linux-kernel@...r.kernel.org, heiko@...ech.de,
dianders@...omium.org, mka@...omium.org, groeck@...omium.org,
kernel@...labora.com, bleung@...omium.org,
linux-pwm@...r.kernel.org, Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the
last applied state
On Mon, Oct 21, 2019 at 11:42:05AM +0200, Enric Balletbo i Serra wrote:
> Hi Thierry,
> On 17/10/19 13:35, Thierry Reding wrote:
> > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:
>
> [snip]
>
> >
> > --- >8 ---
> > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <thierry.reding@...il.com>
> > Date: Thu, 17 Oct 2019 13:21:15 +0200
> > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value
> >
> > The ChromeOS embedded controller doesn't differentiate between disabled
> > and duty cycle being 0. In order not to potentially confuse consumers,
> > cache the duty cycle and return the cached value instead of the real
> > value when the PWM is disabled.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@...il.com>
> > ---
> > drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> > index 89497448d217..09c08dee099e 100644
> > --- a/drivers/pwm/pwm-cros-ec.c
> > +++ b/drivers/pwm/pwm-cros-ec.c
> > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
> > struct pwm_chip chip;
> > };
> >
> > +/**
> > + * struct cros_ec_pwm - per-PWM driver data
> > + * @duty_cycle: cached duty cycle
> > + */
> > +struct cros_ec_pwm {
> > + u16 duty_cycle;
> > +};
> > +
> > static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
> > {
> > return container_of(c, struct cros_ec_pwm_device, chip);
> > }
> >
> > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct cros_ec_pwm *channel;
> > +
> > + channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > + if (!channel)
> > + return -ENOMEM;
> > +
> > + pwm_set_chip_data(pwm, channel);
> > +
> > + return 0;
> > +}
> > +
> > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > +
> > + kfree(channel);
> > +}
> > +
> > static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
> > {
> > struct {
> > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > const struct pwm_state *state)
> > {
> > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > - int duty_cycle;
> > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > + u16 duty_cycle;
> > + int ret;
> >
> > /* The EC won't let us change the period */
> > if (state->period != EC_PWM_MAX_DUTY)
> > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > */
> > duty_cycle = state->enabled ? state->duty_cycle : 0;
> >
> > - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > + if (ret < 0)
> > + return ret;
> > +
> > + channel->duty_cycle = state->duty_cycle;
> > +
> > + return 0;
> > }
> >
> > static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > struct pwm_state *state)
> > {
> > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > int ret;
> >
> > ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > state->enabled = (ret > 0);
> > state->period = EC_PWM_MAX_DUTY;
> >
> > - /* Note that "disabled" and "duty cycle == 0" are treated the same */
> > - state->duty_cycle = ret;
> > + /*
> > + * Note that "disabled" and "duty cycle == 0" are treated the same. If
> > + * the cached duty cycle is not zero, used the cached duty cycle. This
> > + * ensures that the configured duty cycle is kept across a disable and
> > + * enable operation and avoids potentially confusing consumers.
> > + *
> > + * For the case of the initial hardware readout, channel->duty_cycle
> > + * will be 0 and the actual duty cycle read from the EC is used.
> > + */
> > + if (ret == 0 && channel->duty_cycle > 0)
> > + state->duty_cycle = channel->duty_cycle;
> > + else
> > + state->duty_cycle = ret;
> > }
> >
> > static struct pwm_device *
> > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> > }
> >
> > static const struct pwm_ops cros_ec_pwm_ops = {
> > + .request = cros_ec_pwm_request,
> > + .free = cros_ec_pwm_free,
> > .get_state = cros_ec_pwm_get_state,
> > .apply = cros_ec_pwm_apply,
> > .owner = THIS_MODULE,
> >
>
> I just tried your approach but I got a NULL pointer dereference while probe. I
> am just back from a week off but I'll be able to dig into it between today and
> tomorrow, just wanted to let you know that the patch doesn't works as is for me.
>
> [ 10.128455] Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
> [ 10.141895] Mem abort info:
>
> [ 10.145090] ESR = 0x96000004
>
> [ 10.148537] EC = 0x25: DABT (current EL), IL = 32 bits
>
> [ 10.154560] SET = 0, FnV = 0
>
> [ 10.157986] EA = 0, S1PTW = 0
>
> [ 10.161548] Data abort info:
>
> [ 10.164804] ISV = 0, ISS = 0x00000004
>
> [ 10.169111] CM = 0, WnR = 0
>
> [ 10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000
>
> [ 10.179660] [0000000000000000] pgd=0000000000000000
>
> [ 10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>
> [ 10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+)
> rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_
> tables ipv6 nf_defrag_ipv6
>
> [ 10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230
>
> [ 10.179696] Hardware name: Google Kevin (DT)
>
> [ 10.179700] pstate: 60000005 (nZCv daif -PAN -UAO)
>
> [ 10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
>
> [ 10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec]
>
> [ 10.247829] sp : ffff800012433810
>
> [ 10.251531] x29: ffff800012433810 x28: 0000000200000026
>
> [ 10.257469] x27: ffff800012433942 x26: ffff0000ef075010
>
> [ 10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30
>
> [ 10.269341] x23: 0000000000000000 x22: ffff0000ee273190
> [ 10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270
> [ 10.281214] x19: ffff800011bc98c8 x18: 0000000000000003
> [ 10.287150] x17: 0000000000000007 x16: 000000000000000e
> [ 10.293088] x15: 0000000000000001 x14: 0000000000000019
> [ 10.299024] x13: 0000000000000033 x12: 0000000000000018
> [ 10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0
> [ 10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA
> Objects: 31
> [ 10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830
> [ 10.310905] x7 : 000000000000011b x6 : 0000000000000001
> [ 10.310908] x5 : 0000000000000000 x4 : 0000000000000000
> [ 10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002
> [ 10.310914] x1 : 0000000000000000 x0 : 0000000000000000
> [ 10.310918] Call trace:
> [ 10.310931] cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
> [ 10.310944] pwmchip_add_with_polarity+0x134/0x290
The problem is that .get_state is called from the core without
requesting the pwm and so .request wasn't called before.
I still think it's wrong to cache the duty-cycle in the driver, but if
you still want to do it, .duty_cycle must go into the driver struct
which however is very inefficient given that probably most of the
machines don't actually have 255 PWMs.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Powered by blists - more mailing lists