linux-kernel - Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1b9d966-5eb1-da02-5e77-6fe008e0c8a2@gmail.com>
Date:   Fri, 29 Oct 2021 19:28:15 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Nishanth Menon <nm@...com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-mmc@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-clk@...r.kernel.org, David Heidelberg <david@...t.cz>
Subject: Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support

29.10.2021 18:28, Ulf Hansson пишет:
> On Fri, 29 Oct 2021 at 17:20, Dmitry Osipenko <digetx@...il.com> wrote:
>>
>> 26.10.2021 01:40, Dmitry Osipenko пишет:
>>> +     ret = devm_pm_runtime_enable(&pdev->dev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = pm_runtime_resume_and_get(&pdev->dev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>>       /* Set maximum frequency of the IP */
>>> -     ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
>>> +     ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency);
>>>       if (ret < 0) {
>>>               dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
>>> -             return ret;
>>> +             goto put_pm;
>>>       }
>>>
>>>       /*
>>> @@ -278,7 +294,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>>>       if (IS_ERR(pwm->rst)) {
>>>               ret = PTR_ERR(pwm->rst);
>>>               dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);
>>> -             return ret;
>>> +             goto put_pm;
>>>       }
>>>
>>>       reset_control_deassert(pwm->rst);
>>> @@ -291,10 +307,15 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>>>       if (ret < 0) {
>>>               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>>>               reset_control_assert(pwm->rst);
>>> -             return ret;
>>> +             goto put_pm;
>>>       }
>>>
>>> +     pm_runtime_put(&pdev->dev);
>>> +
>>>       return 0;
>>> +put_pm:
>>> +     pm_runtime_put_sync_suspend(&pdev->dev);
>>> +     return ret;
>>>  }
>>>
>>>  static int tegra_pwm_remove(struct platform_device *pdev)
>>> @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev)
>>>
>>>       reset_control_assert(pc->rst);
>>>
>>> +     pm_runtime_force_suspend(&pdev->dev);
>>
>> I just noticed that RPM core doesn't reset RPM-enable count of a device
>> on driver's unbind (pm_runtime_reinit). It was a bad idea to use
>> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is
>> disabled twice on driver's removal, and thus, RPM will never be enabled
>> again.
>>
>> I'll fix it for PWM and other drivers in this series, in v15.
> 
> Good catch - and sorry for not spotting it while reviewing!
> 
> Maybe devm_pm_runtime_enable() isn't that useful after all? Should we
> suggest to remove it so others don't fall into the same trap?

devm_pm_runtime_enable() was added to the recent v5.15 kernel. It's a
useful helper, if it's used consciously. I'm not going to remove its
usage entirely from this series, for example it still should be good to
use for Tegra FUSE and HDMI drivers.