Re: [PATCH 0/2] ACPI: Re-factor and remove ./drivers/acpi/atomicio.[ch]

[Myron Stowe <myron.stowe@gmail.com>]

On Mon, Oct 31, 2011 at 4:47 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Friday 28 October 2011 17:03:03 Bjorn Helgaas wrote:
>> On Thu, Oct 27, 2011 at 7:49 PM, Thomas Renninger <trenn@suse.de> wrote:
>> > On Thursday 29 September 2011 23:59:08 Myron Stowe wrote:
>> > ..
>> >> Myron Stowe (2):
>> >>       ACPI: Convert acpi_pre_map_gar()/acpi_atomic_read() and remove ./drivers/acpi/atomicio.[ch]
>> >>       ACPI: Export interfaces for ioremapping/iounmapping ACPI registers
>> >
>> > Would be great to know whether these are going to be accepted.
>> > If yes, this check should get removed as well:
>> >
>> > drivers/acpi/acpica/hwregs.c:
>> > acpi_status
>> > acpi_hw_validate_register(struct acpi_generic_address *reg,
>> >                          u8 max_bit_width, u64 *address)
>> > {
>> > ...
>> >        if (reg->bit_offset != 0) {
>> >                ACPI_WARNING((AE_INFO,
>> >                              "Unsupported register bit offset: 0x%X",
>> >                              reg->bit_offset));
>> >        }
>> >
>> > because APEI GAR declarations do use bit_offset != 0.
>>
>> Half of this makes sense to me.  Myron's patch changes APEI from using
>> acpi_atomic_read() (which doesn't call acpi_hw_validate_register()) to
>> using acpi_read(), which *does* call it.  So after Myron's patch,
>> we'll see warnings we didn't see before.
>>
>> The part that doesn't make sense to me is just removing the warning.
>> That warning looks to me like it's saying "oops, here's something we
>> should support, but haven't implemented yet."  Wouldn't it be better
>> to implement support for bit_offset in acpi_read() at the same time we
>> remove the warning?  Then Myron could update his patch to drop the
>> bit_offset support in __apei_exec_read_register() when converting to
>> acpi_read().
>>
>> If APEI uses bit_offset != 0, it's at least possible that other areas
>> will use it in the future, and it'd be nicer to have all the support
>> in acpi_read() rather than forcing APEI and others to each implement
>> their own support for it.
> Googling for the warning:
> "Unsupported register bit offset"
> only points to code snippets.
> The code needs to be compatible with a long history of ACPI table
> implementations (the reason why I thought to keep bit offset handling
> in APEI code for now is the safer approach). But bit_offset != 0 seem
> to only appear in latest APEI table implementations.
> Looks like this condition was never run into and it should be safe
> to add bit offset support to these generic parts.
> -> I agree that bit offset handling can/should get added there.
>
> Still, if Windows has duplicated code for APEI GAR handling (with
> additional mask value, for example ignoring bit width) and does it
> slightly different than they do it in other parts,
> we also might not come around APEI specific GAR checking/workarounds.
>

I've hit a snag - hopefully I'm just not seeing something obvious so I thought
I would relay what I'm encountering and see if you or Bjorn have any ideas.

We all (Thomas, Bjorn, and myself) seem to all agree that GAS/GAR 'bit_offset'
handling should be handled in the generic ACPI code - acpi_read()/acpi_write()
as it is a member of GAS.  In order to do this we need to augment acpi_read()/
acpi_write() to handle 'bit_offset's properly and at the same time remove the
'bit_offset' warning from acpi_hw_validate_register().

By the same reasoning, APEI 'mask' handling should remain in the APEI code
as it is APEI specific.

Now, with that context, I have been attempting to convert APEI usages of
acpi_atomic code to generic ACPI code, specifically:
  acpi_pre_map_gar()   ->  acpi_os_map_generic_address()
  acpi_post_unmap_gar()  ->  acpi_os_unmap_generic_address()
  acpi_atomic_read()  ->   acpi_read()
  acpi_atomic_write()  ->  acpi_write()
which seems to work out fairly nicely, allowing for the removal of
drivers/acpi/atomicio.[ch] in its entirety as it is now fairly redundant.

The snag I've hit concerns these conversions with respect to
__apei_exec_write_register().  If __apei_exec_write_register() is called with a
APEI flag of APEI_EXEC_PRESERVE_REGISTER the code writes the
target register's bits of interest, specified by 'mask' and 'bit_offset', while
maintaining the current value of all the other bits of the register.

If we believe that 'bit_offset' should be handled by generic GAS/GAR related
code and 'mask' should be handled by APEI specific code, I do not see how
to maintain the behavior of __apei_exec_write_register()'s
APEI_EXEC_PRESERVE_REGISTER block.  With the 'bit_offset' handling
split out into ACPI generic code as with such, we would end up losing the
value of any bits below a 'bit_offset' value that need to be maintained.

If you work through this example of __apei_exec_write_register() and assume
'bit_offset' has been moved into ACPI generic code I believe you will see what
I am trying to point out.

Assume val = 0x0123456789abcdef, mask = 0xff, bit_offset = 16 (0x10),
valr = 0xfedcba987654321, and (flags & APEI_EXEC_PRESERVE_REGISTER)
is true the the current code works as follows:
  val &= mask     0x00000000000000ef
  val <<= bit_offset     0x0000000000ef0000
  now the APEI_EXEC_PRESERVE_REGISTER block
  valr &= ~(mask << bit_offset)     0xfedcba9876003210
  val |= valr     0xfedeba9876EF3210

I don't see how to maintain this behavior with a converted acpi_write() that
itself handles 'bit_offset' shifting.

The only solution I've thought of would be to maintain much of
__apei_exec_write_register()'s current behavior with the addition of a hacky
adjustment to zero out 'bit_offset' just prior to the write if the
flag test is true.

I believe there has to be a better solution but I'm just not seeing it
currently?


Thomas - this issue exists with either conversion approach (my approach of
getting acpi_read()/acpi_write() augmented to handle 'bit_offset' values and
using those or your idea of copying acpi_atomicio_read()/acpi_atomicio_write(),
changing their names to apei_read()/apei_write(), augmenting those to
handle 'bit_offset' values and using them).


Thanks,
 Myron

>   Thomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html