linux-kernel - Re: [Patch] cpufreq: tegra194: remove opp table in exit hook

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] [day] [month] [year] [list]

Date:   Fri, 25 Aug 2023 17:03:27 +0530
From:   Sumit Gupta <sumitg@...dia.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
CC:     <rafael@...nel.org>, <treding@...dia.com>, <jonathanh@...dia.com>,
        <linux-pm@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <bbasu@...dia.com>,
        Sumit Gupta <sumitg@...dia.com>
Subject: Re: [Patch] cpufreq: tegra194: remove opp table in exit hook


>>>> Add exit hook and remove OPP table when all the CPU's in a policy
>>>> are offlined. It will fix the below error messages when onlining
>>>> first CPU from a policy whose all CPU's were previously offlined.
>>>>
>>>>    debugfs: File 'cpu5' in directory 'opp' already present!
>>>>    debugfs: File 'cpu6' in directory 'opp' already present!
>>>>    debugfs: File 'cpu7' in directory 'opp' already present!
>>>>
>>>> Fixes: f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")
>>>> Signed-off-by: Sumit Gupta <sumitg@...dia.com>
>>>> ---
>>>>    drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
>>>> index c90b30469165..66a9c23544db 100644
>>>> --- a/drivers/cpufreq/tegra194-cpufreq.c
>>>> +++ b/drivers/cpufreq/tegra194-cpufreq.c
>>>> @@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
>>>>                 if (ret < 0)
>>>>                         return ret;
>>>>
>>>> +             dev_pm_opp_put(opp);
>>>> +
>>>>                 freq_table[j].driver_data = pos->driver_data;
>>>>                 freq_table[j].frequency = pos->frequency;
>>>>                 j++;
>>>> @@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>>>>         return 0;
>>>>    }
>>>>
>>>> +static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
>>>> +{
>>>> +     struct device *cpu_dev = get_cpu_device(policy->cpu);
>>>> +
>>>> +     dev_pm_opp_remove_all_dynamic(cpu_dev);
>>>> +     dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>>    static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>>>>                                        unsigned int index)
>>>>    {
>>>> @@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
>>>>         .target_index = tegra194_cpufreq_set_target,
>>>>         .get = tegra194_get_speed,
>>>>         .init = tegra194_cpufreq_init,
>>>> +     .exit = tegra194_cpufreq_exit,
>>>>         .attr = cpufreq_generic_attr,
>>>>    };
>>>
>>> If it is only about hotplugging of the CPUs, then you can also do this I guess.
>>>
>>> commit 263abfe74b5f ("cpufreq: dt: Implement online/offline() callbacks")
> 
> You should do this as well, this makes hotplugging paths much faster. i.e. on
> top of this patch.
> 

Sent a separate patch to add online/offline callbacks.
   https://lore.kernel.org/lkml/20230825111920.8257-1-sumitg@nvidia.com/

Also, sent v2 of this patch with updated commit description.
   https://lore.kernel.org/lkml/20230825111617.8069-1-sumitg@nvidia.com/

Thank you,
Sumit Gupta

>>> But since your driver is capable of being built as a module, I suggest you try
>>> to build it as one and insert remove it multiple times. It must cause you some
>>> trouble as you don't implement an .exit() before this patch.
>>>
>>> Eventually, I think you need to do both, what this patch and 263abfe74b5f do.
>>> Just that the reasons need to be correct for both the changes.
>>>
>>> --
>>> viresh
>>
>> Hi Viresh,
>> I got the same message on inserting and removing the module multiple times
>> as you suggested. After applying this change, the message is not coming. So,
>> the current change is resolving both scenarios as __cpufreq_offline() calls
>> either exit() or offline().
>> I can update the commit message to mention both scenarios and keep change as
>> it is?
>>
>>    cpufreq_remove_dev
>>    |-__cpufreq_offline
>>    |--tegra194_cpufreq_exit
>>
>>    cpuhp_cpufreq_offline
>>    |-__cpufreq_offline
>>    |--tegra194_cpufreq_exit
> 
> --
> viresh