Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 24-09-18 11:16, Andy Shevchenko wrote:
> On Tue, Sep 11, 2018 at 07:45:32PM +0200, Hans de Goede wrote:
> > Add a helper function to properly get / put the pwm's runtime_pm
> > reference when changing from enabled to disable or vice versa.
> >
> > And use this to ensure the pwm's runtime_pm reference is properly
> > released on remove.
>
> Can you provide a test sequence to reproduce the issue?

This patch is mostly a preparation patch for adding the get_state
callback to make the driver fully support the pwm-atomic model.

When I first tried to upstream the get_state code about a year
ago, there was some not pretty code in there to deal with getting
the runtime pm reference if the backlight was already enabled when
the driver loads. One of the review requests was to factor out the
code dealing with getting/putting the runtime_pm reference when
changing state from enabled to disabled or the other way around.
That is the mean reason for this commit.

When writing this I noticed that if we rmmod the driver while
the backlight has been enabled through the driver (e.g. through
the sysfs interface) that we then leak the runtime-pm reference
which we get when we enable the pwm.

Note the purpose of adding a get_state callback is to have the i915
driver eventually read back the backlight state at boot and avoid
the backlight always jumping to 100% brightness at boot which
causes an ugly visual flicker.

Regards,

Hans



> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >   drivers/pwm/pwm-lpss.c | 47 +++++++++++++++++++++++++++++-------------
> >   1 file changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> > index e602835fd6de..d39158800baa 100644
> > --- a/drivers/pwm/pwm-lpss.c
> > +++ b/drivers/pwm/pwm-lpss.c
> > @@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
> >   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
> >   }
> >   
> > +static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
> > +					bool old_enabled, bool new_enabled)
> > +{
> > +	if (new_enabled == old_enabled)
> > +		return;
> > +
> > +	if (new_enabled)
> > +		pm_runtime_get(chip->dev);
> > +	else
> > +		pm_runtime_put(chip->dev);
> > +}
> > +
> >   static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >   			  struct pwm_state *state)
> >   {
> >   	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	pm_runtime_get_sync(chip->dev);
> >   
> >   	if (state->enabled) {
> >   		if (!pwm_is_enabled(pwm)) {
> > -			pm_runtime_get_sync(chip->dev);
> >   			ret = pwm_lpss_is_updating(pwm);
> > -			if (ret) {
> > -				pm_runtime_put(chip->dev);
> > -				return ret;
> > -			}
> > +			if (ret)
> > +				goto out;
> > +
> >   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> >   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
> >   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
> >   			ret = pwm_lpss_wait_for_update(pwm);
> > -			if (ret) {
> > -				pm_runtime_put(chip->dev);
> > -				return ret;
> > -			}
> > +			if (ret)
> > +				goto out;
> > +
> >   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
> >   		} else {
> >   			ret = pwm_lpss_is_updating(pwm);
> >   			if (ret)
> > -				return ret;
> > +				goto out;
> > +
> >   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> >   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
> > -			return pwm_lpss_wait_for_update(pwm);
> > +			ret = pwm_lpss_wait_for_update(pwm);
> >   		}
> >   	} else if (pwm_is_enabled(pwm)) {
> >   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> > -		pm_runtime_put(chip->dev);
> >   	}
> >   
> > -	return 0;
> > +	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
> > +
> > +out:
> > +	pm_runtime_put(chip->dev);
> > +	return ret;
> >   }
> >   
> >   static const struct pwm_ops pwm_lpss_ops = {
> > @@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> >   
> >   int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
> >   {
> > +	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
> > +
> > +	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
> > +
> >   	return pwmchip_remove(&lpwm->chip);
> >   }
> >   EXPORT_SYMBOL_GPL(pwm_lpss_remove);
> > --
> > 2.19.0.rc0