Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Thierry Reding]

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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> >  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
> [   10.363576]  pwmchip_add+0x24/0x30
> [   10.367383]  cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec]
> [   10.373325]  platform_drv_probe+0x58/0xa8
> [   10.377809]  really_probe+0xe0/0x318
> [   10.381804]  driver_probe_device+0x5c/0xf0
> [   10.386381]  device_driver_attach+0x74/0x80

Okay, this is caused by ->get_state() getting called during
pwmchip_add_with_polarity(), which is before any of the PWMs can have
been requested, which means ->request() has not been called yet and
therefore the per-PWM state doesn't exist yet. There's a few ways to
work around that (one would be to store the state within the per-chip
structure (rather than per-PWM, another would be to defer calling the
->get_state() callback when the PWM is requested).

Either way, given that we've got a number of other drivers with similar
problems, I think at this point it's better to revert for now in v5.4
and if we do want to get this functionality back we need to start work
on fixing up the problematic drivers.

Thierry

Attachment: signature.asc
Description: PGP signature