Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT

[Rafael J. Wysocki]

On Thu, Oct 10, 2024 at 7:40 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Wed, Oct 9, 2024, at 19:40, Rafael J. Wysocki wrote:
> > On Mon, Oct 7, 2024 at 9:23 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >> 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.
>
> Correct, this is a result of removing ec.o from the list of objects.
>
> >>         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.
>
> Correct. The way I read ec.c it makes no sense if acpi_ec_write_cmd()
> and acpi_ec_write_data() don't actually access the registers. Is
> there anything in ec.c that still makes sense without port I/O?
>
> >> 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.
>
> Fair enough. Should I add stubs for all the global EC functions then?

Yes, please.

> I also wonder if there should be a separate CONFIG_ACPI_EC symbol
> that allows the EC logic to be turned off on non-x86 architectures
> even when HAS_IOPORT is otherwise enabled.

Yes, CONFIG_ACPI_EC would make sense IMV.

Thanks!