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: <1034458d-78e0-5036-e278-6fee5d0d75ac@gmail.com>
Date:   Mon, 2 Aug 2021 21:23:51 +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

02.08.2021 17:48, Ulf Hansson пишет:
...
>> +       if (!list_empty(&genpd->child_links)) {
>> +               link = list_first_entry(&genpd->child_links, struct gpd_link,
>> +                                       child_node);
>> +               core_genpd = link->parent;
>> +       } else {
>> +               core_genpd = genpd;
>> +       }
> 
> This looks a bit odd to me. A genpd provider shouldn't need to walk
> these links as these are considered internals to genpd. Normally this
> needs lockings, etc.
> 
> Why exactly do you need this?

We have a chain of PMC domain -> core domain, both domains are created
and liked together by this PMC driver. Devices are attached to either
PMC domain or to core domain. PMC domain doesn't handle the performance
changes, performance requests go down to the core domain.

This is needed in order to translate the device's OPP into performance
state of the core domain, based on the domain to which device is attached.

>> +
>> +       pd_opp_table = dev_pm_opp_get_opp_table(&core_genpd->dev);
>> +       if (IS_ERR(pd_opp_table)) {
>> +               dev_err(&genpd->dev, "failed to get OPP table of %s: %pe\n",
>> +                       dev_name(&core_genpd->dev), pd_opp_table);
>> +               ret = PTR_ERR(pd_opp_table);
>> +               goto put_dev_opp;
>> +       }
>> +
>> +       pd_opp = dev_pm_opp_xlate_required_opp(opp_table, pd_opp_table, opp);
>> +       if (IS_ERR(pd_opp)) {
>> +               dev_err(&genpd->dev,
>> +                       "failed to xlate required OPP for %luHz of %s: %pe\n",
>> +                       rate, dev_name(dev), pd_opp);
>> +               ret = PTR_ERR(pd_opp);
>> +               goto put_pd_opp_table;
>> +       }
>> +
>> +       /*
>> +        * The initialized state will be applied by GENPD core on the first
>> +        * RPM-resume of the device.  This means that drivers don't need to
>> +        * explicitly initialize performance state.
>> +        */
>> +       state = pm_genpd_opp_to_performance_state(&core_genpd->dev, pd_opp);
>> +       gpd_data->rpm_pstate = state;
> 
> Could the above be replaced with Rajendra's suggestion [1], which
> changes genpd to internally during attach, to set a default
> performance state when there is a "required-opp" specified in the
> device  node?

It's not a "static" performance level here, but any level depending on
h/w state left from bootloader and etc. The performance level
corresponds to the voltage of the core domain, hence we need to
initialize the voltage vote before device is resumed.