Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT
From: Arnd Bergmann
Date: Mon Oct 07 2024 - 15:23:15 EST
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 =