| Message ID | 20241011061948.3211423-2-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] acpi: make EC support compile-time conditional | expand |
On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann 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 on non-x86 architectures. > The result is a set of errors like: > > drivers/acpi/osl.c: In function 'acpi_os_read_port': > include/asm-generic/io.h:542:14: error: call to '_inb' declared with attribute error: inb()) requires CONFIG_HAS_IOPORT > > Nothing should actually call these functions in this configuration, > and if it does, the result would be undefined behavior today, possibly > a NULL pointer dereference. > > Change the low-level functions to return a proper error code when > HAS_IOPORT is disabled. ... > + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { > + *value = BIT_MASK(width); > + return AE_NOT_IMPLEMENTED; Perhaps it has already been discussed, but why do we need to file value with semi-garbage when we know it's invalid anyway? > + }
On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: > > ... > >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { >> + *value = BIT_MASK(width); >> + return AE_NOT_IMPLEMENTED; > > Perhaps it has already been discussed, but why do we need to file value with > semi-garbage when we know it's invalid anyway? It's not strictly necessary, just precaution for possible callers that use the resulting data without checking the error code. The all-ones data is what an x86 PC would see when an I/O port is read that is not connected to any device. Arnd
On Fri, Oct 11, 2024 at 09:59:46AM +0000, Arnd Bergmann wrote: > On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: > > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: ... > >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { > >> + *value = BIT_MASK(width); > >> + return AE_NOT_IMPLEMENTED; > > > > Perhaps it has already been discussed, but why do we need to file value with > > semi-garbage when we know it's invalid anyway? > > It's not strictly necessary, just precaution for possible callers > that use the resulting data without checking the error code. Do you have any examples of that in the kernel? > The all-ones data is what an x86 PC would see when an I/O > port is read that is not connected to any device. Yes, but it's not what your code does.
On Fri, Oct 11, 2024 at 1:12 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 11, 2024 at 09:59:46AM +0000, Arnd Bergmann wrote: > > On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: > > > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: > > ... > > > >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { > > >> + *value = BIT_MASK(width); > > >> + return AE_NOT_IMPLEMENTED; > > > > > > Perhaps it has already been discussed, but why do we need to file value with > > > semi-garbage when we know it's invalid anyway? > > > > It's not strictly necessary, just precaution for possible callers > > that use the resulting data without checking the error code. > > Do you have any examples of that in the kernel? Yes, there are at least 2 cases. May not be relevant, though. > > The all-ones data is what an x86 PC would see when an I/O > > port is read that is not connected to any device. > > Yes, but it's not what your code does. Care to elaborate?
On Fri, Oct 11, 2024, at 11:12, Andy Shevchenko wrote: > On Fri, Oct 11, 2024 at 09:59:46AM +0000, Arnd Bergmann wrote: >> On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: >> > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: >> >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { >> >> + *value = BIT_MASK(width); >> >> + return AE_NOT_IMPLEMENTED; >> > >> > Perhaps it has already been discussed, but why do we need to file value with >> > semi-garbage when we know it's invalid anyway? >> >> It's not strictly necessary, just precaution for possible callers >> that use the resulting data without checking the error code. > > Do you have any examples of that in the kernel? drivers/acpi/processor_throttling.c: acpi_os_read_port((acpi_io_address) throttling->status_register. -- drivers/cpufreq/acpi-cpufreq.c- drivers/cpufreq/acpi-cpufreq.c: acpi_os_read_port(reg->address, &val, reg->bit_width); $ git grep ^[^=]*acpi_os_read_port drivers/acpi/processor_throttling.c: acpi_os_read_port(\ (acpi_io_address) throttling->status_register. drivers/cpufreq/acpi-cpufreq.c: acpi_os_read_port(reg->address, &val, reg->bit_width); >> The all-ones data is what an x86 PC would see when an I/O >> port is read that is not connected to any device. > > Yes, but it's not what your code does. My bad, I was confused about what BIT_MASK() does. I'll change it to "GENMASK(width, 0)", which should do what I intended. Arnd
On Fri, Oct 11, 2024 at 01:28:23PM +0200, Rafael J. Wysocki wrote: > On Fri, Oct 11, 2024 at 1:12 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 11, 2024 at 09:59:46AM +0000, Arnd Bergmann wrote: > > > On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: > > > > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: ... > > > >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { > > > >> + *value = BIT_MASK(width); > > > >> + return AE_NOT_IMPLEMENTED; > > > > > > > > Perhaps it has already been discussed, but why do we need to file value with > > > > semi-garbage when we know it's invalid anyway? > > > > > > It's not strictly necessary, just precaution for possible callers > > > that use the resulting data without checking the error code. > > > > Do you have any examples of that in the kernel? > > Yes, there are at least 2 cases. May not be relevant, though. Btw, may be we even can add the error check to them, dunno... > > > The all-ones data is what an x86 PC would see when an I/O > > > port is read that is not connected to any device. > > > > Yes, but it's not what your code does. > > Care to elaborate? Sure, but it seems Arnd already figured out that he set one bit only somewhere in the returned value, not what he stated in the explanation in this email thread.
On Fri, Oct 11, 2024 at 11:40:05AM +0000, Arnd Bergmann wrote: > On Fri, Oct 11, 2024, at 11:12, Andy Shevchenko wrote: > > On Fri, Oct 11, 2024 at 09:59:46AM +0000, Arnd Bergmann wrote: > >> On Fri, Oct 11, 2024, at 09:53, Andy Shevchenko wrote: > >> > On Fri, Oct 11, 2024 at 06:18:18AM +0000, Arnd Bergmann wrote: > >> >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { > >> >> + *value = BIT_MASK(width); > >> >> + return AE_NOT_IMPLEMENTED; > >> > > >> > Perhaps it has already been discussed, but why do we need to file value with > >> > semi-garbage when we know it's invalid anyway? > >> > >> It's not strictly necessary, just precaution for possible callers > >> that use the resulting data without checking the error code. > > > > Do you have any examples of that in the kernel? > > drivers/acpi/processor_throttling.c: acpi_os_read_port((acpi_io_address) throttling->status_register. > -- > drivers/cpufreq/acpi-cpufreq.c- > drivers/cpufreq/acpi-cpufreq.c: acpi_os_read_port(reg->address, &val, reg->bit_width); > > $ git grep ^[^=]*acpi_os_read_port > drivers/acpi/processor_throttling.c: acpi_os_read_port(\ (acpi_io_address) throttling->status_register. > drivers/cpufreq/acpi-cpufreq.c: acpi_os_read_port(reg->address, &val, reg->bit_width); May be we can add checks to them, but dunno... > >> The all-ones data is what an x86 PC would see when an I/O > >> port is read that is not connected to any device. > > > > Yes, but it's not what your code does. > > My bad, I was confused about what BIT_MASK() does. > I'll change it to "GENMASK(width, 0)", which should > do what I intended. Okay. Maybe also adding a comment that it's usual behaviour in response to the read from non-existing IO port? (Or for the curios it's all comes from the Data Bus on hardware being Open Drain an hence use of pull-up resistors and when there is no response on the bus, the default will be "All 1:s").
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 70af3fbbebe5..19342ccfabb9 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) {