| 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
| ||
|
Message-ID: <54d71bb6-27bb-494f-8f63-710e0e4148e7@oss.qualcomm.com>
Date: Wed, 20 Aug 2025 09:57:44 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
Stephen Boyd <sboyd@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Manivannan Sadhasivam <mani@...nel.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 1/3] OPP: Add support to find OPP for a set of keys
On 8/19/2025 1:48 PM, Viresh Kumar wrote:
> On 19-08-25, 11:04, Krishna Chaitanya Chundru wrote:
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index edbd60501cf00dfd1957f7d19b228d1c61bbbdcc..ce359a3d444b0b7099cdd2421ab1019963d05d9f 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -461,6 +461,15 @@ int dev_pm_opp_get_opp_count(struct device *dev)
>> EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
>>
>> /* Helpers to read keys */
>> +static unsigned long _read_opp_key(struct dev_pm_opp *opp, int index, struct dev_pm_opp_key *key)
>
> Move this to the end of the list, after _read_bw() i.e.
>
ack
>> +{
>> + key->bandwidth = opp->bandwidth ? opp->bandwidth[index].peak : 0;
>> + key->freq = opp->rates[index];
>> + key->level = opp->level;
>> +
>> + return true;
>> +}
>> +
>> static unsigned long _read_freq(struct dev_pm_opp *opp, int index)
>> {
>> return opp->rates[index];
>> @@ -488,6 +497,23 @@ static bool _compare_exact(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> return false;
>> }
>>
>> +static bool _compare_opp_key_exact(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> + struct dev_pm_opp_key opp_key, struct dev_pm_opp_key key)
>> +{
>
> And this after _compare_floor().
>
ack
>> + bool level_match = (opp_key.level == OPP_LEVEL_UNSET ||
>
> Why are we still checking this ? You removed this from freq check but
> not level and bandwidth ?
>
ok I will change for level and bw also similar to freq.
>> + key.level == OPP_LEVEL_UNSET || opp_key.level == key.level);
>> + bool bw_match = (opp_key.bandwidth == 0 ||
>> + key.bandwidth == 0 || opp_key.bandwidth == key.bandwidth);
>> + bool freq_match = (key.freq == 0 || opp_key.freq == key.freq);
>> +
>> + if (freq_match && level_match && bw_match) {
>> + *opp = temp_opp;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static bool _compare_ceil(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> unsigned long opp_key, unsigned long key)
>> {
>> @@ -541,6 +567,40 @@ static struct dev_pm_opp *_opp_table_find_key(struct opp_table *opp_table,
>> return opp;
>> }
>>
>> +static struct dev_pm_opp *_opp_table_find_opp_key(struct opp_table *opp_table,
>> + struct dev_pm_opp_key *key, int index, bool available,
>> + unsigned long (*read)(struct dev_pm_opp *opp, int index,
>> + struct dev_pm_opp_key *key),
>> + bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> + struct dev_pm_opp_key opp_key, struct dev_pm_opp_key key),
>> + bool (*assert)(struct opp_table *opp_table, unsigned int index))
>> +{
>> + struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
>> + struct dev_pm_opp_key temp_key;
>> +
>> + /* Assert that the requirement is met */
>> + if (assert && !assert(opp_table, index))
>
> Just drop the `assert` check, it isn't optional. Make the same change
> in _opp_table_find_key() too in a separate patch if you can.
>
ack
>> + return ERR_PTR(-EINVAL);
>> +
>> + guard(mutex)(&opp_table->lock);
>> +
>> + list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
>> + if (temp_opp->available == available) {
>> + read(temp_opp, index, &temp_key);
>> + if (compare(&opp, temp_opp, temp_key, *key))
>
> Update *key and do dev_pm_opp_get() here itself. And same in
> _opp_table_find_key().
>
ack
>> + break;
>> + }
>> + }
>> +
>> + /* Increment the reference count of OPP */
>> + if (!IS_ERR(opp)) {
>> + *key = temp_key;
>> + dev_pm_opp_get(opp);
>> + }
>> +
>> + return opp;
>> +}
>> +
>> static struct dev_pm_opp *
>> _find_key(struct device *dev, unsigned long *key, int index, bool available,
>> unsigned long (*read)(struct dev_pm_opp *opp, int index),
>> @@ -632,6 +692,46 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>> }
>> EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
>>
>> +/**
>> + * dev_pm_opp_find_key_exact() - Search for an exact OPP key
>> + * @dev: Device for which the OPP is being searched
>> + * @key: OPP key to match
>> + * @available: true/false - match for available OPP
>> + *
>> + * Return: Searches for an exact match the OPP key in the OPP table and returns
>
> The `Return` section should only talk about the returned values. The
> above line for example should be present as a standalone line before
> the `Return` section.
>
ack
>> + * pointer to the matching opp if found, else returns ERR_PTR in case of error
>> + * and should be handled using IS_ERR. Error return values can be:
>> + * EINVAL: for bad pointer
>> + * ERANGE: no match found for search
>> + * ENODEV: if device not found in list of registered devices
>> + *
>> + * Note: available is a modifier for the search. if available=true, then the
>> + * match is for exact matching key and is available in the stored OPP
>> + * table. if false, the match is for exact key which is not available.
>> + *
>> + * This provides a mechanism to enable an opp which is not available currently
>> + * or the opposite as well.
>> + *
>> + * The callers are required to call dev_pm_opp_put() for the returned OPP after
>> + * use.
>
> There are various minor issues in the text here, like double spaces,
> not starting with a capital letter after a full stop, etc. Also put
> arguments in `` block, like `available`.
>
ack
>> + */
>> +struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> + struct dev_pm_opp_key key,
>> + bool available)
>> +{
>> + struct opp_table *opp_table __free(put_opp_table) = _find_opp_table(dev);
>> +
>> + if (IS_ERR(opp_table)) {
>> + dev_err(dev, "%s: OPP table not found (%ld)\n", __func__,
>> + PTR_ERR(opp_table));
>> + return ERR_CAST(opp_table);
>> + }
>> +
>> + return _opp_table_find_opp_key(opp_table, &key, 0, available, _read_opp_key,
>> + _compare_opp_key_exact, assert_single_clk);
>
> Since the only user doesn't use `index` for now, I wonder if that
> should be added at all in all these functions.
>
ok I will remove it.
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_find_key_exact);
>> +
>> /**
>> * dev_pm_opp_find_freq_exact_indexed() - Search for an exact freq for the
>> * clock corresponding to the index
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index cf477beae4bbede88223566df5f43d85adc5a816..53e02098129d215970d0854b1f8ffaf4499f2bd4 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -98,6 +98,18 @@ struct dev_pm_opp_data {
>> unsigned long u_volt;
>> };
>>
>> +/**
>> + * struct dev_pm_opp_key - Key used to identify OPP entries
>> + * @freq: Frequency in Hz
>> + * @level: Performance level associated with the OPP entry
>> + * @bandwidth: Bandwidth associated with the OPP entry
>
> Also mention the NOP value of all these keys, i.e. what the user must
> fill them with if that key is not supposed to be matched.
>
ack
>> + */
>> +struct dev_pm_opp_key {
>> + unsigned long freq;
>> + unsigned int level;
>> + u32 bandwidth;
>
> Maybe use `bw` here like in other APIs.
>
ack.
- Krishna Chaitanya.
>> +};
>> +
>> #if defined(CONFIG_PM_OPP)
>>
>> struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
>> @@ -131,6 +143,10 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>> unsigned long freq,
>> bool available);
>>
>> +struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> + struct dev_pm_opp_key key,
>> + bool available);
>> +
>> struct dev_pm_opp *
>> dev_pm_opp_find_freq_exact_indexed(struct device *dev, unsigned long freq,
>> u32 index, bool available);
>> @@ -289,6 +305,13 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>> return ERR_PTR(-EOPNOTSUPP);
>> }
>>
>> +static inline struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> + struct dev_pm_opp_key key,
>> + bool available)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> static inline struct dev_pm_opp *
>> dev_pm_opp_find_freq_exact_indexed(struct device *dev, unsigned long freq,
>> u32 index, bool available)
>>
>> --
>> 2.34.1
>
Powered by blists - more mailing lists