Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT

[Rafael J. Wysocki]

On Mon, Oct 7, 2024 at 9:23 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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?

Yes, that can be done.

> The version below builds fine and doesn't touch acpica but
> it's a bit harder to predict what would happen at runtime.

Sure.

> 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();

The above two changes mean that it is not necessary to compile either
acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() at all if
CONFIG_HAS_IOPORT is not enabled.

>
>         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();

And this means that the whole EC driver is not going to work at all then.

>         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) {

The above two changes look reasonable to me.

> 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;

All of the existing callers of acpi_processor_pstate_control() are x86
which has CONFIG_HAS_IOPORT AFAICS.

And if you care about the code size, acpi_processor_notify_smm() can
go away for !CONFIG_HAS_IOPORT too.

>         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);

I'd rather have an empty stub of acpi_ec_register_opregions() for
!CONFIG_HAS_IOPORT instead.

>
>         if (!device->flags.initialized) {
>                 device->flags.power_manageable =