Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT

From: Rafael J. Wysocki
Date: Mon Oct 07 2024 - 12:05:13 EST

Next message: Leon Romanovsky: "Re: of c4iw_fill_res_qp_entry"
Previous message: Rob Herring (Arm): "Re: [PATCH 1/2] dt-bindings: pinctrl: amlogic,meson8-pinctrl-aobus: add reg for aobus pinctrl nodes"
In reply to: Arnd Bergmann: "[PATCH] acpi: allow building without CONFIG_HAS_IOPORT"
Next in thread: Arnd Bergmann: "Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Oct 4, 2024 at 10:48 PM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
>
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> 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.

Can addressing this be deferred to that point?

> 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
>
> In function 'acpi_ec_read_status',
> inlined from 'advance_transaction' at drivers/acpi/ec.c:665:11:
> include/asm-generic/io.h:542:14: error: call to '_inb' declared with attribute error: inb()) requires CONFIG_HAS_IOPORT
>
> Since the embedded controller can only exist when port I/O is
> active, it makes sense to disable that code on targets that don't
> have it. The same is true for anything using acpi_os_read_port()
> and similar functions.
>
> Add compile-time conditionals around all of those and their callers.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> Should this be split up into smaller patches?

No need, but the ACPICA part is kind of nasty.

> ---
> drivers/acpi/Kconfig | 1 +
> drivers/acpi/Makefile | 2 +-
> drivers/acpi/acpica/Makefile | 4 +++-
> drivers/acpi/acpica/evhandler.c | 3 ++-
> drivers/acpi/acpica/exregion.c | 2 ++
> drivers/acpi/acpica/hwregs.c | 6 ++++--
> drivers/acpi/acpica/hwxface.c | 3 ++-
> drivers/acpi/apei/apei-base.c | 4 ++++
> drivers/acpi/bus.c | 9 ++++++---
> drivers/acpi/cppc_acpi.c | 6 ++++--
> drivers/acpi/osl.c | 2 ++
> drivers/acpi/processor_perflib.c | 3 ++-
> drivers/acpi/scan.c | 3 ++-
> 13 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index b8924077163b..5ec58c4e0332 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -134,6 +134,7 @@ config ACPI_REV_OVERRIDE_POSSIBLE
>
> config ACPI_EC_DEBUGFS
> tristate "EC read/write access through /sys/kernel/debug/ec"
> + depends on HAS_IOPORT
> help
> Say N to disable Embedded Controller /sys/kernel/debug interface
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 61ca4afe83dc..132357815324 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -41,7 +41,7 @@ acpi-y += resource.o
> acpi-y += acpi_processor.o
> acpi-y += processor_core.o
> acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> -acpi-y += ec.o
> +acpi-$(CONFIG_HAS_IOPORT) += ec.o
> acpi-$(CONFIG_ACPI_DOCK) += dock.o
> acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o
> obj-$(CONFIG_ACPI_MCFG) += pci_mcfg.o
> diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile
> index 8d18af396de9..9ba5e71348cb 100644
> --- a/drivers/acpi/acpica/Makefile
> +++ b/drivers/acpi/acpica/Makefile
> @@ -80,10 +80,12 @@ acpi-y += \
> hwgpe.o \
> hwregs.o \
> hwsleep.o \
> - hwvalid.o \
> hwxface.o \
> hwxfsleep.o
>
> +acpi-$(CONFIG_HAS_IOPORT) += \
> + hwvalid.o
> +
> acpi-$(CONFIG_PCI) += hwpci.o
> acpi-$(ACPI_FUTURE_USAGE) += hwtimer.o
>
> diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
> index 1c8cb6d924df..20f61936ff9b 100644
> --- a/drivers/acpi/acpica/evhandler.c
> +++ b/drivers/acpi/acpica/evhandler.c
> @@ -358,12 +358,13 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
> handler = acpi_ex_system_memory_space_handler;
> setup = acpi_ev_system_memory_region_setup;
> break;
> -
> +#ifdef CONFIG_HAS_IOPORT
> case ACPI_ADR_SPACE_SYSTEM_IO:
>
> handler = acpi_ex_system_io_space_handler;
> setup = acpi_ev_io_space_region_setup;
> break;
> +#endif

All changes like the above in the ACPICA code potentially increase the
number of times when upstream ACPICA patches will have to be ported to
Linux manually, which in turn increases the number of potential
mistakes in the process.

I'd rather avoid making them, if possible.

> #ifdef ACPI_PCI_CONFIGURED
> case ACPI_ADR_SPACE_PCI_CONFIG:
>
> diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> index c49b9f8de723..8f96828614ed 100644
> --- a/drivers/acpi/acpica/exregion.c
> +++ b/drivers/acpi/acpica/exregion.c
> @@ -261,6 +261,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> return_ACPI_STATUS(status);
> }
>
> +#ifdef CONFIG_HAS_IOPORT
> /*******************************************************************************
> *
> * FUNCTION: acpi_ex_system_io_space_handler
> @@ -319,6 +320,7 @@ acpi_ex_system_io_space_handler(u32 function,
>
> return_ACPI_STATUS(status);
> }
> +#endif
>
> #ifdef ACPI_PCI_CONFIGURED
> /*******************************************************************************
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index f62d5d024205..845d88a01272 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -239,7 +239,8 @@ acpi_status acpi_hw_read(u64 *value, struct acpi_generic_address *reg)
> ACPI_DIV_8
> (access_width),
> &value64, access_width);
> - } else { /* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
> + } else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
> + /* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
>
> status = acpi_hw_read_port((acpi_io_address)
> address +
> @@ -336,7 +337,8 @@ acpi_status acpi_hw_write(u64 value, struct acpi_generic_address *reg)
> ACPI_DIV_8
> (access_width),
> value64, access_width);
> - } else { /* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
> + } else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
> + /* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
>
> status = acpi_hw_write_port((acpi_io_address)
> address +
> diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c
> index c31f803995c6..022e706e10a1 100644
> --- a/drivers/acpi/acpica/hwxface.c
> +++ b/drivers/acpi/acpica/hwxface.c
> @@ -45,7 +45,8 @@ acpi_status acpi_reset(void)
> return_ACPI_STATUS(AE_NOT_EXIST);
> }
>
> - if (reset_reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> + if (IS_ENABLED(CONFIG_HAS_IOPORT) &&
> + reset_reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> /*
> * For I/O space, write directly to the OSL. This bypasses the port
> * validation mechanism, which may block a valid write to the reset
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index c7c26872f4ce..19357f951bae 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -661,12 +661,14 @@ int apei_read(u64 *val, struct acpi_generic_address *reg)
> if (ACPI_FAILURE(status))
> return -EIO;
> break;
> +#ifdef CONFIG_HAS_IOPORT
> case ACPI_ADR_SPACE_SYSTEM_IO:
> status = acpi_os_read_port(address, (u32 *)val,
> access_bit_width);
> if (ACPI_FAILURE(status))
> return -EIO;
> break;
> +#endif
> default:
> return -EINVAL;
> }
> @@ -694,11 +696,13 @@ int apei_write(u64 val, struct acpi_generic_address *reg)
> if (ACPI_FAILURE(status))
> return -EIO;
> break;
> +#ifdef CONFIG_HAS_IOPORT
> case ACPI_ADR_SPACE_SYSTEM_IO:
> status = acpi_os_write_port(address, val, access_bit_width);
> if (ACPI_FAILURE(status))
> return -EIO;
> break;
> +#endif
> default:
> return -EINVAL;
> }
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 16917dc3ad60..535d6a72ce1b 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1356,7 +1356,8 @@ static int __init acpi_bus_init(void)
> * Do that before calling acpi_initialize_objects() which may trigger EC
> * address space accesses.
> */
> - acpi_ec_ecdt_probe();
> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> + acpi_ec_ecdt_probe();
>
> status = acpi_enable_subsystem(ACPI_NO_ACPI_ENABLE);
> if (ACPI_FAILURE(status)) {
> @@ -1391,7 +1392,8 @@ static int __init acpi_bus_init(void)
> * Maybe EC region is required at bus_scan/acpi_get_devices. So it
> * is necessary to enable it as early as possible.
> */
> - acpi_ec_dsdt_probe();
> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> + acpi_ec_dsdt_probe();
>
> pr_info("Interpreter enabled\n");
>
> @@ -1464,7 +1466,8 @@ static int __init acpi_init(void)
> acpi_arm_init();
> acpi_riscv_init();
> acpi_scan_init();
> - acpi_ec_init();
> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> + acpi_ec_init();
> acpi_debugfs_init();
> acpi_sleep_proc_init();
> acpi_wakeup_device_init();
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 5b06e236aabe..cb545cdfdc19 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;
>
> @@ -1090,7 +1091,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..28eb5ff123a9 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -638,6 +638,7 @@ u64 acpi_os_get_timer(void)
> (ACPI_100NSEC_PER_SEC / HZ);
> }
>
> +#ifdef CONFIG_HAS_IOPORT
> acpi_status acpi_os_read_port(acpi_io_address port, u32 *value, u32 width)
> {
> u32 dummy;
> @@ -680,6 +681,7 @@ acpi_status acpi_os_write_port(acpi_io_address port, u32 value, u32 width)
> }
>
> EXPORT_SYMBOL(acpi_os_write_port);
> +#endif
>
> int acpi_os_read_iomem(void __iomem *virt_addr, u64 *value, u32 width)
> {
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index 4265814c74f8..8be453d89ef8 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -455,7 +455,8 @@ int acpi_processor_pstate_control(void)
> {
> acpi_status status;
>
> - if (!acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
> + if (!IS_ENABLED(CONFIG_HAS_IOPORT) ||
> + !acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
> return 0;
>
> pr_debug("Writing pstate_control [0x%x] to smi_command [0x%x]\n",
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7ecc401fb97f..9d5e6dd542bf 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2293,7 +2293,8 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
> if (device->handler)
> goto ok;
>
> - acpi_ec_register_opregions(device);
> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> + acpi_ec_register_opregions(device);
>
> if (!device->flags.initialized) {
> device->flags.power_manageable =
> --
> 2.39.2
>
>

Next message: Leon Romanovsky: "Re: of c4iw_fill_res_qp_entry"
Previous message: Rob Herring (Arm): "Re: [PATCH 1/2] dt-bindings: pinctrl: amlogic,meson8-pinctrl-aobus: add reg for aobus pinctrl nodes"
In reply to: Arnd Bergmann: "[PATCH] acpi: allow building without CONFIG_HAS_IOPORT"
Next in thread: Arnd Bergmann: "Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]