Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] cpufreq:powernv: Fix init_chip_info initialization in
 numa=off
To:     ego@linux.vnet.ibm.com
Cc:     mpe@ellerman.id.au, rjw@rjwysocki.net, linux-pm@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, pratik.r.sampat@gmail.com
References: <20210726170758.61041-1-psampat@linux.ibm.com>
 <20210727061656.GA10282@in.ibm.com>
From:   Pratik Sampat <psampat@linux.ibm.com>
Message-ID: <60aaa78a-ece1-73d2-ea4c-e3835a6e3ab8@linux.ibm.com>
Date:   Tue, 27 Jul 2021 12:19:04 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210727061656.GA10282@in.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 27/07/21 11:46 am, Gautham R Shenoy wrote:
> On Mon, Jul 26, 2021 at 10:37:57PM +0530, Pratik R. Sampat wrote:
>> In the numa=off kernel command-line configuration init_chip_info() loops
>> around the number of chips and attempts to copy the cpumask of that node
>> which is NULL for all iterations after the first chip.
>>
>> Hence, store the cpu mask for each chip instead of derving cpumask from
>> node while populating the "chips" struct array and copy that to the
>> chips[i].mask
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 053819e0bf84 ("cpufreq: powernv: Handle throttling due to Pmax capping at chip level")
>> Signed-off-by: Pratik R. Sampat <psampat@linux.ibm.com>
>> Reported-by: Shirisha Ganta <shirisha.ganta1@ibm.com>
>> ---
>>   drivers/cpufreq/powernv-cpufreq.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 005600cef273..8ec10d9aed8f 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -1046,12 +1046,20 @@ static int init_chip_info(void)
>>   	unsigned int *chip;
>>   	unsigned int cpu, i;
>>   	unsigned int prev_chip_id = UINT_MAX;
>> +	cpumask_t *chip_cpu_mask;
>>   	int ret = 0;
>>
>>   	chip = kcalloc(num_possible_cpus(), sizeof(*chip), GFP_KERNEL);
>>   	if (!chip)
>>   		return -ENOMEM;
>>
>> +	/* Allocate a chip cpu mask large enough to fit mask for all chips */
>> +	chip_cpu_mask = kcalloc(32, sizeof(cpumask_t), GFP_KERNEL);
> I suppose by 32 you mean the maximum number of chips possible. You
> could use a #define for that.

ack, I could #define the constant.

> Otherwise, the patch looks good to me.
>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>
Thanks
Pratik

>
>> +	if (!chip_cpu_mask) {
>> +		ret = -ENOMEM;
>> +		goto free_and_return;
>> +	}
>> +
>>   	for_each_possible_cpu(cpu) {
>>   		unsigned int id = cpu_to_chip_id(cpu);
>>
>> @@ -1059,22 +1067,25 @@ static int init_chip_info(void)
>>   			prev_chip_id = id;
>>   			chip[nr_chips++] = id;
>>   		}
>> +		cpumask_set_cpu(cpu, &chip_cpu_mask[nr_chips-1]);
>>   	}
>>
>>   	chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
>>   	if (!chips) {
>>   		ret = -ENOMEM;
>> -		goto free_and_return;
>> +		goto out_chip_cpu_mask;
>>   	}
>>
>>   	for (i = 0; i < nr_chips; i++) {
>>   		chips[i].id = chip[i];
>> -		cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
>> +		cpumask_copy(&chips[i].mask, &chip_cpu_mask[i]);
>>   		INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
>>   		for_each_cpu(cpu, &chips[i].mask)
>>   			per_cpu(chip_info, cpu) =  &chips[i];
>>   	}
>>
>> +out_chip_cpu_mask:
>> +	kfree(chip_cpu_mask);
>>   free_and_return:
>>   	kfree(chip);
>>   	return ret;
>> -- 
>> 2.31.1
>>