Received-SPF: pass (google.com: domain of linux-kernel+bounces-5246-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <00f6f550-0f60-4f62-a301-2af94c68b7f8@arm.com>
Date: Tue, 19 Dec 2023 13:19:30 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 07/23] PM: EM: Refactor how the EM table is allocated
 and populated
Content-Language: en-US
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: rui.zhang@intel.com, amit.kucheria@verdurent.com,
 linux-kernel@vger.kernel.org, amit.kachhap@gmail.com,
 daniel.lezcano@linaro.org, viresh.kumar@linaro.org, len.brown@intel.com,
 pavel@ucw.cz, mhiramat@kernel.org, qyousef@layalina.io, wvw@google.com,
 linux-pm@vger.kernel.org, rafael@kernel.org
References: <20231129110853.94344-1-lukasz.luba@arm.com>
 <20231129110853.94344-8-lukasz.luba@arm.com>
 <fbee5e3b-4c32-443a-b756-943762ae07b8@arm.com>
From: Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <fbee5e3b-4c32-443a-b756-943762ae07b8@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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!