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 =
