[dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code

[Burakov, Anatoly]

On 07-May-21 3:13 AM, Richael Zhuang wrote:
>> @@ -262,18 +189,16 @@ static int
>>   power_init_for_setting_freq(struct acpi_power_info *pi)  {
>>   	FILE *f;
>> -	char fullpath[PATH_MAX];
>>   	char buf[BUFSIZ];
>>   	uint32_t i, freq;
>> -	char *s;
>> +	int ret;
>>
>> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>> -			pi->lcore_id);
>> -	f = fopen(fullpath, "rw+");
>> -	FOPEN_OR_ERR_RET(f, -1);
>> +	open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "r",
>> +			&f);
> Hi Anatoly,
> I tried to verify your patch on my arm platform,  and I found several bugs.
> 
> Here it should be "rw+", for it will write freq to POWER_SYSFILE_SETSPEED later.
> 
> Best Regards,
> Richael

<snip>

>>   	return 1;
>>   }
>> +
>> +int
>> +open_core_sysfs_file(const char *template, unsigned int core, const char
>> *mode,
>> +		FILE **f)
>> +{
>> +	char fullpath[PATH_MAX];
>> +	FILE *tmpf;
>> +
>> +	/* silenced -Wformat-nonliteral here */
>> +	snprintf(fullpath, sizeof(fullpath), template, core);
>> +	tmpf = fopen(fullpath, mode);
>> +	if (tmpf == NULL)
>> +		return -1;
>> +	*f = tmpf;
> 
> When the file that open_core_sysfs_file() opens doesn't exist, there's segmentation fault.
> Moving *f=tmpf above runs OK.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +read_core_sysfs_u32(FILE *f, uint32_t *val) {
>> +	char buf[BUFSIZ];
>> +	uint32_t fval;
>> +	char *s;
>> +
>> +	s = fgets(buf, sizeof(buf), f);
>> +	if (s == NULL)
>> +		return -1;
>> +

<snip>

>> +	/* strip off any terminating newlines */
>> +	*strchrnul(buf, '\n') = '\0';
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +write_core_sysfs_s(FILE *f, const char *str) {
>> +	int ret;
>> +
>> +	ret = fseek(f, 0, SEEK_SET);
>> +	if (ret != 0)
>> +		return -1;
>> +
>> +	ret = fputs(str, f);
>> +	if (ret != 0)
> 
> ret >=0 if success, EOF means failure.
> 

Hi Richael,

Thank you very much for testing! I'll address these issues in the next 
revision, and double-check everything else.

-- 
Thanks,
Anatoly