acpi_atomic_read() requirements

Hello,

You recently added acpi_atomic_read() and acpi_atomic_write(). These are similar to acpi_read() and acpi_write(), but differ mainly in two ways:

- The atomic ones can be used in interrupt context, because the ioremap() (which may block) can be done earlier by acpi_pre_map_gar()

- The atomic ones do 64-bit accesses directly with readq() rather than splitting them into two 32-bit accesses as acpi_read() does.

It's obvious to me that you need the first property (usable in interrupt context). Do you also rely on the second property (doing a single 64-bit access rather than two 32-bit accesses)?

I'm curious because there's another path that performs memory accesses from interrupt context, using acpi_read() and acpi_os_read_memory(). The ACPI IRQ handler reads the PM1 Status register to learn the interrupt source. If this is memory mapped (not in I/O port space), we currently panic because ioremap() can't be called in interrupt context:

http://marc.info/?l=linux-acpi&m=128094238920118&w=2

I wrote a patch that fixes this by premapping these areas so the ioremap() happens at boot-time rather than interrupt-time. This works fine, but it ends up looking very much like the atomic functions you added. I'd like to integrate them somehow rather than adding more code that does basically the same thing as your code.

If your APEI code that uses acpi_atomic_read()/write() doesn't require the single 64-bit access, it might be possible to keep the premapping support, i.e., acpi_pre_map_gar(), change acpi_os_read_memory() to check the acpi_iomaps list first and only do the ioremap() if it doesn't find anything, remove acpi_atomic_read(), and just use acpi_read() instead.

Quoted 12 lines · Bjorn Helgaas

Hello,

You recently added acpi_atomic_read() and acpi_atomic_write(). These are similar to acpi_read() and acpi_write(), but differ mainly in two ways:

- The atomic ones can be used in interrupt context, because the ioremap() (which may block) can be done earlier by acpi_pre_map_gar()

- The atomic ones do 64-bit accesses directly with readq() rather than splitting them into two 32-bit accesses as acpi_read() does.

Quoted 3 lines

It's obvious to me that you need the first property (usable in interrupt context). Do you also rely on the second property (doing a single 64-bit access rather than two 32-bit accesses)?

We don't rely on the 64-bit access. But I am not sure whether all corresponding hardware is OK for splitting accessing. But maybe we can fix it in the future if necessary. For example adding an acpi_os_read_memory64.

Quoted 16 lines

I'm curious because there's another path that performs memory accesses from interrupt context, using acpi_read() and acpi_os_read_memory(). The ACPI IRQ handler reads the PM1 Status register to learn the interrupt source. If this is memory mapped (not in I/O port space), we currently panic because ioremap() can't be called in interrupt context:

http://marc.info/?l=linux-acpi&m=128094238920118&w=2

I wrote a patch that fixes this by premapping these areas so the ioremap() happens at boot-time rather than interrupt-time. This works fine, but it ends up looking very much like the atomic functions you added. I'd like to integrate them somehow rather than adding more code that does basically the same thing as your code.

Quoted 6 lines

If your APEI code that uses acpi_atomic_read()/write() doesn't require the single 64-bit access, it might be possible to keep the premapping support, i.e., acpi_pre_map_gar(), change acpi_os_read_memory() to check the acpi_iomaps list first and only do the ioremap() if it doesn't find anything, remove acpi_atomic_read(), and just use acpi_read() instead.

That is OK for me.

Good. Unless there's a spec requirement for 64-bit accesses in this area, I think it's better to do it the same way as acpi_read(), just so we're as consistent as possible.

Quoted 3 lines

Good. Unless there's a spec requirement for 64-bit accesses in this area, I think it's better to do it the same way as acpi_read(), just so we're as consistent as possible.

I'm not sure if the spec explicit asks for it, but doing a full 64bit access would be in the spirit of it at least.

On the other hand I would expect hardware that really requires 64bit to be rare because 32bit OS would always have some trouble with it.