Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Oct 7, 2024, at 16:04, Rafael J. Wysocki wrote:
> On Fri, Oct 4, 2024 at 10:48 PM Arnd Bergmann <arnd@kernel.org> 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.
>
> 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@arndb.de>
>> ---
>> 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 =