I see you posted a first version of this series a couple days
ago, but there weren't any responses (at least on linux-acpi),
and you didn't say anything about what you changed between
-v1 and -v2.
On Thursday 10 December 2009 12:16:53 am Huang Ying wrote:
> Some ACPI IO accessing need to be done in atomic context. For example,
> APEI ERST operations may be used for permanent storage in hardware
> error handler. That is, it may be called in atomic contexts such as
> IRQ or NMI, etc. And, ERST/EINJ implement their operations via IO
> memory/port accessing. But the IO memory accessing method provided by
> ACPI (acpi_read/acpi_write) maps the IO memory during it is accessed,
> so it can not be used in atomic context. To solve the issue, the IO
> memory should be pre-mapped during EINJ/ERST initializing. A linked
> list is used to record which memory area has been mapped, when memory
> is accessed in hardware error handler, search the linked list for the
> mapped virtual address from the given physical address.
The ACPI CA has functions called acpi_hw_read() and acpi_hw_write()
that have similar prototypes and functionality (but of course, they
don't work in atomic context). It'd be nice if your new functions
had similar names, e.g., acpi_hw_map(), acpi_hw_unmap(),
acpi_hw_read_atomic(), acpi_hw_write_atomic().
I think your code would be simpler if acpi_pre_map_gar() returned a
struct acpi_iomap pointer (from the caller's point of view, this would
be an opaque cookie). Then you could just supply that cookie to
acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
you could even get rid of the list and all the fancy RCU & kref stuff
then, too.
> +/* In NMI handler, should set silent = 1 */
> +static int acpi_check_gar(struct acpi_generic_address *reg,
> + u64 *paddr, int silent)
> +{
> + u32 width;
> +
> + /* Handle possible alignment issues */
> + memcpy(paddr, ®->address, sizeof(*paddr));
> + if (!*paddr) {
> + if (!silent)
> + pr_info(
> + "Invalid physical address in GAR, firmware bug?\n");
> + return -EINVAL;
> + }
> +
> + width = reg->bit_width;
> + if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> + if (!silent)
> + pr_info(
> + "Invalid bit width in GAR, firmware bug?\n");
> + return -EINVAL;
> + }
> +
> + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> + if (!silent)
> + pr_info(
> + "Invalid address space type in GAR, firmware bug?\n");
Error messages with constant text are nearly useless because they
don't give much of a clue about where to look for a problem.
Personally, for something this, I would just return failure and
never print anything. If a map fails, the caller should notice
and you then have a good idea of where to look.
> +static int acpi_atomic_read_port(u64 port, u32 *val, u32 width)
> +{
> + switch (width) {
> + case 8:
> + *val = inb(port);
> + break;
> + case 16:
> + *val = inw(port);
> + break;
> + case 32:
> + *val = inl(port);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Can you use acpi_os_read_port() and acpi_os_write_port() instead of
duplicating this code?
> +static int acpi_atomic_write_port(u64 port, u32 val, u32 width)
> +{
> + switch (width) {
> + case 8:
> + outb(val, port);
> + break;
> + case 16:
> + outw(val, port);
> + break;
> + case 32:
> + outl(val, port);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Bjorn
--
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
