On 21.01.2025 07:14, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, January 9, 2025 6:55 PM
>>
>> On 03.12.2024 09:11, Penny Zheng wrote:
>>> + {
>>> + /* Read Processor Max Speed(mhz) from DMI table as anchor point */
>>> + mul = dmi_max_speed_mhz;
>>> + div = data->hw.highest_perf;
>>> +
>>> + return (unsigned int)(mul / div) * data->hw.lowest_perf *
>>> + 1000;
>>
>> No risk of the cast chopping off set bits?
>
> Mul shall be uint64_t, highest_perf and lowest_perf shall be uint8_t, and
> normally
> the equation output shall not be a too big value...
> If you think it's safer to do cast after comparing with the UINT32_MAX, I
> will add the check
Well, I can only say as much: Dividing a uint64_t by a uint8_t has ample
opportunity for there being more than 32 significant bits in the result.
>>> + if ( !(val & AMD_CPPC_ENABLE) )
>>> + {
>>> + val |= AMD_CPPC_ENABLE;
>>> + if ( wrmsr_safe(MSR_AMD_CPPC_ENABLE, val) )
>>> + {
>>> + amd_pstate_err(policy->cpu,
>> "wrmsr_safe(MSR_AMD_CPPC_ENABLE, %lx)\n", val);
>>> + data->err = -EINVAL;
>>> + return;
>>> + }
>>> + }
>>
>> Do you really need to enable befor reading ...
>>
>>> + if ( rdmsr_safe(MSR_AMD_CPPC_CAP1, data->amd_caps) )
>>
>> ... capabilities and ...
>>
>
> Yes.
> Only when CPPC is enabled, the hardware will calculate the processor’s
> performance capabilities and
> initialize the performance level fields in the CPPC capability registers.
> See 17.6.3
> https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
Ah yes, I see. Yet then the text there also says that the ENABLE bit can't
be cleared (except by reset), so your error handling is questionable.
>>> + {
>>> + amd_pstate_err(policy->cpu, "rdmsr_safe(MSR_AMD_CPPC_CAP1)\n");
>>> + goto error;
>>> + }
>>> +
>>> + if ( data->hw.highest_perf == 0 || data->hw.lowest_perf == 0 ||
>>> + data->hw.nominal_perf == 0 || data->hw.lowest_nonlinear_perf == 0
>>> )
>>> + {
>>> + amd_pstate_err(policy->cpu, "Platform malfunction, read CPPC
>> highest_perf: %u, lowest_perf: %u, nominal_perf: %u, lowest_nonlinear_perf:
>> %u
>> zero value\n",
>>> + data->hw.highest_perf, data->hw.lowest_perf,
>>> + data->hw.nominal_perf,
>>> data->hw.lowest_nonlinear_perf);
>>> + goto error;
>>> + }
>>> +
>>> + min_freq = amd_get_min_freq(data);
>>> + nominal_freq = amd_get_nominal_freq(data);
>>> + max_freq = amd_get_max_freq(data);
>>> + if ( min_freq > max_freq )
>>> + {
>>> + amd_pstate_err(policy->cpu, "min_freq(%u) or max_freq(%u) value is
>> incorrect\n",
>>> + min_freq, max_freq);
>>> + goto error;
>>> + }
>>
>> ... checking them? Error handling would be easier if you enabled only
>> afterwards.
>>
>>> + highest_perf = data->hw.highest_perf;
>>> + nominal_perf = data->hw.nominal_perf;
>>> +
>>> + if ( highest_perf <= nominal_perf )
>>> + return;
>>> +
>>> + policy->turbo = CPUFREQ_TURBO_ENABLED; }
>>
>> And why the helper variables in the first place?
>
> Sorry, maybe a bit more specific, got confused about the question ;/
With local variables like these, you end up with more code volume for
no real gain. IOW what's wrong with
if ( data->hw.highest_perf <= data->hw.nominal_perf )
return;
? (As an aside, even if the variables were actually useful, it would
still be better to have them have initializers than to use separate
declarations and assignments.)
Jan
