linux-kernel - Re: [PATCH v7 02/37] soc/tegra: pmc: Implement attach

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: <aab38f90-f7b2-900f-897b-470b81d473f2@gmail.com>
Date:   Wed, 11 Aug 2021 22:30:37 +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>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v7 02/37] soc/tegra: pmc: Implement attach_dev() of power
 domain drivers

10.08.2021 13:51, Ulf Hansson пишет:
...
>> +               if (power_on) {
>> +                       ret = dev_pm_genpd_set_performance_state(dev, default_pstate);
> 
> However, this is more questionable to me.
> 
> First, I don't think we should care about whether this is "power_on"
> or not. At this point, performance states are treated orthogonal to
> idle states in genpd. We may decide to change that in some way, but
> that deserves a different change.

Okay

> Second, I don't think we should call
> dev_pm_genpd_set_performance_state() from here. It's probably better
> handled from the genpd callback itself, if/when needed.
Indeed

> That said, perhaps the new callback should just return a regular error
> code and zero on success, rather than the current performance state.
> See more below.
> 
>> +                       if (ret) {
>> +                               dev_err(dev, "failed to set default performance state %u for PM domain %s: %d\n",
>> +                                       default_pstate, pd->name, ret);
>> +                               goto out;
>> +                       }
>> +               } else {
>> +                       dev_gpd_data(dev)->rpm_pstate = default_pstate;
> 
> No, this isn't the right thing to do.
> 
> It looks like you are trying to use the ->rpm_pstate for
> synchronization with runtime PM for consumer drivers. This is fragile
> as it depends on the runtime PM deployment in the consumer driver. I
> think you should look at ->rpm_pstate as a variable solely for
> managing save/restore of the performance state for the device, during
> runtime suspend/resume in genpd.
> 
> Synchronization of a vote for a performance state for a device, needs
> to be managed by calling dev_pm_genpd_set_performance_state() - or by
> calling an OPP function that calls it, like dev_pm_opp_set_rate(), for
> example.

The !power_on case should be skipped at all because common core code
can't handle it properly, hence rpm_pstate shouldn't be touched. I
realized it only after sending out this email.

...
>>> Rather than doing the OF parsing above to find out the current state
>>> for the device, why can't you just call dev_pm_opp_set_rate() to
>>> initialize a proper vote instead?
>>>
>>
>> For some devices clock rate is either preset by bootloader, or by clk driver, or by assigned-clocks in a device-tree. And then I don't see what's the difference in comparison to initialization for the current rate.
>>
>> For some devices, like memory controller, we can't just change the clock rate because it's a complex procedure and some boards will use fixed rate, but the power vote still must be initialized.
> 
> I am not saying you should change the clock rate. The current code
> path that runs via devm_tegra_core_dev_init_opp_table() just calls
> clk_get_rate and then dev_pm_opp_set_rate() with the current rate to
> vote for the corresponding OPP level. Right?
> 
> Isn't this exactly what you want? No?

I see now what you meant, it's actually a simpler variant and it works
too. Thank you for the suggestion, I'll prepare v8.