On Mon, Oct 31, 2011 at 4:47 AM, Thomas Renninger <trenn@xxxxxxx> wrote:
> On Friday 28 October 2011 17:03:03 Bjorn Helgaas wrote:
>> On Thu, Oct 27, 2011 at 7:49 PM, Thomas Renninger <trenn@xxxxxxx> 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.
>
We all seem to agree that bit offset handling can/should get added to the
generic parts at some point.
As for APEI specifically and my patch to remove atomicio.[ch] I think we
should add code prior to the converted 'acpi_read'/'acpi_write' calls to copy
the GAS structure parameter onto the local stack and zero out the
gas.bit_offset value as in:
struct acpi_generic_address gas; /* on local stack */
gas = entry->register_region;
gas.bit_offset = 0;
acpi_read(val, &gas);
This should cover the three issues that Thomas pointed out: "unsupported
register bit offset" warnings, invalid bit_width entries in APEI GAR entries,
and GAR structures located in reserved BIOS areas that need to be
treated as const.
As for invalid bit_width entries in APEI GAR entries - atomicio.c currently
ignores bit_offset so the approach above of clearing out such should be
safe in this context.
The above would also _not_ introduce "unsupported register bit offset"
warnings that were not there before although it might be nice to wrap the
above GAS copying and gas.bit_offset clearing out into a wrapper routine
and add a 'warn_once' within such indicating that we are ignoring a non-
zero bit_offset. These warnings should be harmless in APEI context but
are admittedly alarming.
Lastly, the above addition also mitigates any modification of GAR structures
in the 'acpi_read'/'acpi_write' call paths.
Let me know what you all think and thanks Thomas for bringing these
issues to light.
Myron
> 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.
>
> Thomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
