linux-kernel - Re: [PATCH v3 1/9] opp: add new helper API dev_pm_opp_set

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: <e72aeda8-035d-4144-a9ce-ae276bca87bf@quicinc.com>
Date: Fri, 2 May 2025 13:07:33 +0530
From: Praveen Talari <quic_ptalari@...cinc.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby
	<jirislaby@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Viresh Kumar
	<vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd
	<sboyd@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-serial@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-pm@...r.kernel.org>, <psodagud@...cinc.com>,
        <djaggi@...cinc.com>, <quic_msavaliy@...cinc.com>,
        <quic_vtanuku@...cinc.com>, <quic_arandive@...cinc.com>,
        <quic_mnaresh@...cinc.com>, <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v3 1/9] opp: add new helper API dev_pm_opp_set_level()

Hi Viresh

Thank you for review.

On 5/2/2025 11:07 AM, Viresh Kumar wrote:
> On 02-05-25, 08:40, Praveen Talari wrote:
>> To configure a device to a specific performance level, consumer drivers
>> currently need to determine the OPP based on the exact level and then
>> set it, resulting in code duplication across drivers.
>>
>> The new helper API, dev_pm_opp_set_level(), addresses this issue by
>> providing a streamlined method for consumer drivers to find and set the
>> OPP based on the desired performance level, thereby eliminating
>> redundancy.
>>
>> Signed-off-by: Praveen Talari <quic_ptalari@...cinc.com>
>>
>> v2 -> v3
>> - moved function defination to pm_opp.h from core.c with inline
>> - updated return value with IS_ERR(opp)
>>
>> v1 -> v2
>> - reorder sequence of tags in commit text
> As Trilok mentioned, this is not the right place for this.
>
>> ---
>>   include/linux/pm_opp.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index e7b5c602c92f..31ed8a7b554e 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -197,6 +197,28 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>>   void dev_pm_opp_remove_table(struct device *dev);
>>   void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
>>   int dev_pm_opp_sync_regulators(struct device *dev);
>> +
>> +/*
>> + * dev_pm_opp_set_level() - Configure device for a level
>> + * @dev: device for which we do this operation
>> + * @level: level to set to
>> + *
>> + * Return: 0 on success, a non-zero value if there is an error otherwise.
>> + */
> No need of these for simple wrappers.
>
>> +static inline int dev_pm_opp_set_level(struct device *dev, unsigned int level)
>> +{
>> +	struct dev_pm_opp *opp = dev_pm_opp_find_level_exact(dev, level);
>> +	int ret;
>> +
>> +	if (IS_ERR(opp))
>> +		return IS_ERR(opp);
> IS_ERR is wrong here, should be PTR_ERR.
>
>> +
>> +	ret = dev_pm_opp_set_opp(dev, opp);
>> +	dev_pm_opp_put(opp);
>> +
>> +	return ret;
>> +}
>> +
>>   #else
>>   static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
>>   {
>> @@ -461,6 +483,11 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)
>>   	return -EOPNOTSUPP;
>>   }
>>   
>> +static inline int dev_pm_opp_set_level(struct device *dev, unsigned int level)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
> No need of these too for such wrappers. And then this isn't rebased
How come? i have synced  linux-next today itself and pushed from it, 
even i didn't face any issue.
Let me know how/where rebased i.e linux-next or linux?
> over latest changes in the OPP core.
>
> I modified it and applied the below version to my tree now.
>