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  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]
Date:   Wed, 11 Aug 2021 22:30:37 +0300
From:   Dmitry Osipenko <>
To:     Ulf Hansson <>
Cc:     Thierry Reding <>,
        Jonathan Hunter <>,
        Viresh Kumar <>,
        Stephen Boyd <>,
        Peter De Schrijver <>,
        Linux Kernel Mailing List <>,
        linux-tegra <>,
        Linux PM <>
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.


> 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.

> 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.

Powered by blists - more mailing lists