acpi_atomic_read() requirements

acpi_atomic_read() requirements

[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.

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.

Bjorn

Re: acpi_atomic_read() requirements

[Huang Ying]

On Fri, 2010-09-03 at 09:34 +0800, Bjorn Helgaas wrote:
> 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)?

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.

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

That is OK for me.

Best Regards,
Huang Ying

Re: acpi_atomic_read() requirements

[Bjorn Helgaas]

On Thursday, September 02, 2010 07:49:59 pm Huang Ying wrote:
> On Fri, 2010-09-03 at 09:34 +0800, Bjorn Helgaas wrote:
> > 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)?
> 
> 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.

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 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.
> 
> That is OK for me.

Great, I'll experiment with this approach.

Thanks a lot for the help!

Bjorn

Re: acpi_atomic_read() requirements

[Andi Kleen]

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

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: acpi_atomic_read() requirements

[Bjorn Helgaas]

On Friday, September 03, 2010 03:17:05 am Andi Kleen wrote:
> > 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.

It would, and I hoped the spec would clarify that, but I didn't
see anything.

If there's a reason to have acpi_atomic_read() work differently
than acpi_read() in this respect, that's fine.  I just don't see
it yet.  If they *can* be made similar in this, I think we might
be able to collapse them into a single, smaller implementation.

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