[PATCH 1/2] [v3] acpi: processor_perflib: extend X86 dependency

[PATCH 1/2] [v3] acpi: processor_perflib: extend X86 dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The majority of the processor_perflib code is only used by cpufreq
drivers on the x86 architecture and makes no sense without the
x86 SMI interactions that rely on I/O port access.

Replace the existing #ifdef checks with one that covers all of the
code that is only used by x86 drivers, saving a little bit
of kernel code size on other architectures.

There is likely more code under CONFIG_ACPI_PROCESSOR that falls
into this category, but changing those would require a larger
rework.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This is not needed for correctness, only as a small optimization.

v3: fix build warning
---
 drivers/acpi/processor_perflib.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 4265814c74f8..53996f1a2d80 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -24,8 +24,6 @@
 
 #define ACPI_PROCESSOR_FILE_PERFORMANCE	"performance"
 
-static DEFINE_MUTEX(performance_mutex);
-
 /*
  * _PPC support is implemented as a CPUfreq policy notifier:
  * This means each time a CPUfreq driver registered also with
@@ -209,6 +207,10 @@ void acpi_processor_ppc_exit(struct cpufreq_policy *policy)
 	}
 }
 
+#ifdef CONFIG_X86
+
+static DEFINE_MUTEX(performance_mutex);
+
 static int acpi_processor_get_performance_control(struct acpi_processor *pr)
 {
 	int result = 0;
@@ -267,7 +269,6 @@ static int acpi_processor_get_performance_control(struct acpi_processor *pr)
 	return result;
 }
 
-#ifdef CONFIG_X86
 /*
  * Some AMDs have 50MHz frequency multiples, but only provide 100MHz rounding
  * in their ACPI data. Calculate the real values and fix up the _PSS data.
@@ -298,9 +299,6 @@ static void amd_fixup_frequency(struct acpi_processor_px *px, int i)
 			px->core_frequency = (100 * (fid + 8)) >> did;
 	}
 }
-#else
-static void amd_fixup_frequency(struct acpi_processor_px *px, int i) {};
-#endif
 
 static int acpi_processor_get_performance_states(struct acpi_processor *pr)
 {
@@ -440,13 +438,11 @@ int acpi_processor_get_performance_info(struct acpi_processor *pr)
 	 * the BIOS is older than the CPU and does not know its frequencies
 	 */
  update_bios:
-#ifdef CONFIG_X86
 	if (acpi_has_method(pr->handle, "_PPC")) {
 		if(boot_cpu_has(X86_FEATURE_EST))
 			pr_warn(FW_BUG "BIOS needs update for CPU "
 			       "frequency support\n");
 	}
-#endif
 	return result;
 }
 EXPORT_SYMBOL_GPL(acpi_processor_get_performance_info);
@@ -788,3 +784,4 @@ void acpi_processor_unregister_performance(unsigned int cpu)
 	mutex_unlock(&performance_mutex);
 }
 EXPORT_SYMBOL(acpi_processor_unregister_performance);
+#endif
-- 
2.39.5

[PATCH 2/2] [v3] acpi: allow building without CONFIG_HAS_IOPORT

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

CONFIG_HAS_IOPORT will soon become optional and cause a build time
failure when it is disabled but a driver calls inb()/outb(). At the
moment, all architectures that can support ACPI have port I/O, but this
is not necessarily the case in the future on non-x86 architectures.
The result is a set of errors like:

drivers/acpi/osl.c: In function 'acpi_os_read_port':
include/asm-generic/io.h:542:14: error: call to '_inb' declared with attribute error: inb()) requires CONFIG_HAS_IOPORT

Nothing should actually call these functions in this configuration,
and if it does, the result would be undefined behavior today, possibly
a NULL pointer dereference.

Change the low-level functions to return a proper error code when
HAS_IOPORT is disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v3: fix the returned value and add a comment
---
 drivers/acpi/cppc_acpi.c |  6 ++++--
 drivers/acpi/osl.c       | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 1a40f0514eaa..3757424b715f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1017,7 +1017,8 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 	*val = 0;
 	size = GET_BIT_WIDTH(reg);
 
-	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (IS_ENABLED(CONFIG_HAS_IOPORT) &&
+	    reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
 		u32 val_u32;
 		acpi_status status;
 
@@ -1091,7 +1092,8 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 
 	size = GET_BIT_WIDTH(reg);
 
-	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (IS_ENABLED(CONFIG_HAS_IOPORT) &&
+	    reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
 		acpi_status status;
 
 		status = acpi_os_write_port((acpi_io_address)reg->address,
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 78a81969d90e..8ab1802c164b 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -642,6 +642,15 @@ acpi_status acpi_os_read_port(acpi_io_address port, u32 *value, u32 width)
 {
 	u32 dummy;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+		/*
+		 * set all-1 result as if reading from non-existing
+		 * I/O port
+		 */
+		*value = GENMASK(width, 0);
+		return AE_NOT_IMPLEMENTED;
+	}
+
 	if (value)
 		*value = 0;
 	else
@@ -665,6 +674,9 @@ EXPORT_SYMBOL(acpi_os_read_port);
 
 acpi_status acpi_os_write_port(acpi_io_address port, u32 value, u32 width)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return AE_NOT_IMPLEMENTED;
+
 	if (width <= 8) {
 		outb(value, port);
 	} else if (width <= 16) {
-- 
2.39.5

Re: [PATCH 2/2] [v3] acpi: allow building without CONFIG_HAS_IOPORT

[Andy Shevchenko]

On Wed, Oct 30, 2024 at 12:36:41PM +0000, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> CONFIG_HAS_IOPORT will soon become optional and cause a build time
> failure when it is disabled but a driver calls inb()/outb(). At the
> moment, all architectures that can support ACPI have port I/O, but this
> is not necessarily the case in the future on non-x86 architectures.
> The result is a set of errors like:
> 
> drivers/acpi/osl.c: In function 'acpi_os_read_port':
> include/asm-generic/io.h:542:14: error: call to '_inb' declared with attribute error: inb()) requires CONFIG_HAS_IOPORT
> 
> Nothing should actually call these functions in this configuration,
> and if it does, the result would be undefined behavior today, possibly
> a NULL pointer dereference.
> 
> Change the low-level functions to return a proper error code when
> HAS_IOPORT is disabled.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +	if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> +		/*
> +		 * set all-1 result as if reading from non-existing
> +		 * I/O port
> +		 */

Don't know if Rafael can / want to tweak this, but would be nice to follow
standard style for multi-line comments.

		/*
		 * Set all-1 result as if reading from non-existing
		 * I/O port.
		 */

> +		*value = GENMASK(width, 0);
> +		return AE_NOT_IMPLEMENTED;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] [v3] acpi: allow building without CONFIG_HAS_IOPORT

[Andy Shevchenko]

On Wed, Oct 30, 2024 at 05:13:15PM +0200, Andy Shevchenko wrote:
> On Wed, Oct 30, 2024 at 12:36:41PM +0000, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > CONFIG_HAS_IOPORT will soon become optional and cause a build time
> > failure when it is disabled but a driver calls inb()/outb(). At the
> > moment, all architectures that can support ACPI have port I/O, but this
> > is not necessarily the case in the future on non-x86 architectures.
> > The result is a set of errors like:
> > 
> > drivers/acpi/osl.c: In function 'acpi_os_read_port':
> > include/asm-generic/io.h:542:14: error: call to '_inb' declared with attribute error: inb()) requires CONFIG_HAS_IOPORT
> > 
> > Nothing should actually call these functions in this configuration,
> > and if it does, the result would be undefined behavior today, possibly
> > a NULL pointer dereference.
> > 
> > Change the low-level functions to return a proper error code when
> > HAS_IOPORT is disabled.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> > +	if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> > +		/*
> > +		 * set all-1 result as if reading from non-existing
> > +		 * I/O port
> > +		 */
> 
> Don't know if Rafael can / want to tweak this, but would be nice to follow
> standard style for multi-line comments.
> 
> 		/*
> 		 * Set all-1 result as if reading from non-existing
> 		 * I/O port.
> 		 */
> 
> > +		*value = GENMASK(width, 0);

Actually, shouldn't this be width - 1 ?

> > +		return AE_NOT_IMPLEMENTED;
> > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] [v3] acpi: processor_perflib: extend X86 dependency

[Rafael J. Wysocki]

On Wed, Oct 30, 2024 at 1:37 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The majority of the processor_perflib code is only used by cpufreq
> drivers on the x86 architecture and makes no sense without the
> x86 SMI interactions that rely on I/O port access.
>
> Replace the existing #ifdef checks with one that covers all of the
> code that is only used by x86 drivers, saving a little bit
> of kernel code size on other architectures.
>
> There is likely more code under CONFIG_ACPI_PROCESSOR that falls
> into this category, but changing those would require a larger
> rework.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This is not needed for correctness, only as a small optimization.
>
> v3: fix build warning
> ---
>  drivers/acpi/processor_perflib.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index 4265814c74f8..53996f1a2d80 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -24,8 +24,6 @@
>
>  #define ACPI_PROCESSOR_FILE_PERFORMANCE        "performance"
>
> -static DEFINE_MUTEX(performance_mutex);
> -
>  /*
>   * _PPC support is implemented as a CPUfreq policy notifier:
>   * This means each time a CPUfreq driver registered also with
> @@ -209,6 +207,10 @@ void acpi_processor_ppc_exit(struct cpufreq_policy *policy)
>         }
>  }
>
> +#ifdef CONFIG_X86
> +
> +static DEFINE_MUTEX(performance_mutex);
> +
>  static int acpi_processor_get_performance_control(struct acpi_processor *pr)
>  {
>         int result = 0;
> @@ -267,7 +269,6 @@ static int acpi_processor_get_performance_control(struct acpi_processor *pr)
>         return result;
>  }
>
> -#ifdef CONFIG_X86
>  /*
>   * Some AMDs have 50MHz frequency multiples, but only provide 100MHz rounding
>   * in their ACPI data. Calculate the real values and fix up the _PSS data.
> @@ -298,9 +299,6 @@ static void amd_fixup_frequency(struct acpi_processor_px *px, int i)
>                         px->core_frequency = (100 * (fid + 8)) >> did;
>         }
>  }
> -#else
> -static void amd_fixup_frequency(struct acpi_processor_px *px, int i) {};
> -#endif
>
>  static int acpi_processor_get_performance_states(struct acpi_processor *pr)
>  {
> @@ -440,13 +438,11 @@ int acpi_processor_get_performance_info(struct acpi_processor *pr)
>          * the BIOS is older than the CPU and does not know its frequencies
>          */
>   update_bios:
> -#ifdef CONFIG_X86
>         if (acpi_has_method(pr->handle, "_PPC")) {
>                 if(boot_cpu_has(X86_FEATURE_EST))
>                         pr_warn(FW_BUG "BIOS needs update for CPU "
>                                "frequency support\n");
>         }
> -#endif
>         return result;
>  }
>  EXPORT_SYMBOL_GPL(acpi_processor_get_performance_info);
> @@ -788,3 +784,4 @@ void acpi_processor_unregister_performance(unsigned int cpu)
>         mutex_unlock(&performance_mutex);
>  }
>  EXPORT_SYMBOL(acpi_processor_unregister_performance);
> +#endif
> --

Applied along with the [2/2] as 6.13 material, thanks!