| Message ID | 20201105125524.4409-9-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | cppc_cpufreq: fix, clarify and improve support | expand |
On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote: > > For errors parsing the _PSD domains, a separate domain is returned for > each CPU in the failed _PSD domain with no coordination (as per previous > comment). But contrary to the intention, the code was setting > CPUFREQ_SHARED_TYPE_ALL as coordination type. > > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing > the domain information. The function still return the error and the caller > is free to bail out the domain initialisation altogether in that case. > > Given that both functions return domains with a single CPU, this change > does not affect the functionality, but clarifies the intention. Is this related to any other patches in the series? > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/acpi/cppc_acpi.c | 2 +- > drivers/acpi/processor_perflib.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 75e36b909ae6..e1e46cc66eeb 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) > /* Assume no coordination on any error parsing domain info */ > cpumask_clear(domain->shared_cpu_map); > cpumask_set_cpu(cpu, domain->shared_cpu_map); > - domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + domain->shared_type = CPUFREQ_SHARED_TYPE_NONE; > > return -EFAULT; > } > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index 5909e8fa4013..5ce638537791 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance( > if (retval) { > cpumask_clear(pr->performance->shared_cpu_map); > cpumask_set_cpu(i, pr->performance->shared_cpu_map); > - pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE; > } > pr->performance = NULL; /* Will be set for real in register */ > } > -- > 2.17.1 >
Hi Rafael, On Thursday 05 Nov 2020 at 14:05:55 (+0100), Rafael J. Wysocki wrote: > On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote: > > > > For errors parsing the _PSD domains, a separate domain is returned for > > each CPU in the failed _PSD domain with no coordination (as per previous > > comment). But contrary to the intention, the code was setting > > CPUFREQ_SHARED_TYPE_ALL as coordination type. > > > > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing > > the domain information. The function still return the error and the caller > > is free to bail out the domain initialisation altogether in that case. > > > > Given that both functions return domains with a single CPU, this change > > does not affect the functionality, but clarifies the intention. > > Is this related to any other patches in the series? > It does not depend on any of the other patches. I first noticed this in acpi_get_psd_map() which is solely used by cppc_cpufreq.c, but looking some more into it showed processor_perflib.c's acpi_processor_preregister_performance() had the same inconsistency. I can submit this separately, if that works better. Thanks, Ionela. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Len Brown <lenb@kernel.org> > > Cc: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/acpi/cppc_acpi.c | 2 +- > > drivers/acpi/processor_perflib.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > > index 75e36b909ae6..e1e46cc66eeb 100644 > > --- a/drivers/acpi/cppc_acpi.c > > +++ b/drivers/acpi/cppc_acpi.c > > @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) > > /* Assume no coordination on any error parsing domain info */ > > cpumask_clear(domain->shared_cpu_map); > > cpumask_set_cpu(cpu, domain->shared_cpu_map); > > - domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; > > + domain->shared_type = CPUFREQ_SHARED_TYPE_NONE; > > > > return -EFAULT; > > } > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > index 5909e8fa4013..5ce638537791 100644 > > --- a/drivers/acpi/processor_perflib.c > > +++ b/drivers/acpi/processor_perflib.c > > @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance( > > if (retval) { > > cpumask_clear(pr->performance->shared_cpu_map); > > cpumask_set_cpu(i, pr->performance->shared_cpu_map); > > - pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL; > > + pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE; > > } > > pr->performance = NULL; /* Will be set for real in register */ > > } > > -- > > 2.17.1 > >
On Thursday, November 5, 2020 3:02:02 PM CET Ionela Voinescu wrote: > Hi Rafael, > > On Thursday 05 Nov 2020 at 14:05:55 (+0100), Rafael J. Wysocki wrote: > > On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote: > > > > > > For errors parsing the _PSD domains, a separate domain is returned for > > > each CPU in the failed _PSD domain with no coordination (as per previous > > > comment). But contrary to the intention, the code was setting > > > CPUFREQ_SHARED_TYPE_ALL as coordination type. > > > > > > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing > > > the domain information. The function still return the error and the caller > > > is free to bail out the domain initialisation altogether in that case. > > > > > > Given that both functions return domains with a single CPU, this change > > > does not affect the functionality, but clarifies the intention. > > > > Is this related to any other patches in the series? > > > > It does not depend on any of the other patches. I first noticed this in > acpi_get_psd_map() which is solely used by cppc_cpufreq.c, but looking > some more into it showed processor_perflib.c's > acpi_processor_preregister_performance() had the same inconsistency. > > I can submit this separately, if that works better. No need this time, but in general sending unrelated changes separately is less confusing. Thanks!
On 05-11-20, 12:55, Ionela Voinescu wrote: > For errors parsing the _PSD domains, a separate domain is returned for > each CPU in the failed _PSD domain with no coordination (as per previous > comment). But contrary to the intention, the code was setting > CPUFREQ_SHARED_TYPE_ALL as coordination type. > > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing > the domain information. The function still return the error and the caller > is free to bail out the domain initialisation altogether in that case. > > Given that both functions return domains with a single CPU, this change > does not affect the functionality, but clarifies the intention. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/acpi/cppc_acpi.c | 2 +- > drivers/acpi/processor_perflib.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 75e36b909ae6..e1e46cc66eeb 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) > /* Assume no coordination on any error parsing domain info */ > cpumask_clear(domain->shared_cpu_map); > cpumask_set_cpu(cpu, domain->shared_cpu_map); > - domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + domain->shared_type = CPUFREQ_SHARED_TYPE_NONE; > > return -EFAULT; > } > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index 5909e8fa4013..5ce638537791 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance( > if (retval) { > cpumask_clear(pr->performance->shared_cpu_map); > cpumask_set_cpu(i, pr->performance->shared_cpu_map); > - pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE; > } > pr->performance = NULL; /* Will be set for real in register */ > } Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 75e36b909ae6..e1e46cc66eeb 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) /* Assume no coordination on any error parsing domain info */ cpumask_clear(domain->shared_cpu_map); cpumask_set_cpu(cpu, domain->shared_cpu_map); - domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; + domain->shared_type = CPUFREQ_SHARED_TYPE_NONE; return -EFAULT; } diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c index 5909e8fa4013..5ce638537791 100644 --- a/drivers/acpi/processor_perflib.c +++ b/drivers/acpi/processor_perflib.c @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance( if (retval) { cpumask_clear(pr->performance->shared_cpu_map); cpumask_set_cpu(i, pr->performance->shared_cpu_map); - pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL; + pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE; } pr->performance = NULL; /* Will be set for real in register */ }
For errors parsing the _PSD domains, a separate domain is returned for each CPU in the failed _PSD domain with no coordination (as per previous comment). But contrary to the intention, the code was setting CPUFREQ_SHARED_TYPE_ALL as coordination type. Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing the domain information. The function still return the error and the caller is free to bail out the domain initialisation altogether in that case. Given that both functions return domains with a single CPU, this change does not affect the functionality, but clarifies the intention. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/acpi/cppc_acpi.c | 2 +- drivers/acpi/processor_perflib.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)