| Message ID | c2809391c877dd5842389aaf87bf2b5fce5dc866.1711335714.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Needs ACK |
| Headers | show |
| Series | AMD Pstate Fixes And Enhancements | expand |
On Mon, Mar 25, 2024 at 11:03:25AM +0800, Yuan, Perry wrote: > The amd-pstate driver cannot work when the min_freq, nominal_freq or > the max_freq is zero. When this happens it is prudent to error out > early on rather than waiting failing at the time of the governor > initialization. > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > Tested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 132330b4942f..6708c436e1a2 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -839,9 +839,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) > nominal_freq = READ_ONCE(cpudata->nominal_freq); > lowest_nonlinear_freq = READ_ONCE(cpudata->lowest_nonlinear_freq); > > - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { > - dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n", > - min_freq, max_freq); > + if (min_freq <= 0 || max_freq <= 0 || > + nominal_freq <= 0 || min_freq > max_freq) { > + dev_err(dev, > + "min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect\n", > + min_freq, max_freq, nominal_freq); I suggest that we add one comment to remind that should be the error of ACPI table or BIOS. > ret = -EINVAL; > goto free_cpudata1; > } > @@ -1299,9 +1301,11 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > max_freq = READ_ONCE(cpudata->max_freq); > nominal_freq = READ_ONCE(cpudata->nominal_freq); > lowest_nonlinear_freq = READ_ONCE(cpudata->lowest_nonlinear_freq); > - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { > - dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n", > - min_freq, max_freq); > + if (min_freq <= 0 || max_freq <= 0 || > + nominal_freq <= 0 || min_freq > max_freq) { > + dev_err(dev, > + "min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n", > + min_freq, max_freq, nominal_freq); The same with above. With that fixed, patch is Acked-by: Huang Rui <ray.huang@amd.com> Thanks, Ray
[AMD Official Use Only - General] Regards. Perry > -----Original Message----- > From: Huang, Ray <Ray.Huang@amd.com> > Sent: Monday, April 15, 2024 10:59 PM > To: Yuan, Perry <Perry.Yuan@amd.com> > Cc: rafael.j.wysocki@intel.com; Limonciello, Mario > <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Shenoy, Gautham > Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav > <Borislav.Petkov@amd.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Huang, Shimmer > <Shimmer.Huang@amd.com>; oleksandr@natalenko.name; Du, Xiaojian > <Xiaojian.Du@amd.com>; Meng, Li (Jassmine) <Li.Meng@amd.com>; linux- > pm@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v10 5/8] cpufreq: amd-pstate: Bail out if > min/max/nominal_freq is 0 > > On Mon, Mar 25, 2024 at 11:03:25AM +0800, Yuan, Perry wrote: > > The amd-pstate driver cannot work when the min_freq, nominal_freq or > > the max_freq is zero. When this happens it is prudent to error out > > early on rather than waiting failing at the time of the governor > > initialization. > > > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > > Tested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > --- > > drivers/cpufreq/amd-pstate.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > b/drivers/cpufreq/amd-pstate.c index 132330b4942f..6708c436e1a2 > 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -839,9 +839,11 @@ static int amd_pstate_cpu_init(struct > cpufreq_policy *policy) > > nominal_freq = READ_ONCE(cpudata->nominal_freq); > > lowest_nonlinear_freq = READ_ONCE(cpudata- > >lowest_nonlinear_freq); > > > > - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { > > - dev_err(dev, "min_freq(%d) or max_freq(%d) value is > incorrect\n", > > - min_freq, max_freq); > > + if (min_freq <= 0 || max_freq <= 0 || > > + nominal_freq <= 0 || min_freq > max_freq) { > > + dev_err(dev, > > + "min_freq(%d) or max_freq(%d) or nominal_freq > (%d) value is incorrect\n", > > + min_freq, max_freq, nominal_freq); > > I suggest that we add one comment to remind that should be the error of > ACPI table or BIOS. Ok, thanks for comment, will add one more comment in v11. > > > ret = -EINVAL; > > goto free_cpudata1; > > } > > @@ -1299,9 +1301,11 @@ static int amd_pstate_epp_cpu_init(struct > cpufreq_policy *policy) > > max_freq = READ_ONCE(cpudata->max_freq); > > nominal_freq = READ_ONCE(cpudata->nominal_freq); > > lowest_nonlinear_freq = READ_ONCE(cpudata- > >lowest_nonlinear_freq); > > - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { > > - dev_err(dev, "min_freq(%d) or max_freq(%d) value is > incorrect\n", > > - min_freq, max_freq); > > + if (min_freq <= 0 || max_freq <= 0 || > > + nominal_freq <= 0 || min_freq > max_freq) { > > + dev_err(dev, > > + "min_freq(%d) or max_freq(%d) or nominal_freq(%d) > value is incorrect\n", > > + min_freq, max_freq, nominal_freq); > > The same with above. > > With that fixed, patch is Acked-by: Huang Rui <ray.huang@amd.com> > > Thanks, > Ray Thanks for the ACK. Perry.
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 132330b4942f..6708c436e1a2 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -839,9 +839,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) nominal_freq = READ_ONCE(cpudata->nominal_freq); lowest_nonlinear_freq = READ_ONCE(cpudata->lowest_nonlinear_freq); - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { - dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n", - min_freq, max_freq); + if (min_freq <= 0 || max_freq <= 0 || + nominal_freq <= 0 || min_freq > max_freq) { + dev_err(dev, + "min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect\n", + min_freq, max_freq, nominal_freq); ret = -EINVAL; goto free_cpudata1; } @@ -1299,9 +1301,11 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) max_freq = READ_ONCE(cpudata->max_freq); nominal_freq = READ_ONCE(cpudata->nominal_freq); lowest_nonlinear_freq = READ_ONCE(cpudata->lowest_nonlinear_freq); - if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) { - dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n", - min_freq, max_freq); + if (min_freq <= 0 || max_freq <= 0 || + nominal_freq <= 0 || min_freq > max_freq) { + dev_err(dev, + "min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n", + min_freq, max_freq, nominal_freq); ret = -EINVAL; goto free_cpudata1; }