On Mon, Sep 24, 2018 at 11:58:42AM +0200, Hans de Goede wrote:
> 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.
Thanks.
I'm just wondering if we can leave pwm_lpss_apply() untouched and use a new helper for ->remove() and ->get_state() only.
> > > --- 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);
--
With Best Regards,
Andy Shevchenko
