linux-kernel - Re: [PATCH v5 07/23] PM: EM: Refactor how the EM table is allocated and populated

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]

Date: Tue, 19 Dec 2023 13:19:30 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Dietmar Eggemann <dietmar.eggemann@....com>
Cc: rui.zhang@...el.com, amit.kucheria@...durent.com,
 linux-kernel@...r.kernel.org, amit.kachhap@...il.com,
 daniel.lezcano@...aro.org, viresh.kumar@...aro.org, len.brown@...el.com,
 pavel@....cz, mhiramat@...nel.org, qyousef@...alina.io, wvw@...gle.com,
 linux-pm@...r.kernel.org, rafael@...nel.org
Subject: Re: [PATCH v5 07/23] PM: EM: Refactor how the EM table is allocated
 and populated



On 12/12/23 18:50, Dietmar Eggemann wrote:
> On 29/11/2023 12:08, Lukasz Luba wrote:
>> Split the process of allocation and data initialization for the EM table.
>> The upcoming changes for modifiable EM will use it.
>>
>> This change is not expected to alter the general functionality.
> 
> NIT: IMHO, I guess you wanted to say: "No functional changes
> introduced"? I.e. all not only general functionality ...
> 

Yes 'no functional changes'. Rafael gave me that sense once - and I use
in such cases.

> [...]
> 
>>   static int em_create_pd(struct device *dev, int nr_states,
>> @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states,
>>   			return -ENOMEM;
>>   	}
>>   
>> -	ret = em_create_perf_table(dev, pd, nr_states, cb, flags);
>> -	if (ret) {
>> -		kfree(pd);
>> -		return ret;
>> -	}
>> +	pd->nr_perf_states = nr_states;
> 
> Why does `pd->nr_perf_states = nr_states;` have to move from
> em_create_perf_table() to em_create_pd()?

Because I have split the old code which did allocation and
initialization w/ data the in em_create_perf_table().

Now we are going to have separate:
1. allocation of a new table (which can be re-used later)
2. initialization of the data (power, freq, etc) in registration
    code path

It will allow to also allow to introduce update data function,
and simply use the same allocation function for both cases:
- EM registration code path
- update EM code path

> 
>> +
>> +	ret = em_allocate_perf_table(pd, nr_states);
>> +	if (ret)
>> +		goto free_pd;
>> +
>> +	ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags);
> 
> If you set it in em_create_pd() then you can use 'pd->nr_perf_states' in
> em_create_perf_table() and doesn't have to pass `nr_states`.
> 
> [...]

That's true. I could further refactor that function and remove that
'nr_states' argument.

I'll do this in v6. Thanks!