On Mon, Oct 7, 2024, at 16:04, Rafael J. Wysocki wrote: > 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? Yes. I would like to have all of arm64 and riscv be able to turn off HAS_IOPORT eventually, but nothing depends on doing this when ACPI is enabled. >> 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. Right, I see. >> --- 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. Understood. Does that mean that on the flip-side we can change the drivers/acpi/osl.c portion to turn acpi_os_read_port() and acpi_os_write_port() into a runtime error for configurations without port I/O, without causing the same maintenance overhead? The version below builds fine and doesn't touch acpica but it's a bit harder to predict what would happen at runtime. Arnd 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/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 b73b3aa92f3f..326b73ae77a9 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..04d3864073ba 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -642,6 +642,11 @@ acpi_status acpi_os_read_port(acpi_io_address port, u32 *value, u32 width) { u32 dummy; + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { + *value = BIT_MASK(width); + return AE_NOT_IMPLEMENTED; + } + if (value) *value = 0; else @@ -665,6 +670,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) { 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 =