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

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

[Myron Stowe]

Late last year I submitted a patch series that re-factored some existing
work that Huang Ying introduced adding support for accessing ACPI
generic registers backed by Memory Mapped I/O (MMIO) while within
interrupt context:
  Huang Ying's commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262,
  My series: http://marc.info/?l=linux-acpi&m=128769263327206&w=2.

While the impetus of the series was to resolve
https://bugzilla.kernel.org/show_bug.cgi?id=18012, an underlying goal
of the effort was to re-factor ./drivers/acpi/atomicio.c into
./drivers/acpi/osl.c, providing equivalent functinality but in a more
generalized manner, to allow usage in non-specific (i.e. APEI) contexts
and remove atomicio.c.

At that point in time, there was controversy as to the approach that
was being taken concerning APEI.  Due the controversy, all of the
series' re-factoring efforts were taken in except for patch 7/7 which
took the last step of converting over to the new ACPI generic register
capabilities and removing ./drivers/acpi/atomicio.[ch].  As a result,
./drivers/acpi/atomicio.c still exists leaving us with two
sets of code providing similar functionality.

Shortly after the new ACPI generic register capabilities were accepted
upstream Rafael Wysocki submitted a series of subsequent patches which
resolved a couple of bugs and provided a number of optimizations,
especially with respect locking, to the new capabilities.  As mentioned
in the original re-factoring series' preamble ([PATCH 0/7]), some of its
implementation was based upon Huang Ying's original work - specifically
patches 5/7 and 6/7 - and so at least some of the issues that Rafael
addressed in the new capabilities remain unresolved within atomicio.c [*].

This patch series addresses both of these issues - the duplication of
code and likely latent issues that remain within atomicio.c - by
reintroducing patch 7/7 from last year: "Re-factor and remove
./drivers/acpi/atomicio.[ch]".


[*]  Bug fixes and optimizations that Len and Rafael subsequently provided
to the new ACPI generic register capabilities:
  6d5bbf00..	ACPI: Use ioremap_cache()
  2d6d9fd3..	ACPI: Introduce acpi_os_ioremap()
  884b821f..	ACPI: Fix acpi_os_read_memory() and acpi_os_write_memory()
  7bbb8903..	ACPI: Change acpi_ioremap_lock into a mutex
  7fe135dc..	ACPI: Avoid walking the list of memory mappings in osl.c twice
  7ffd0443..	ACPI: Make acpi_os_map_memory() avoid unnecessary mappings
  b7c1fadd..	ACPI: Do not use krefs under a mutex in osl.c
---

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


 drivers/acpi/Makefile         |    1 
 drivers/acpi/apei/apei-base.c |   12 +
 drivers/acpi/apei/ghes.c      |   10 +
 drivers/acpi/atomicio.c       |  365 -----------------------------------------
 drivers/acpi/osl.c            |    6 -
 include/acpi/atomicio.h       |   10 -
 include/linux/acpi_io.h       |    3 
 7 files changed, 18 insertions(+), 389 deletions(-)
 delete mode 100644 drivers/acpi/atomicio.c
 delete mode 100644 include/acpi/atomicio.h

-- 

[PATCH 1/2] ACPI: Export interfaces for ioremapping/iounmapping ACPI registers

[Myron Stowe]

From: Myron Stowe <mstowe@redhat.com>

Export remapping and unmapping interfaces - acpi_os_map_generic_address()
and acpi_os_unmap_generic_address() - for ACPI generic registers
that are backed by memory mapped I/O (MMIO).

ACPI Generic Address Structure (GAS) reference (ACPI's fixed/generic
hardware registers use the GAS format):
  ACPI Specification, Revision 4.0, Section 5.2.3.1, "Generic Address
  Structure"

Signed_off_by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/acpi/osl.c      |    6 ++++--
 include/linux/acpi_io.h |    3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index fa32f58..46c5c18 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -426,7 +426,7 @@ void __init early_acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		__acpi_unmap_table(virt, size);
 }
 
-static int acpi_os_map_generic_address(struct acpi_generic_address *addr)
+int acpi_os_map_generic_address(struct acpi_generic_address *addr)
 {
 	void __iomem *virt;
 
@@ -442,8 +442,9 @@ static int acpi_os_map_generic_address(struct acpi_generic_address *addr)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(acpi_os_map_generic_address);
 
-static void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
+void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
 {
 	struct acpi_ioremap *map;
 
@@ -464,6 +465,7 @@ static void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
 
 	acpi_os_map_cleanup(map);
 }
+EXPORT_SYMBOL_GPL(acpi_os_unmap_generic_address);
 
 #ifdef ACPI_FUTURE_USAGE
 acpi_status
diff --git a/include/linux/acpi_io.h b/include/linux/acpi_io.h
index 4afd710..b0ffa21 100644
--- a/include/linux/acpi_io.h
+++ b/include/linux/acpi_io.h
@@ -12,4 +12,7 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 
 void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size);
 
+int acpi_os_map_generic_address(struct acpi_generic_address *addr);
+void acpi_os_unmap_generic_address(struct acpi_generic_address *addr);
+
 #endif

Re: [PATCH 1/2] ACPI: Export interfaces for ioremapping/iounmapping ACPI registers

[Rafael J. Wysocki]

On Thursday, September 29, 2011, Myron Stowe wrote:
> From: Myron Stowe <mstowe@redhat.com>
> 
> Export remapping and unmapping interfaces - acpi_os_map_generic_address()
> and acpi_os_unmap_generic_address() - for ACPI generic registers
> that are backed by memory mapped I/O (MMIO).
> 
> ACPI Generic Address Structure (GAS) reference (ACPI's fixed/generic
> hardware registers use the GAS format):
>   ACPI Specification, Revision 4.0, Section 5.2.3.1, "Generic Address
>   Structure"
> 
> Signed_off_by: Myron Stowe <myron.stowe@redhat.com>

Nothing wrong with that in principle.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
> 
>  drivers/acpi/osl.c      |    6 ++++--
>  include/linux/acpi_io.h |    3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index fa32f58..46c5c18 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -426,7 +426,7 @@ void __init early_acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
>  		__acpi_unmap_table(virt, size);
>  }
>  
> -static int acpi_os_map_generic_address(struct acpi_generic_address *addr)
> +int acpi_os_map_generic_address(struct acpi_generic_address *addr)
>  {
>  	void __iomem *virt;
>  
> @@ -442,8 +442,9 @@ static int acpi_os_map_generic_address(struct acpi_generic_address *addr)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(acpi_os_map_generic_address);
>  
> -static void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
> +void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
>  {
>  	struct acpi_ioremap *map;
>  
> @@ -464,6 +465,7 @@ static void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
>  
>  	acpi_os_map_cleanup(map);
>  }
> +EXPORT_SYMBOL_GPL(acpi_os_unmap_generic_address);
>  
>  #ifdef ACPI_FUTURE_USAGE
>  acpi_status
> diff --git a/include/linux/acpi_io.h b/include/linux/acpi_io.h
> index 4afd710..b0ffa21 100644
> --- a/include/linux/acpi_io.h
> +++ b/include/linux/acpi_io.h
> @@ -12,4 +12,7 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
>  
>  void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size);
>  
> +int acpi_os_map_generic_address(struct acpi_generic_address *addr);
> +void acpi_os_unmap_generic_address(struct acpi_generic_address *addr);
> +
>  #endif
> 
> 
> 

[PATCH 2/2] ACPI: Convert acpi_pre_map_gar()/acpi_atomic_read() and remove ./drivers/acpi/atomicio.[ch]

[Myron Stowe]

From: Myron Stowe <mstowe@redhat.com>

APEI needs memory access in interrupt context.  The obvious choice is
acpi_read(), but originally it couldn't be used in interrupt context
because it makes temporary mappings with ioremap().  Therefore, we added
drivers/acpi/atomicio.c, which provides:
	acpi_pre_map_gar()      <-- ioremap in process context
        acpi_atomic_read()      <-- memory access in interrupt context
        acpi_post_unmap_gar()   <-- iounmap

Later we added acpi_os_map_generic_address() (2971852) and enhanced
acpi_read() so it works in interrupt context as long as the address has
been previously mapped (620242a).  Now this sequence:
        acpi_os_map_generic_address()   <-- ioremap in process context
        acpi_read()                     <-- now OK in interrupt context
        acpi_os_unmap_generic_address()
is equivalent to what atomicio.c provides.


This patch converts APEI to use the existing calls within osl.c and the
CA, based on the re-factoring that was done in an earlier patch series -
http://marc.info/?l=linux-acpi&m=128769263327206&w=2
	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()
and removes atomicio.[ch].

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/acpi/Makefile         |    1 
 drivers/acpi/apei/apei-base.c |   12 +
 drivers/acpi/apei/ghes.c      |   10 +
 drivers/acpi/atomicio.c       |  365 -----------------------------------------
 include/acpi/atomicio.h       |   10 -
 5 files changed, 11 insertions(+), 387 deletions(-)
 delete mode 100644 drivers/acpi/atomicio.c
 delete mode 100644 include/acpi/atomicio.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index ecb26b4..e7c1d0f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -19,7 +19,6 @@ obj-y				+= acpi.o \
 
 # All the builtin files are in the "acpi." module_param namespace.
 acpi-y				+= osl.o utils.o reboot.o
-acpi-y				+= atomicio.o
 
 # sleep related files
 acpi-y				+= wakeup.o
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 6154036..2ea95ec 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -34,13 +34,13 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/acpi.h>
+#include <linux/acpi_io.h>
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/kref.h>
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
-#include <acpi/atomicio.h>
 
 #include "apei-internal.h"
 
@@ -70,7 +70,7 @@ int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val)
 {
 	int rc;
 
-	rc = acpi_atomic_read(val, &entry->register_region);
+	rc = acpi_read(val, &entry->register_region);
 	if (rc)
 		return rc;
 	*val >>= entry->register_region.bit_offset;
@@ -116,13 +116,13 @@ int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val)
 	val <<= entry->register_region.bit_offset;
 	if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
 		u64 valr = 0;
-		rc = acpi_atomic_read(&valr, &entry->register_region);
+		rc = acpi_read(&valr, &entry->register_region);
 		if (rc)
 			return rc;
 		valr &= ~(entry->mask << entry->register_region.bit_offset);
 		val |= valr;
 	}
-	rc = acpi_atomic_write(val, &entry->register_region);
+	rc = acpi_write(val, &entry->register_region);
 
 	return rc;
 }
@@ -243,7 +243,7 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		return acpi_pre_map_gar(&entry->register_region);
+		return acpi_os_map_generic_address(&entry->register_region);
 
 	return 0;
 }
@@ -276,7 +276,7 @@ static int post_unmap_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		acpi_post_unmap_gar(&entry->register_region);
+		acpi_os_unmap_generic_address(&entry->register_region);
 
 	return 0;
 }
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0784f99..9814e05 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -33,6 +33,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/acpi.h>
+#include <linux/acpi_io.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/timer.h>
@@ -46,7 +47,6 @@
 #include <linux/llist.h>
 #include <linux/genalloc.h>
 #include <acpi/apei.h>
-#include <acpi/atomicio.h>
 #include <acpi/hed.h>
 #include <asm/mce.h>
 #include <asm/tlbflush.h>
@@ -298,7 +298,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	if (!ghes)
 		return ERR_PTR(-ENOMEM);
 	ghes->generic = generic;
-	rc = acpi_pre_map_gar(&generic->error_status_address);
+	rc = acpi_os_map_generic_address(&generic->error_status_address);
 	if (rc)
 		goto err_free;
 	error_block_length = generic->error_block_length;
@@ -318,7 +318,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	return ghes;
 
 err_unmap:
-	acpi_post_unmap_gar(&generic->error_status_address);
+	acpi_os_unmap_generic_address(&generic->error_status_address);
 err_free:
 	kfree(ghes);
 	return ERR_PTR(rc);
@@ -327,7 +327,7 @@ err_free:
 static void ghes_fini(struct ghes *ghes)
 {
 	kfree(ghes->estatus);
-	acpi_post_unmap_gar(&ghes->generic->error_status_address);
+	acpi_os_unmap_generic_address(&ghes->generic->error_status_address);
 }
 
 enum {
@@ -398,7 +398,7 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
 	u32 len;
 	int rc;
 
-	rc = acpi_atomic_read(&buf_paddr, &g->error_status_address);
+	rc = acpi_read(&buf_paddr, &g->error_status_address);
 	if (rc) {
 		if (!silent && printk_ratelimit())
 			pr_warning(FW_WARN GHES_PFX
diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
deleted file mode 100644
index 7489b89..0000000
--- a/drivers/acpi/atomicio.c
+++ /dev/null
@@ -1,365 +0,0 @@
-/*
- * atomicio.c - ACPI IO memory pre-mapping/post-unmapping, then
- * accessing in atomic context.
- *
- * This is used for NMI handler to access IO memory area, because
- * ioremap/iounmap can not be used in NMI handler. The IO memory area
- * is pre-mapped in process context and accessed in NMI handler.
- *
- * Copyright (C) 2009-2010, Intel Corp.
- *	Author: Huang Ying <ying.huang@intel.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version
- * 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/acpi.h>
-#include <linux/io.h>
-#include <linux/kref.h>
-#include <linux/rculist.h>
-#include <linux/interrupt.h>
-#include <linux/slab.h>
-#include <acpi/atomicio.h>
-
-#define ACPI_PFX "ACPI: "
-
-static LIST_HEAD(acpi_iomaps);
-/*
- * Used for mutual exclusion between writers of acpi_iomaps list, for
- * synchronization between readers and writer, RCU is used.
- */
-static DEFINE_SPINLOCK(acpi_iomaps_lock);
-
-struct acpi_iomap {
-	struct list_head list;
-	void __iomem *vaddr;
-	unsigned long size;
-	phys_addr_t paddr;
-	struct kref ref;
-};
-
-/* acpi_iomaps_lock or RCU read lock must be held before calling */
-static struct acpi_iomap *__acpi_find_iomap(phys_addr_t paddr,
-					    unsigned long size)
-{
-	struct acpi_iomap *map;
-
-	list_for_each_entry_rcu(map, &acpi_iomaps, list) {
-		if (map->paddr + map->size >= paddr + size &&
-		    map->paddr <= paddr)
-			return map;
-	}
-	return NULL;
-}
-
-/*
- * Atomic "ioremap" used by NMI handler, if the specified IO memory
- * area is not pre-mapped, NULL will be returned.
- *
- * acpi_iomaps_lock or RCU read lock must be held before calling
- */
-static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
-					 unsigned long size)
-{
-	struct acpi_iomap *map;
-
-	map = __acpi_find_iomap(paddr, size);
-	if (map)
-		return map->vaddr + (paddr - map->paddr);
-	else
-		return NULL;
-}
-
-/* acpi_iomaps_lock must be held before calling */
-static void __iomem *__acpi_try_ioremap(phys_addr_t paddr,
-					unsigned long size)
-{
-	struct acpi_iomap *map;
-
-	map = __acpi_find_iomap(paddr, size);
-	if (map) {
-		kref_get(&map->ref);
-		return map->vaddr + (paddr - map->paddr);
-	} else
-		return NULL;
-}
-
-/*
- * Used to pre-map the specified IO memory area. First try to find
- * whether the area is already pre-mapped, if it is, increase the
- * reference count (in __acpi_try_ioremap) and return; otherwise, do
- * the real ioremap, and add the mapping into acpi_iomaps list.
- */
-static void __iomem *acpi_pre_map(phys_addr_t paddr,
-				  unsigned long size)
-{
-	void __iomem *vaddr;
-	struct acpi_iomap *map;
-	unsigned long pg_sz, flags;
-	phys_addr_t pg_off;
-
-	spin_lock_irqsave(&acpi_iomaps_lock, flags);
-	vaddr = __acpi_try_ioremap(paddr, size);
-	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
-	if (vaddr)
-		return vaddr;
-
-	pg_off = paddr & PAGE_MASK;
-	pg_sz = ((paddr + size + PAGE_SIZE - 1) & PAGE_MASK) - pg_off;
-	vaddr = ioremap(pg_off, pg_sz);
-	if (!vaddr)
-		return NULL;
-	map = kmalloc(sizeof(*map), GFP_KERNEL);
-	if (!map)
-		goto err_unmap;
-	INIT_LIST_HEAD(&map->list);
-	map->paddr = pg_off;
-	map->size = pg_sz;
-	map->vaddr = vaddr;
-	kref_init(&map->ref);
-
-	spin_lock_irqsave(&acpi_iomaps_lock, flags);
-	vaddr = __acpi_try_ioremap(paddr, size);
-	if (vaddr) {
-		spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
-		iounmap(map->vaddr);
-		kfree(map);
-		return vaddr;
-	}
-	list_add_tail_rcu(&map->list, &acpi_iomaps);
-	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
-
-	return map->vaddr + (paddr - map->paddr);
-err_unmap:
-	iounmap(vaddr);
-	return NULL;
-}
-
-/* acpi_iomaps_lock must be held before calling */
-static void __acpi_kref_del_iomap(struct kref *ref)
-{
-	struct acpi_iomap *map;
-
-	map = container_of(ref, struct acpi_iomap, ref);
-	list_del_rcu(&map->list);
-}
-
-/*
- * Used to post-unmap the specified IO memory area. The iounmap is
- * done only if the reference count goes zero.
- */
-static void acpi_post_unmap(phys_addr_t paddr, unsigned long size)
-{
-	struct acpi_iomap *map;
-	unsigned long flags;
-	int del;
-
-	spin_lock_irqsave(&acpi_iomaps_lock, flags);
-	map = __acpi_find_iomap(paddr, size);
-	BUG_ON(!map);
-	del = kref_put(&map->ref, __acpi_kref_del_iomap);
-	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
-
-	if (!del)
-		return;
-
-	synchronize_rcu();
-	iounmap(map->vaddr);
-	kfree(map);
-}
-
-/* In NMI handler, should set silent = 1 */
-static int acpi_check_gar(struct acpi_generic_address *reg,
-			  u64 *paddr, int silent)
-{
-	u32 width, space_id;
-
-	width = reg->bit_width;
-	space_id = reg->space_id;
-	/* Handle possible alignment issues */
-	memcpy(paddr, &reg->address, sizeof(*paddr));
-	if (!*paddr) {
-		if (!silent)
-			pr_warning(FW_BUG ACPI_PFX
-			"Invalid physical address in GAR [0x%llx/%u/%u]\n",
-				   *paddr, width, space_id);
-		return -EINVAL;
-	}
-
-	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
-		if (!silent)
-			pr_warning(FW_BUG ACPI_PFX
-				   "Invalid bit width in GAR [0x%llx/%u/%u]\n",
-				   *paddr, width, space_id);
-		return -EINVAL;
-	}
-
-	if (space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
-	    space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
-		if (!silent)
-			pr_warning(FW_BUG ACPI_PFX
-			"Invalid address space type in GAR [0x%llx/%u/%u]\n",
-				   *paddr, width, space_id);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/* Pre-map, working on GAR */
-int acpi_pre_map_gar(struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	void __iomem *vaddr;
-	int rc;
-
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		return 0;
-
-	rc = acpi_check_gar(reg, &paddr, 0);
-	if (rc)
-		return rc;
-
-	vaddr = acpi_pre_map(paddr, reg->bit_width / 8);
-	if (!vaddr)
-		return -EIO;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(acpi_pre_map_gar);
-
-/* Post-unmap, working on GAR */
-int acpi_post_unmap_gar(struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	int rc;
-
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		return 0;
-
-	rc = acpi_check_gar(reg, &paddr, 0);
-	if (rc)
-		return rc;
-
-	acpi_post_unmap(paddr, reg->bit_width / 8);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(acpi_post_unmap_gar);
-
-/*
- * Can be used in atomic (including NMI) or process context. RCU read
- * lock can only be released after the IO memory area accessing.
- */
-static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width)
-{
-	void __iomem *addr;
-
-	rcu_read_lock();
-	addr = __acpi_ioremap_fast(paddr, width);
-	switch (width) {
-	case 8:
-		*val = readb(addr);
-		break;
-	case 16:
-		*val = readw(addr);
-		break;
-	case 32:
-		*val = readl(addr);
-		break;
-#ifdef readq
-	case 64:
-		*val = readq(addr);
-		break;
-#endif
-	default:
-		return -EINVAL;
-	}
-	rcu_read_unlock();
-
-	return 0;
-}
-
-static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)
-{
-	void __iomem *addr;
-
-	rcu_read_lock();
-	addr = __acpi_ioremap_fast(paddr, width);
-	switch (width) {
-	case 8:
-		writeb(val, addr);
-		break;
-	case 16:
-		writew(val, addr);
-		break;
-	case 32:
-		writel(val, addr);
-		break;
-#ifdef writeq
-	case 64:
-		writeq(val, addr);
-		break;
-#endif
-	default:
-		return -EINVAL;
-	}
-	rcu_read_unlock();
-
-	return 0;
-}
-
-/* GAR accessing in atomic (including NMI) or process context */
-int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	int rc;
-
-	rc = acpi_check_gar(reg, &paddr, 1);
-	if (rc)
-		return rc;
-
-	*val = 0;
-	switch (reg->space_id) {
-	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
-		return acpi_atomic_read_mem(paddr, val, reg->bit_width);
-	case ACPI_ADR_SPACE_SYSTEM_IO:
-		return acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
-	default:
-		return -EINVAL;
-	}
-}
-EXPORT_SYMBOL_GPL(acpi_atomic_read);
-
-int acpi_atomic_write(u64 val, struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	int rc;
-
-	rc = acpi_check_gar(reg, &paddr, 1);
-	if (rc)
-		return rc;
-
-	switch (reg->space_id) {
-	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
-		return acpi_atomic_write_mem(paddr, val, reg->bit_width);
-	case ACPI_ADR_SPACE_SYSTEM_IO:
-		return acpi_os_write_port(paddr, val, reg->bit_width);
-	default:
-		return -EINVAL;
-	}
-}
-EXPORT_SYMBOL_GPL(acpi_atomic_write);
diff --git a/include/acpi/atomicio.h b/include/acpi/atomicio.h
deleted file mode 100644
index 8b9fb4b..0000000
--- a/include/acpi/atomicio.h
+++ /dev/null
@@ -1,10 +0,0 @@
-#ifndef ACPI_ATOMIC_IO_H
-#define ACPI_ATOMIC_IO_H
-
-int acpi_pre_map_gar(struct acpi_generic_address *reg);
-int acpi_post_unmap_gar(struct acpi_generic_address *reg);
-
-int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg);
-int acpi_atomic_write(u64 val, struct acpi_generic_address *reg);
-
-#endif

Re: [PATCH 2/2] ACPI: Convert acpi_pre_map_gar()/acpi_atomic_read() and remove ./drivers/acpi/atomicio.[ch]

[Rafael J. Wysocki]

On Thursday, September 29, 2011, Myron Stowe wrote:
> From: Myron Stowe <mstowe@redhat.com>
> 
> APEI needs memory access in interrupt context.  The obvious choice is
> acpi_read(), but originally it couldn't be used in interrupt context
> because it makes temporary mappings with ioremap().  Therefore, we added
> drivers/acpi/atomicio.c, which provides:
> 	acpi_pre_map_gar()      <-- ioremap in process context
>         acpi_atomic_read()      <-- memory access in interrupt context
>         acpi_post_unmap_gar()   <-- iounmap
> 
> Later we added acpi_os_map_generic_address() (2971852) and enhanced
> acpi_read() so it works in interrupt context as long as the address has
> been previously mapped (620242a).  Now this sequence:
>         acpi_os_map_generic_address()   <-- ioremap in process context
>         acpi_read()                     <-- now OK in interrupt context
>         acpi_os_unmap_generic_address()
> is equivalent to what atomicio.c provides.
> 
> 
> This patch converts APEI to use the existing calls within osl.c and the
> CA, based on the re-factoring that was done in an earlier patch series -
> http://marc.info/?l=linux-acpi&m=128769263327206&w=2
> 	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()
> and removes atomicio.[ch].
> 
> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>

Makes sense to me.

Reviewed-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
> 
>  drivers/acpi/Makefile         |    1 
>  drivers/acpi/apei/apei-base.c |   12 +
>  drivers/acpi/apei/ghes.c      |   10 +
>  drivers/acpi/atomicio.c       |  365 -----------------------------------------
>  include/acpi/atomicio.h       |   10 -
>  5 files changed, 11 insertions(+), 387 deletions(-)
>  delete mode 100644 drivers/acpi/atomicio.c
>  delete mode 100644 include/acpi/atomicio.h
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index ecb26b4..e7c1d0f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -19,7 +19,6 @@ obj-y				+= acpi.o \
>  
>  # All the builtin files are in the "acpi." module_param namespace.
>  acpi-y				+= osl.o utils.o reboot.o
> -acpi-y				+= atomicio.o
>  
>  # sleep related files
>  acpi-y				+= wakeup.o
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index 6154036..2ea95ec 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -34,13 +34,13 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/acpi.h>
> +#include <linux/acpi_io.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/kref.h>
>  #include <linux/rculist.h>
>  #include <linux/interrupt.h>
>  #include <linux/debugfs.h>
> -#include <acpi/atomicio.h>
>  
>  #include "apei-internal.h"
>  
> @@ -70,7 +70,7 @@ int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val)
>  {
>  	int rc;
>  
> -	rc = acpi_atomic_read(val, &entry->register_region);
> +	rc = acpi_read(val, &entry->register_region);
>  	if (rc)
>  		return rc;
>  	*val >>= entry->register_region.bit_offset;
> @@ -116,13 +116,13 @@ int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val)
>  	val <<= entry->register_region.bit_offset;
>  	if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
>  		u64 valr = 0;
> -		rc = acpi_atomic_read(&valr, &entry->register_region);
> +		rc = acpi_read(&valr, &entry->register_region);
>  		if (rc)
>  			return rc;
>  		valr &= ~(entry->mask << entry->register_region.bit_offset);
>  		val |= valr;
>  	}
> -	rc = acpi_atomic_write(val, &entry->register_region);
> +	rc = acpi_write(val, &entry->register_region);
>  
>  	return rc;
>  }
> @@ -243,7 +243,7 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
>  	u8 ins = entry->instruction;
>  
>  	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
> -		return acpi_pre_map_gar(&entry->register_region);
> +		return acpi_os_map_generic_address(&entry->register_region);
>  
>  	return 0;
>  }
> @@ -276,7 +276,7 @@ static int post_unmap_gar_callback(struct apei_exec_context *ctx,
>  	u8 ins = entry->instruction;
>  
>  	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
> -		acpi_post_unmap_gar(&entry->register_region);
> +		acpi_os_unmap_generic_address(&entry->register_region);
>  
>  	return 0;
>  }
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 0784f99..9814e05 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -33,6 +33,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/acpi.h>
> +#include <linux/acpi_io.h>
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/timer.h>
> @@ -46,7 +47,6 @@
>  #include <linux/llist.h>
>  #include <linux/genalloc.h>
>  #include <acpi/apei.h>
> -#include <acpi/atomicio.h>
>  #include <acpi/hed.h>
>  #include <asm/mce.h>
>  #include <asm/tlbflush.h>
> @@ -298,7 +298,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  	if (!ghes)
>  		return ERR_PTR(-ENOMEM);
>  	ghes->generic = generic;
> -	rc = acpi_pre_map_gar(&generic->error_status_address);
> +	rc = acpi_os_map_generic_address(&generic->error_status_address);
>  	if (rc)
>  		goto err_free;
>  	error_block_length = generic->error_block_length;
> @@ -318,7 +318,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  	return ghes;
>  
>  err_unmap:
> -	acpi_post_unmap_gar(&generic->error_status_address);
> +	acpi_os_unmap_generic_address(&generic->error_status_address);
>  err_free:
>  	kfree(ghes);
>  	return ERR_PTR(rc);
> @@ -327,7 +327,7 @@ err_free:
>  static void ghes_fini(struct ghes *ghes)
>  {
>  	kfree(ghes->estatus);
> -	acpi_post_unmap_gar(&ghes->generic->error_status_address);
> +	acpi_os_unmap_generic_address(&ghes->generic->error_status_address);
>  }
>  
>  enum {
> @@ -398,7 +398,7 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
>  	u32 len;
>  	int rc;
>  
> -	rc = acpi_atomic_read(&buf_paddr, &g->error_status_address);
> +	rc = acpi_read(&buf_paddr, &g->error_status_address);
>  	if (rc) {
>  		if (!silent && printk_ratelimit())
>  			pr_warning(FW_WARN GHES_PFX
> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
> deleted file mode 100644
> index 7489b89..0000000
> --- a/drivers/acpi/atomicio.c
> +++ /dev/null
> @@ -1,365 +0,0 @@
> -/*
> - * atomicio.c - ACPI IO memory pre-mapping/post-unmapping, then
> - * accessing in atomic context.
> - *
> - * This is used for NMI handler to access IO memory area, because
> - * ioremap/iounmap can not be used in NMI handler. The IO memory area
> - * is pre-mapped in process context and accessed in NMI handler.
> - *
> - * Copyright (C) 2009-2010, Intel Corp.
> - *	Author: Huang Ying <ying.huang@intel.com>
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License version
> - * 2 as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
> -#include <linux/acpi.h>
> -#include <linux/io.h>
> -#include <linux/kref.h>
> -#include <linux/rculist.h>
> -#include <linux/interrupt.h>
> -#include <linux/slab.h>
> -#include <acpi/atomicio.h>
> -
> -#define ACPI_PFX "ACPI: "
> -
> -static LIST_HEAD(acpi_iomaps);
> -/*
> - * Used for mutual exclusion between writers of acpi_iomaps list, for
> - * synchronization between readers and writer, RCU is used.
> - */
> -static DEFINE_SPINLOCK(acpi_iomaps_lock);
> -
> -struct acpi_iomap {
> -	struct list_head list;
> -	void __iomem *vaddr;
> -	unsigned long size;
> -	phys_addr_t paddr;
> -	struct kref ref;
> -};
> -
> -/* acpi_iomaps_lock or RCU read lock must be held before calling */
> -static struct acpi_iomap *__acpi_find_iomap(phys_addr_t paddr,
> -					    unsigned long size)
> -{
> -	struct acpi_iomap *map;
> -
> -	list_for_each_entry_rcu(map, &acpi_iomaps, list) {
> -		if (map->paddr + map->size >= paddr + size &&
> -		    map->paddr <= paddr)
> -			return map;
> -	}
> -	return NULL;
> -}
> -
> -/*
> - * Atomic "ioremap" used by NMI handler, if the specified IO memory
> - * area is not pre-mapped, NULL will be returned.
> - *
> - * acpi_iomaps_lock or RCU read lock must be held before calling
> - */
> -static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
> -					 unsigned long size)
> -{
> -	struct acpi_iomap *map;
> -
> -	map = __acpi_find_iomap(paddr, size);
> -	if (map)
> -		return map->vaddr + (paddr - map->paddr);
> -	else
> -		return NULL;
> -}
> -
> -/* acpi_iomaps_lock must be held before calling */
> -static void __iomem *__acpi_try_ioremap(phys_addr_t paddr,
> -					unsigned long size)
> -{
> -	struct acpi_iomap *map;
> -
> -	map = __acpi_find_iomap(paddr, size);
> -	if (map) {
> -		kref_get(&map->ref);
> -		return map->vaddr + (paddr - map->paddr);
> -	} else
> -		return NULL;
> -}
> -
> -/*
> - * Used to pre-map the specified IO memory area. First try to find
> - * whether the area is already pre-mapped, if it is, increase the
> - * reference count (in __acpi_try_ioremap) and return; otherwise, do
> - * the real ioremap, and add the mapping into acpi_iomaps list.
> - */
> -static void __iomem *acpi_pre_map(phys_addr_t paddr,
> -				  unsigned long size)
> -{
> -	void __iomem *vaddr;
> -	struct acpi_iomap *map;
> -	unsigned long pg_sz, flags;
> -	phys_addr_t pg_off;
> -
> -	spin_lock_irqsave(&acpi_iomaps_lock, flags);
> -	vaddr = __acpi_try_ioremap(paddr, size);
> -	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
> -	if (vaddr)
> -		return vaddr;
> -
> -	pg_off = paddr & PAGE_MASK;
> -	pg_sz = ((paddr + size + PAGE_SIZE - 1) & PAGE_MASK) - pg_off;
> -	vaddr = ioremap(pg_off, pg_sz);
> -	if (!vaddr)
> -		return NULL;
> -	map = kmalloc(sizeof(*map), GFP_KERNEL);
> -	if (!map)
> -		goto err_unmap;
> -	INIT_LIST_HEAD(&map->list);
> -	map->paddr = pg_off;
> -	map->size = pg_sz;
> -	map->vaddr = vaddr;
> -	kref_init(&map->ref);
> -
> -	spin_lock_irqsave(&acpi_iomaps_lock, flags);
> -	vaddr = __acpi_try_ioremap(paddr, size);
> -	if (vaddr) {
> -		spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
> -		iounmap(map->vaddr);
> -		kfree(map);
> -		return vaddr;
> -	}
> -	list_add_tail_rcu(&map->list, &acpi_iomaps);
> -	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
> -
> -	return map->vaddr + (paddr - map->paddr);
> -err_unmap:
> -	iounmap(vaddr);
> -	return NULL;
> -}
> -
> -/* acpi_iomaps_lock must be held before calling */
> -static void __acpi_kref_del_iomap(struct kref *ref)
> -{
> -	struct acpi_iomap *map;
> -
> -	map = container_of(ref, struct acpi_iomap, ref);
> -	list_del_rcu(&map->list);
> -}
> -
> -/*
> - * Used to post-unmap the specified IO memory area. The iounmap is
> - * done only if the reference count goes zero.
> - */
> -static void acpi_post_unmap(phys_addr_t paddr, unsigned long size)
> -{
> -	struct acpi_iomap *map;
> -	unsigned long flags;
> -	int del;
> -
> -	spin_lock_irqsave(&acpi_iomaps_lock, flags);
> -	map = __acpi_find_iomap(paddr, size);
> -	BUG_ON(!map);
> -	del = kref_put(&map->ref, __acpi_kref_del_iomap);
> -	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
> -
> -	if (!del)
> -		return;
> -
> -	synchronize_rcu();
> -	iounmap(map->vaddr);
> -	kfree(map);
> -}
> -
> -/* In NMI handler, should set silent = 1 */
> -static int acpi_check_gar(struct acpi_generic_address *reg,
> -			  u64 *paddr, int silent)
> -{
> -	u32 width, space_id;
> -
> -	width = reg->bit_width;
> -	space_id = reg->space_id;
> -	/* Handle possible alignment issues */
> -	memcpy(paddr, &reg->address, sizeof(*paddr));
> -	if (!*paddr) {
> -		if (!silent)
> -			pr_warning(FW_BUG ACPI_PFX
> -			"Invalid physical address in GAR [0x%llx/%u/%u]\n",
> -				   *paddr, width, space_id);
> -		return -EINVAL;
> -	}
> -
> -	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> -		if (!silent)
> -			pr_warning(FW_BUG ACPI_PFX
> -				   "Invalid bit width in GAR [0x%llx/%u/%u]\n",
> -				   *paddr, width, space_id);
> -		return -EINVAL;
> -	}
> -
> -	if (space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> -	    space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> -		if (!silent)
> -			pr_warning(FW_BUG ACPI_PFX
> -			"Invalid address space type in GAR [0x%llx/%u/%u]\n",
> -				   *paddr, width, space_id);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -/* Pre-map, working on GAR */
> -int acpi_pre_map_gar(struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	void __iomem *vaddr;
> -	int rc;
> -
> -	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -		return 0;
> -
> -	rc = acpi_check_gar(reg, &paddr, 0);
> -	if (rc)
> -		return rc;
> -
> -	vaddr = acpi_pre_map(paddr, reg->bit_width / 8);
> -	if (!vaddr)
> -		return -EIO;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(acpi_pre_map_gar);
> -
> -/* Post-unmap, working on GAR */
> -int acpi_post_unmap_gar(struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	int rc;
> -
> -	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -		return 0;
> -
> -	rc = acpi_check_gar(reg, &paddr, 0);
> -	if (rc)
> -		return rc;
> -
> -	acpi_post_unmap(paddr, reg->bit_width / 8);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(acpi_post_unmap_gar);
> -
> -/*
> - * Can be used in atomic (including NMI) or process context. RCU read
> - * lock can only be released after the IO memory area accessing.
> - */
> -static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width)
> -{
> -	void __iomem *addr;
> -
> -	rcu_read_lock();
> -	addr = __acpi_ioremap_fast(paddr, width);
> -	switch (width) {
> -	case 8:
> -		*val = readb(addr);
> -		break;
> -	case 16:
> -		*val = readw(addr);
> -		break;
> -	case 32:
> -		*val = readl(addr);
> -		break;
> -#ifdef readq
> -	case 64:
> -		*val = readq(addr);
> -		break;
> -#endif
> -	default:
> -		return -EINVAL;
> -	}
> -	rcu_read_unlock();
> -
> -	return 0;
> -}
> -
> -static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)
> -{
> -	void __iomem *addr;
> -
> -	rcu_read_lock();
> -	addr = __acpi_ioremap_fast(paddr, width);
> -	switch (width) {
> -	case 8:
> -		writeb(val, addr);
> -		break;
> -	case 16:
> -		writew(val, addr);
> -		break;
> -	case 32:
> -		writel(val, addr);
> -		break;
> -#ifdef writeq
> -	case 64:
> -		writeq(val, addr);
> -		break;
> -#endif
> -	default:
> -		return -EINVAL;
> -	}
> -	rcu_read_unlock();
> -
> -	return 0;
> -}
> -
> -/* GAR accessing in atomic (including NMI) or process context */
> -int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	int rc;
> -
> -	rc = acpi_check_gar(reg, &paddr, 1);
> -	if (rc)
> -		return rc;
> -
> -	*val = 0;
> -	switch (reg->space_id) {
> -	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> -		return acpi_atomic_read_mem(paddr, val, reg->bit_width);
> -	case ACPI_ADR_SPACE_SYSTEM_IO:
> -		return acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(acpi_atomic_read);
> -
> -int acpi_atomic_write(u64 val, struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	int rc;
> -
> -	rc = acpi_check_gar(reg, &paddr, 1);
> -	if (rc)
> -		return rc;
> -
> -	switch (reg->space_id) {
> -	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> -		return acpi_atomic_write_mem(paddr, val, reg->bit_width);
> -	case ACPI_ADR_SPACE_SYSTEM_IO:
> -		return acpi_os_write_port(paddr, val, reg->bit_width);
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(acpi_atomic_write);
> diff --git a/include/acpi/atomicio.h b/include/acpi/atomicio.h
> deleted file mode 100644
> index 8b9fb4b..0000000
> --- a/include/acpi/atomicio.h
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -#ifndef ACPI_ATOMIC_IO_H
> -#define ACPI_ATOMIC_IO_H
> -
> -int acpi_pre_map_gar(struct acpi_generic_address *reg);
> -int acpi_post_unmap_gar(struct acpi_generic_address *reg);
> -
> -int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg);
> -int acpi_atomic_write(u64 val, struct acpi_generic_address *reg);
> -
> -#endif
> 
> 
> 

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

[Thomas Renninger]

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.

There is another problem. Would be great to get some opinions/feedback
on it already:
APEI GAR entries seem to have invalid bit_width entries on some platforms.
After looking at several tables, I expect that with APEI tables access width
(in bytes) should get used instead, Windows seem to ignore bit width fields,
at least for APEI tables.

I have patches but I need to know whether your patches are accepted.

There also is a problem with modifying GAR bit_width field to be able to
pass it to the generic acpica functions (what your patches are doing).
The problem is that the structures are located in reserved BIOS areas and
they should be const and not get modified.

I have 2 solutions for this:
  1) Make apei_check_gar() pass 2 struct acpi_generic_address:
     int apei_check_gar(struct acpi_generic_address *reg,
                    const struct acpi_generic_address *orig, u64 *paddr)
     orig -> is the one located in reserved mem area, mapped to virtual addr space
     reg  -> will be a copy of it, possibly with bit_width adjusted which then
            can be passed to acpi_memory_read/write and friends.
  2) Allocate memory and copy whole APEI tables

1. Should have the advantage of less memory waste
2. Should have the advantage of a bit nicer code (kalloc and memcpy per table) and
   if more things like this happen, tables could get adjusted easily.
   It also has the advantage that GAR structures do not need to get double
   checked and eventually adjusted at runtime again.

Below is the first patch which goes for 1. More patches may be needed, but I
as said, I need to know whether your patches get accepted.
What do you think?

Thanks,

    Thomas

----
ACPI APEI: Adjust GAR checking code to what exists out there

Comparing different Generic Adress Register definitions of
different vendors it came out that bit width (at least in APEI
tables) is sometimes wrong or used different compared to older
ACPI BIOS definitions (e.g. older FACP tables).
It looks like Windows ignores the bit width field in
latest implementations. Either in APEI table parts only
(I'd say more likely) or in other ACPI parts as well.

Worst case is that an address value to be read from a GAR structure
can have a 8 bit width definition resulting in:
ERST: Can not request iomem region <0x              3f-0x              3f>
while the access width is correct:
[1B0h 0432  1]                       Action : 0D (Get Error Address Range)
[1B4h 0436 12]              Register Region : <Generic Address Structure>
[1B4h 0436  1]                     Space ID : 00 (SystemMemory)
[1B5h 0437  1]                    Bit Width : 08
[1B6h 0438  1]                   Bit Offset : 00
[1B7h 0439  1]         Encoded Access Width : 04 (QWord Access:64)
[1B8h 0440  8]                      Address : 000000007F8A8032


-> Ignore bit width field in APEI GAR checking code.
Correct bit width value if needed (derived from access width)
in the GAR structure, so that generic acpi read/write functions
can still be used.

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/apei/apei-base.c     |   63 +++++++++++++++++++++++++++++-------
 drivers/acpi/apei/apei-internal.h |    2 +
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 6154036..d05f084 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -520,25 +520,53 @@ void apei_resources_release(struct apei_resources *resources)
 }
 EXPORT_SYMBOL_GPL(apei_resources_release);
 
-static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
+int apei_check_gar(struct acpi_generic_address *reg,
+		   const struct acpi_generic_address *orig, u64 *paddr)
 {
-	u32 width, space_id;
+	int bit_width, space_id, acc_width, acc_width_bits;
 
-	width = reg->bit_width;
+	memcpy(reg, orig, sizeof(struct acpi_generic_address));
+	bit_width = reg->bit_width;
 	space_id = reg->space_id;
+	acc_width = reg->access_width;
+
 	/* Handle possible alignment issues */
 	memcpy(paddr, &reg->address, sizeof(*paddr));
 	if (!*paddr) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid physical address in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+			   *paddr, bit_width, space_id);
 		return -EINVAL;
 	}
 
-	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
+	switch (acc_width) {
+	case 1:
+		acc_width_bits = 8;
+		break;
+	case 2:
+		acc_width_bits = 16;
+		break;
+	case 3:
+		acc_width_bits = 32;
+		break;
+	case 4:
+		acc_width_bits = 64;
+		break;
+	case 0:
+		/* Never seen this, acc_width should always be correct */
+		if ((bit_width == 8) || (bit_width == 16) ||
+		    (bit_width == 32) || (bit_width == 64)) {
+			pr_warning(FW_BUG APEI_PFX
+				   "Incorrect acc width, using bit width"
+				   "[%d]\n", bit_width);
+			acc_width = bit_width / 8;
+			acc_width_bits = bit_width;
+			break;
+		}
+	default:
 		pr_warning(FW_BUG APEI_PFX
-			   "Invalid bit width in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+			   "Invalid access width in GAR [0x%llx/%u/%u]\n",
+			   *paddr, bit_width, space_id);
 		return -EINVAL;
 	}
 
@@ -546,19 +574,28 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
 	    space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid address space type in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+			   *paddr, bit_width, space_id);
 		return -EINVAL;
 	}
 
+	/* bit width is not much worth in APEI GAR structures */
+	if (acc_width_bits != bit_width) {
+		pr_debug(FW_INFO APEI_PFX
+			 "Correcting bit width value 0x%x -> 0x%x\n",
+			 reg->bit_width, acc_width_bits);
+		reg->bit_width = acc_width_bits;
+	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(apei_check_gar);
 
 static int collect_res_callback(struct apei_exec_context *ctx,
 				struct acpi_whea_header *entry,
 				void *data)
 {
 	struct apei_resources *resources = data;
-	struct acpi_generic_address *reg = &entry->register_region;
+	struct acpi_generic_address reg;
+	const struct acpi_generic_address *orig = &entry->register_region;
 	u8 ins = entry->instruction;
 	u64 paddr;
 	int rc;
@@ -566,17 +603,17 @@ static int collect_res_callback(struct apei_exec_context *ctx,
 	if (!(ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER))
 		return 0;
 
-	rc = apei_check_gar(reg, &paddr);
+	rc = apei_check_gar(&reg, orig, &paddr);
 	if (rc)
 		return rc;
 
-	switch (reg->space_id) {
+	switch (reg.space_id) {
 	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
 		return apei_res_add(&resources->iomem, paddr,
-				    reg->bit_width / 8);
+				    reg.bit_width / 8);
 	case ACPI_ADR_SPACE_SYSTEM_IO:
 		return apei_res_add(&resources->ioport, paddr,
-				    reg->bit_width / 8);
+				    reg.bit_width / 8);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index f57050e..63d43d8 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -82,6 +82,8 @@ int apei_exec_noop(struct apei_exec_context *ctx,
 		   struct acpi_whea_header *entry);
 int apei_exec_pre_map_gars(struct apei_exec_context *ctx);
 int apei_exec_post_unmap_gars(struct apei_exec_context *ctx);
+int apei_check_gar(struct acpi_generic_address *reg,
+		   const struct acpi_generic_address *orig, u64 *paddr);
 
 struct apei_resources {
 	struct list_head iomem;

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

[Bjorn Helgaas]

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.

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

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

[Thomas Renninger]

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.

   Thomas

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

[Myron Stowe]

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

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

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

[Rafael J. Wysocki]

On Thursday, November 03, 2011, Myron Stowe wrote:
> 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.
> >
> 
> 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.

This might work as well, but I wonder if it has any impact on performance?

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

We cat do this to start with, IMO, and then move to something more elaborate
if there are any problems.

Thanks,
Rafael

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

[Myron Stowe]

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

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

[Thomas Renninger]

On Saturday 05 November 2011 00:54:39 Myron Stowe wrote:
...
> > 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.
I see that this is yet another nasty problem of trying to let APEI use generic
acpi_read/write funcs.

...
 
> 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).
I can't see how it affects the two steps approach (if generic read/write could
ever get used... The main and really nice win of your patches is getting
rid of the duplicated and complicated/complex pre_map stuff).

What about below patch which even already compiles, it should work with your
atomicio.c removal one(s) (but not without!)?
Why would bit_offset handling need to be inside read/write funcs?
It could be done as before.

    Thomas
---
 drivers/acpi/apei/apei-base.c     |   91 +++++++++++++++++++++++++++++++++++-
 drivers/acpi/apei/apei-internal.h |    3 +
 drivers/acpi/apei/ghes.c          |    2 +-
 drivers/acpi/atomicio.c           |   42 -----------------
 include/acpi/atomicio.h           |    3 -
 5 files changed, 92 insertions(+), 49 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 6154036..9be1a58 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -70,7 +70,7 @@ int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val)
 {
 	int rc;
 
-	rc = acpi_atomic_read(val, &entry->register_region);
+	rc = apei_gar_read(val, &entry->register_region);
 	if (rc)
 		return rc;
 	*val >>= entry->register_region.bit_offset;
@@ -116,13 +116,13 @@ int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val)
 	val <<= entry->register_region.bit_offset;
 	if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
 		u64 valr = 0;
-		rc = acpi_atomic_read(&valr, &entry->register_region);
+		rc = apei_gar_read(&valr, &entry->register_region);
 		if (rc)
 			return rc;
 		valr &= ~(entry->mask << entry->register_region.bit_offset);
 		val |= valr;
 	}
-	rc = acpi_atomic_write(val, &entry->register_region);
+	rc = apei_gar_write(val, &entry->register_region);
 
 	return rc;
 }
@@ -553,6 +553,91 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
 	return 0;
 }
 
+int apei_gar_read(u64 *val, struct acpi_generic_address *reg)
+{
+	u64 paddr;
+	int rc;
+	u32 tmp_val;
+	acpi_status status;
+	u32 width = reg->bit_width;
+
+	if (width == 64)
+		width = 32;	/* Break into two 32-bit transfers */
+
+	rc = apei_check_gar(reg, &paddr);
+	if (rc)
+		return rc;
+
+	*val = 0;
+	switch (reg->space_id) {
+	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+		/* This part is copy and pasted from acpi_read */
+		status = acpi_os_read_memory((acpi_physical_address)
+					     paddr, &tmp_val, width);
+		if (ACPI_FAILURE(status))
+			return -EIO;
+		*val = tmp_val;
+
+		if (reg->bit_width == 64) {
+
+			/* Read the top 32 bits */
+
+			status = acpi_os_read_memory((acpi_physical_address)
+				     (paddr + 4), &tmp_val, 32);
+			if (ACPI_FAILURE(status))
+				return -EIO;
+			*val |= ((u64)tmp_val << 32);
+		}
+	case ACPI_ADR_SPACE_SYSTEM_IO:
+		status = acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
+		if (ACPI_FAILURE(status))
+		    return -EIO;
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(apei_gar_read);
+
+int apei_gar_write(u64 val, struct acpi_generic_address *reg)
+{
+	u64 paddr;
+	int rc;
+	acpi_status status;
+	u32 width = reg->bit_width;
+
+	if (width == 64)
+		width = 32;	/* Break into two 32-bit transfers */
+
+	rc = apei_check_gar(reg, &paddr);
+	if (rc)
+		return rc;
+
+	switch (reg->space_id) {
+	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+		/* This part is copy and pasted from acpi_write */
+		status = acpi_os_write_memory((acpi_physical_address)
+					      paddr, ACPI_LODWORD(val), width);
+
+		if (ACPI_FAILURE(status))
+			return -EIO;
+
+		if (reg->bit_width == 64) {
+			status = acpi_os_write_memory((acpi_physical_address)
+				      (paddr + 4), ACPI_HIDWORD(val), 32);
+
+			if (ACPI_FAILURE(status))
+				return -EIO;
+		}
+	case ACPI_ADR_SPACE_SYSTEM_IO:
+		status = acpi_os_write_port(paddr, val, reg->bit_width);
+		if (ACPI_FAILURE(status))
+		    return -EIO;
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(apei_gar_write);
+
 static int collect_res_callback(struct apei_exec_context *ctx,
 				struct acpi_whea_header *entry,
 				void *data)
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index f57050e..0380cf7 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -68,6 +68,9 @@ static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 actio
 /* IP has been set in instruction function */
 #define APEI_EXEC_SET_IP	1
 
+int apei_gar_read(u64 *val, struct acpi_generic_address *reg);
+int apei_gar_write(u64 val, struct acpi_generic_address *reg);
+
 int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val);
 int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val);
 int apei_exec_read_register(struct apei_exec_context *ctx,
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index b8e08cb..4273b2f 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -399,7 +399,7 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
 	u32 len;
 	int rc;
 
-	rc = acpi_atomic_read(&buf_paddr, &g->error_status_address);
+	rc = apei_gar_read(&buf_paddr, &g->error_status_address);
 	if (rc) {
 		if (!silent && printk_ratelimit())
 			pr_warning(FW_WARN GHES_PFX
diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
index 7489b89..2fb3fc0 100644
--- a/drivers/acpi/atomicio.c
+++ b/drivers/acpi/atomicio.c
@@ -321,45 +321,3 @@ static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)
 
 	return 0;
 }
-
-/* GAR accessing in atomic (including NMI) or process context */
-int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	int rc;
-
-	rc = acpi_check_gar(reg, &paddr, 1);
-	if (rc)
-		return rc;
-
-	*val = 0;
-	switch (reg->space_id) {
-	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
-		return acpi_atomic_read_mem(paddr, val, reg->bit_width);
-	case ACPI_ADR_SPACE_SYSTEM_IO:
-		return acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
-	default:
-		return -EINVAL;
-	}
-}
-EXPORT_SYMBOL_GPL(acpi_atomic_read);
-
-int acpi_atomic_write(u64 val, struct acpi_generic_address *reg)
-{
-	u64 paddr;
-	int rc;
-
-	rc = acpi_check_gar(reg, &paddr, 1);
-	if (rc)
-		return rc;
-
-	switch (reg->space_id) {
-	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
-		return acpi_atomic_write_mem(paddr, val, reg->bit_width);
-	case ACPI_ADR_SPACE_SYSTEM_IO:
-		return acpi_os_write_port(paddr, val, reg->bit_width);
-	default:
-		return -EINVAL;
-	}
-}
-EXPORT_SYMBOL_GPL(acpi_atomic_write);
diff --git a/include/acpi/atomicio.h b/include/acpi/atomicio.h
index 8b9fb4b..e38090c 100644
--- a/include/acpi/atomicio.h
+++ b/include/acpi/atomicio.h
@@ -4,7 +4,4 @@
 int acpi_pre_map_gar(struct acpi_generic_address *reg);
 int acpi_post_unmap_gar(struct acpi_generic_address *reg);
 
-int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg);
-int acpi_atomic_write(u64 val, struct acpi_generic_address *reg);
-
 #endif

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

[Rafael J. Wysocki]

On Saturday, November 05, 2011, Thomas Renninger wrote:
> On Saturday 05 November 2011 00:54:39 Myron Stowe wrote:
> ...
> > > 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.
> I see that this is yet another nasty problem of trying to let APEI use generic
> acpi_read/write funcs.
> 
> ...
>  
> > 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).
> I can't see how it affects the two steps approach (if generic read/write could
> ever get used... The main and really nice win of your patches is getting
> rid of the duplicated and complicated/complex pre_map stuff).
> 
> What about below patch which even already compiles, it should work with your
> atomicio.c removal one(s) (but not without!)?

I haven't analysed it in detail, but it looks like it should work.

> Why would bit_offset handling need to be inside read/write funcs?

Because of the warning we wanted to remove?

> It could be done as before.

Yes, and I think it pretty much has to be done this way if the
APEI_EXEC_PRESERVE_REGISTER is to be handled correctly.

Thanks,
Rafael


> ---
>  drivers/acpi/apei/apei-base.c     |   91 +++++++++++++++++++++++++++++++++++-
>  drivers/acpi/apei/apei-internal.h |    3 +
>  drivers/acpi/apei/ghes.c          |    2 +-
>  drivers/acpi/atomicio.c           |   42 -----------------
>  include/acpi/atomicio.h           |    3 -
>  5 files changed, 92 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index 6154036..9be1a58 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -70,7 +70,7 @@ int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val)
>  {
>  	int rc;
>  
> -	rc = acpi_atomic_read(val, &entry->register_region);
> +	rc = apei_gar_read(val, &entry->register_region);
>  	if (rc)
>  		return rc;
>  	*val >>= entry->register_region.bit_offset;
> @@ -116,13 +116,13 @@ int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val)
>  	val <<= entry->register_region.bit_offset;
>  	if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
>  		u64 valr = 0;
> -		rc = acpi_atomic_read(&valr, &entry->register_region);
> +		rc = apei_gar_read(&valr, &entry->register_region);
>  		if (rc)
>  			return rc;
>  		valr &= ~(entry->mask << entry->register_region.bit_offset);
>  		val |= valr;
>  	}
> -	rc = acpi_atomic_write(val, &entry->register_region);
> +	rc = apei_gar_write(val, &entry->register_region);
>  
>  	return rc;
>  }
> @@ -553,6 +553,91 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
>  	return 0;
>  }
>  
> +int apei_gar_read(u64 *val, struct acpi_generic_address *reg)
> +{
> +	u64 paddr;
> +	int rc;
> +	u32 tmp_val;
> +	acpi_status status;
> +	u32 width = reg->bit_width;
> +
> +	if (width == 64)
> +		width = 32;	/* Break into two 32-bit transfers */
> +
> +	rc = apei_check_gar(reg, &paddr);
> +	if (rc)
> +		return rc;
> +
> +	*val = 0;
> +	switch (reg->space_id) {
> +	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> +		/* This part is copy and pasted from acpi_read */
> +		status = acpi_os_read_memory((acpi_physical_address)
> +					     paddr, &tmp_val, width);
> +		if (ACPI_FAILURE(status))
> +			return -EIO;
> +		*val = tmp_val;
> +
> +		if (reg->bit_width == 64) {
> +
> +			/* Read the top 32 bits */
> +
> +			status = acpi_os_read_memory((acpi_physical_address)
> +				     (paddr + 4), &tmp_val, 32);
> +			if (ACPI_FAILURE(status))
> +				return -EIO;
> +			*val |= ((u64)tmp_val << 32);
> +		}
> +	case ACPI_ADR_SPACE_SYSTEM_IO:
> +		status = acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
> +		if (ACPI_FAILURE(status))
> +		    return -EIO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(apei_gar_read);
> +
> +int apei_gar_write(u64 val, struct acpi_generic_address *reg)
> +{
> +	u64 paddr;
> +	int rc;
> +	acpi_status status;
> +	u32 width = reg->bit_width;
> +
> +	if (width == 64)
> +		width = 32;	/* Break into two 32-bit transfers */
> +
> +	rc = apei_check_gar(reg, &paddr);
> +	if (rc)
> +		return rc;
> +
> +	switch (reg->space_id) {
> +	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> +		/* This part is copy and pasted from acpi_write */
> +		status = acpi_os_write_memory((acpi_physical_address)
> +					      paddr, ACPI_LODWORD(val), width);
> +
> +		if (ACPI_FAILURE(status))
> +			return -EIO;
> +
> +		if (reg->bit_width == 64) {
> +			status = acpi_os_write_memory((acpi_physical_address)
> +				      (paddr + 4), ACPI_HIDWORD(val), 32);
> +
> +			if (ACPI_FAILURE(status))
> +				return -EIO;
> +		}
> +	case ACPI_ADR_SPACE_SYSTEM_IO:
> +		status = acpi_os_write_port(paddr, val, reg->bit_width);
> +		if (ACPI_FAILURE(status))
> +		    return -EIO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(apei_gar_write);
> +
>  static int collect_res_callback(struct apei_exec_context *ctx,
>  				struct acpi_whea_header *entry,
>  				void *data)
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index f57050e..0380cf7 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -68,6 +68,9 @@ static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 actio
>  /* IP has been set in instruction function */
>  #define APEI_EXEC_SET_IP	1
>  
> +int apei_gar_read(u64 *val, struct acpi_generic_address *reg);
> +int apei_gar_write(u64 val, struct acpi_generic_address *reg);
> +
>  int __apei_exec_read_register(struct acpi_whea_header *entry, u64 *val);
>  int __apei_exec_write_register(struct acpi_whea_header *entry, u64 val);
>  int apei_exec_read_register(struct apei_exec_context *ctx,
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index b8e08cb..4273b2f 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -399,7 +399,7 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
>  	u32 len;
>  	int rc;
>  
> -	rc = acpi_atomic_read(&buf_paddr, &g->error_status_address);
> +	rc = apei_gar_read(&buf_paddr, &g->error_status_address);
>  	if (rc) {
>  		if (!silent && printk_ratelimit())
>  			pr_warning(FW_WARN GHES_PFX
> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
> index 7489b89..2fb3fc0 100644
> --- a/drivers/acpi/atomicio.c
> +++ b/drivers/acpi/atomicio.c
> @@ -321,45 +321,3 @@ static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)
>  
>  	return 0;
>  }
> -
> -/* GAR accessing in atomic (including NMI) or process context */
> -int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	int rc;
> -
> -	rc = acpi_check_gar(reg, &paddr, 1);
> -	if (rc)
> -		return rc;
> -
> -	*val = 0;
> -	switch (reg->space_id) {
> -	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> -		return acpi_atomic_read_mem(paddr, val, reg->bit_width);
> -	case ACPI_ADR_SPACE_SYSTEM_IO:
> -		return acpi_os_read_port(paddr, (u32 *)val, reg->bit_width);
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(acpi_atomic_read);
> -
> -int acpi_atomic_write(u64 val, struct acpi_generic_address *reg)
> -{
> -	u64 paddr;
> -	int rc;
> -
> -	rc = acpi_check_gar(reg, &paddr, 1);
> -	if (rc)
> -		return rc;
> -
> -	switch (reg->space_id) {
> -	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> -		return acpi_atomic_write_mem(paddr, val, reg->bit_width);
> -	case ACPI_ADR_SPACE_SYSTEM_IO:
> -		return acpi_os_write_port(paddr, val, reg->bit_width);
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(acpi_atomic_write);
> diff --git a/include/acpi/atomicio.h b/include/acpi/atomicio.h
> index 8b9fb4b..e38090c 100644
> --- a/include/acpi/atomicio.h
> +++ b/include/acpi/atomicio.h
> @@ -4,7 +4,4 @@
>  int acpi_pre_map_gar(struct acpi_generic_address *reg);
>  int acpi_post_unmap_gar(struct acpi_generic_address *reg);
>  
> -int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg);
> -int acpi_atomic_write(u64 val, struct acpi_generic_address *reg);
> -
>  #endif
> 
> 

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

[Bjorn Helgaas]

On Fri, Nov 4, 2011 at 5:54 PM, Myron Stowe <myron.stowe@gmail.com> wrote:
> 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.

I think this analysis is incorrect because it keeps the bit_offset
shift in __apei_exec_write_register() even though it assumes
acpi_read() uses bit_offset to extract the region.

Assume a 64-bit CSR at 0x1000, with bit_offset = 16 in the GAS and an
8-bit mask, so the region of interest is bits [23:16]:

    gas->address                0x1000
    gas->bit_offset             16 (0x10)
    entry->mask                 0x00000000000000ff

Here's how we handle this in the current __apei_exec_write_register(),
with acpi_read() ignoring bit_offset:

      val &= entry->mask;
      val <<= entry->register_region.bit_offset;
      if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
          acpi_atomic_read(&valr, &entry->register_region);
          valr &= ~(entry->mask << entry->register_region.bit_offset);
          val |= valr;
      }
      acpi_atomic_write(val, &entry->register_region);

      initial value @0x1000     0xfedcba9876543210
      value to write (val)      0x0123456789abcdef
      val &= mask               0x00000000000000EF
      val <<= bit_offset        0x0000000000EF0000
      valr                      0xfedcba9876543210 (from acpi_atomic_read)
      valr &= ~(mask << offset) 0xfedcba9876003210
      val |= valr               0xfedcba9876EF3210
      final value @0x1000       0xfedcba9876EF3210 (by acpi_atomic_write)

If we make acpi_read() & acpi_write() pay attention to bit_offset,
__apei_exec_write_register() should look like this:

      val &= entry->mask;
      if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
          acpi_read(&valr, &entry->register_region);  /* valr = bits [xx:16] */
          valr &= ~entry->mask;
          val |= valr;
      }
      acpi_write(val, &entry->register_region);  /* writes only bits [xx:16] */

      initial value @0x1000     0xfedcba9876543210
      value to write (val)      0x0123456789abcdef
      val &= mask               0x00000000000000EF
      valr                      0x0000fedcba987654 (from acpi_read)
      valr &= ~mask             0x0000fedcba987600
      val |= valr               0x0000fedcba9876EF
      final value @0x1000       0xfedcba9876EF3210 (by acpi_write)

This depends on some assumptions about how acpi_read() and
acpi_write() deal with bit_offset and probably bit_width.  I don't
think we have those behaviors all worked out yet, but I think it's
possible, and this is a good opportunity to do it.

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

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

[Rafael J. Wysocki]

On Monday, October 31, 2011, Thomas Renninger 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.

Why does that matter at all?  If bit_offset != 0 is used in _any_ tables,
then we should support that.

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

Do we actually know how Windows works in that respect?

Rafael

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

[Rafael J. Wysocki]

On Friday, October 28, 2011, 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.

Agreed.

Thanks,
Rafael

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

[Bjorn Helgaas]

On Thu, Oct 27, 2011 at 7:49 PM, Thomas Renninger <trenn@suse.de> wrote:
> There is another problem. Would be great to get some opinions/feedback
> on it already:
> APEI GAR entries seem to have invalid bit_width entries on some platforms.
> After looking at several tables, I expect that with APEI tables access width
> (in bytes) should get used instead, Windows seem to ignore bit width fields,
> at least for APEI tables.

I'm confused.  How can you tell that the bit_width is incorrect?  My
understanding is that the bit_width is the size of the *register*,
while the access_width is the size of access the processor must
generate on the bus.  The access_width may be larger, for example, if
the hardware only supports 32-bit or 64-bit reads.  So I don't
understand how you can derive bit_width from access_width.

In the example below, I think we're supposed to do a 64-bit read, then
extract 8 bits that contain the register of interest.  If we keep all
64 bits, I don't see how that can be correct.

> ...
> Comparing different Generic Adress Register definitions of
> different vendors it came out that bit width (at least in APEI
> tables) is sometimes wrong or used different compared to older
> ACPI BIOS definitions (e.g. older FACP tables).
> It looks like Windows ignores the bit width field in
> latest implementations. Either in APEI table parts only
> (I'd say more likely) or in other ACPI parts as well.
>
> Worst case is that an address value to be read from a GAR structure
> can have a 8 bit width definition resulting in:
> ERST: Can not request iomem region <0x              3f-0x              3f>
> while the access width is correct:
> [1B0h 0432  1]                       Action : 0D (Get Error Address Range)
> [1B4h 0436 12]              Register Region : <Generic Address Structure>
> [1B4h 0436  1]                     Space ID : 00 (SystemMemory)
> [1B5h 0437  1]                    Bit Width : 08
> [1B6h 0438  1]                   Bit Offset : 00
> [1B7h 0439  1]         Encoded Access Width : 04 (QWord Access:64)
> [1B8h 0440  8]                      Address : 000000007F8A8032

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

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

[Thomas Renninger]

On Friday 28 October 2011 17:14:39 Bjorn Helgaas wrote:
> On Thu, Oct 27, 2011 at 7:49 PM, Thomas Renninger <trenn@suse.de> wrote:
> > There is another problem. Would be great to get some opinions/feedback
> > on it already:
> > APEI GAR entries seem to have invalid bit_width entries on some platforms.
> > After looking at several tables, I expect that with APEI tables access width
> > (in bytes) should get used instead, Windows seem to ignore bit width fields,
> > at least for APEI tables.
> 
> I'm confused.  How can you tell that the bit_width is incorrect My
> understanding is that the bit_width is the size of the *register*,
> while the access_width is the size of access the processor must
> generate on the bus.  The access_width may be larger, for example, if
> the hardware only supports 32-bit or 64-bit reads.
This also is my understanding.
> So I don't understand how you can derive bit_width from access_width.
The problem is that Windows seem to ignore the bit width field, at least
for APEI tables.
There you also have the mask value and bit width information is not
needed to determine the bits of interest.
While mostly bit width information is correct, I compared quite some
tables and especially IBM is adding some interesting values.
But I've also seen wrong bit widths on Supermicro and others.


> In the example below, I think we're supposed to do a 64-bit read, then
> extract 8 bits that contain the register of interest.  If we keep all
> 64 bits, I don't see how that can be correct.
Yep. But "Get Error Address Range" is supposed to return/contain an iomem address.
Cutting it down to 8 bits produces below error when trying to
reserve a memory region from the retrieved value/address at 0x3f.
Using acpidump to evaluate the register's 8 byte shows:
acpidump --addr 0x7F8A8032 --length 8
0x000000007f8a803f
which perfectly fits into an unused e820 reserved memory region.

Same for "Get Error Address Length" on this machine:
[1D0h 0464  1]                       Action : 0E (Get Error Address Length)
[1D5h 0469  1]                    Bit Width : 08
[1D7h 0471  1]         Encoded Access Width : 03 (DWord Access:32)
[1D8h 0472  8]                      Address : 000000007F8A803A
[1E8h 0488  8]                         Mask : 00000000FFFFFFFF

Reading 0x7F8A803A you get:
0x2000
which is exactly what you expect as "error memory length", but cut
down to 8 bits it's wrong.
Look at the mask value from which you could derive bit width (on APEI tables
only), it should be 0x16, but it's not needed (bit width info) and should
get ignored because it's sometimes wrong.

The question remains whether Windows (which versions?) is only ignoring bit
width in their APEI implementation (this is what I currently expect).
For now, as we have separate APEI GAR checking it should be fine to
only ignore bit width there. But keeping an eye open and check
other ACPI tables containing GAR structures out there in the field
is certainly a good idea.

Another "wrong" GAR definition I've found (only once) is that all fields
(bit width, byte access, ...) are zero, similar to optional FADT GAR structures.
I didn't runtime test it, but currently this should pass an error
and invalidate the whole table (for example should result in erst_disable=1).
Additional logic is needed to ignore and not use this serialization command
as if it does not exist.

  Thomas

> > ...
> > Comparing different Generic Adress Register definitions of
> > different vendors it came out that bit width (at least in APEI
> > tables) is sometimes wrong or used different compared to older
> > ACPI BIOS definitions (e.g. older FACP tables).
> > It looks like Windows ignores the bit width field in
> > latest implementations. Either in APEI table parts only
> > (I'd say more likely) or in other ACPI parts as well.
> >
> > Worst case is that an address value to be read from a GAR structure
> > can have a 8 bit width definition resulting in:
> > ERST: Can not request iomem region <0x              3f-0x              3f>
> > while the access width is correct:
> > [1B0h 0432  1]                       Action : 0D (Get Error Address Range)
> > [1B4h 0436 12]              Register Region : <Generic Address Structure>
> > [1B4h 0436  1]                     Space ID : 00 (SystemMemory)
> > [1B5h 0437  1]                    Bit Width : 08
> > [1B6h 0438  1]                   Bit Offset : 00
> > [1B7h 0439  1]         Encoded Access Width : 04 (QWord Access:64)
> > [1B8h 0440  8]                      Address : 000000007F8A8032
> 
> Bjorn

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

[Bjorn Helgaas]

On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Friday 28 October 2011 17:14:39 Bjorn Helgaas wrote:
>> On Thu, Oct 27, 2011 at 7:49 PM, Thomas Renninger <trenn@suse.de> wrote:
>> > There is another problem. Would be great to get some opinions/feedback
>> > on it already:
>> > APEI GAR entries seem to have invalid bit_width entries on some platforms.
>> > After looking at several tables, I expect that with APEI tables access width
>> > (in bytes) should get used instead, Windows seem to ignore bit width fields,
>> > at least for APEI tables.
>>
>> I'm confused.  How can you tell that the bit_width is incorrect My
>> understanding is that the bit_width is the size of the *register*,
>> while the access_width is the size of access the processor must
>> generate on the bus.  The access_width may be larger, for example, if
>> the hardware only supports 32-bit or 64-bit reads.
> This also is my understanding.
>> So I don't understand how you can derive bit_width from access_width.
> The problem is that Windows seem to ignore the bit width field, at least
> for APEI tables.
> There you also have the mask value and bit width information is not
> needed to determine the bits of interest.
> While mostly bit width information is correct, I compared quite some
> tables and especially IBM is adding some interesting values.
> But I've also seen wrong bit widths on Supermicro and others.
>
>
>> In the example below, I think we're supposed to do a 64-bit read, then
>> extract 8 bits that contain the register of interest.  If we keep all
>> 64 bits, I don't see how that can be correct.
> Yep. But "Get Error Address Range" is supposed to return/contain an iomem address.
> Cutting it down to 8 bits produces below error when trying to
> reserve a memory region from the retrieved value/address at 0x3f.
> Using acpidump to evaluate the register's 8 byte shows:
> acpidump --addr 0x7F8A8032 --length 8
> 0x000000007f8a803f
> which perfectly fits into an unused e820 reserved memory region.
>
> Same for "Get Error Address Length" on this machine:
> [1D0h 0464  1]                       Action : 0E (Get Error Address Length)
> [1D5h 0469  1]                    Bit Width : 08
> [1D7h 0471  1]         Encoded Access Width : 03 (DWord Access:32)
> [1D8h 0472  8]                      Address : 000000007F8A803A
> [1E8h 0488  8]                         Mask : 00000000FFFFFFFF
>
> Reading 0x7F8A803A you get:
> 0x2000
> which is exactly what you expect as "error memory length", but cut
> down to 8 bits it's wrong.
> Look at the mask value from which you could derive bit width (on APEI tables
> only), it should be 0x16, but it's not needed (bit width info) and should
> get ignored because it's sometimes wrong.
>
> The question remains whether Windows (which versions?) is only ignoring bit
> width in their APEI implementation (this is what I currently expect).
> For now, as we have separate APEI GAR checking it should be fine to
> only ignore bit width there. But keeping an eye open and check
> other ACPI tables containing GAR structures out there in the field
> is certainly a good idea.

Seems like these are BIOS bugs.  Do we know for sure that Windows
consumes this information that seems to be wrong?  Have you had a
conversation with the vendor about whether the BIOS is at fault here?

If we make Linux ignore the bit_width, that might "fix" these boxes
with broken BIOSes, but at the cost of breaking a box that uses
bit_width correctly.

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

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

[Thomas Renninger]

On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:

> Seems like these are BIOS bugs.  Do we know for sure that Windows
> consumes this information that seems to be wrong?  Have you had a
> conversation with the vendor about whether the BIOS is at fault here?
Such closed specifications between a major OS and specific HW vendors
should be forbidden by law and I expect in some countries you'll win
if you contest this contract in a high enough court...
APEI is based on the Windows WHEA specification which only specific
vendors can retrieve from Windows if they sign an NDA contract.
I could imagine there you find details about the GAS structure usage
in WHEA/APEI tables the way Windows like it.

After looking at quite a lot APEI tables and their bit width, byte access
and mask values, I am pretty sure bit width is ignored on Windows.
Or say, if these tables are used, access width is always correct while
bit width is not.
 
> If we make Linux ignore the bit_width, that might "fix" these boxes
> with broken BIOSes, but at the cost of breaking a box that uses
> bit_width correctly.
None of the "broken bit width" boxes I looked at should break if
access width is used.

   Thomas

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

[Bjorn Helgaas]

On Thu, Nov 3, 2011 at 3:16 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
>> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
>
>> Seems like these are BIOS bugs.  Do we know for sure that Windows
>> consumes this information that seems to be wrong?  Have you had a
>> conversation with the vendor about whether the BIOS is at fault here?
> Such closed specifications between a major OS and specific HW vendors
> should be forbidden by law and I expect in some countries you'll win
> if you contest this contract in a high enough court...
> APEI is based on the Windows WHEA specification which only specific
> vendors can retrieve from Windows if they sign an NDA contract.
> I could imagine there you find details about the GAS structure usage
> in WHEA/APEI tables the way Windows like it.
>
> After looking at quite a lot APEI tables and their bit width, byte access
> and mask values, I am pretty sure bit width is ignored on Windows.
> Or say, if these tables are used, access width is always correct while
> bit width is not.
>
>> If we make Linux ignore the bit_width, that might "fix" these boxes
>> with broken BIOSes, but at the cost of breaking a box that uses
>> bit_width correctly.
> None of the "broken bit width" boxes I looked at should break if
> access width is used.

Yes, but bit_width is a standard part of the ACPI generic address
structure, and there's nothing to prevent a future BIOS from using it
and expecting it to work as documented.  If we make Linux
ignore/override the bit_width now, we'll build a legacy of machines
that depend on the fact that we ignore it, and then we would have no
way to deal with future machines that might expect us to pay attention
to it.

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

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

[Thomas Renninger]

On Thursday, November 03, 2011 02:53:09 PM Bjorn Helgaas wrote:
> On Thu, Nov 3, 2011 at 3:16 AM, Thomas Renninger <trenn@suse.de> wrote:
> > On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
> >> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
> >
> >> Seems like these are BIOS bugs.  Do we know for sure that Windows
> >> consumes this information that seems to be wrong?  Have you had a
> >> conversation with the vendor about whether the BIOS is at fault here?
> > Such closed specifications between a major OS and specific HW vendors
> > should be forbidden by law and I expect in some countries you'll win
> > if you contest this contract in a high enough court...
> > APEI is based on the Windows WHEA specification which only specific
> > vendors can retrieve from Windows if they sign an NDA contract.
> > I could imagine there you find details about the GAS structure usage
> > in WHEA/APEI tables the way Windows like it.
> >
> > After looking at quite a lot APEI tables and their bit width, byte access
> > and mask values, I am pretty sure bit width is ignored on Windows.
> > Or say, if these tables are used, access width is always correct while
> > bit width is not.
> >
> >> If we make Linux ignore the bit_width, that might "fix" these boxes
> >> with broken BIOSes, but at the cost of breaking a box that uses
> >> bit_width correctly.
> > None of the "broken bit width" boxes I looked at should break if
> > access width is used.
> 
> Yes, but bit_width is a standard part of the ACPI generic address
> structure, and there's nothing to prevent a future BIOS from using it
> and expecting it to work as documented.
Yep, but access width is also part of the standard ACPI generic address
structure and currently gets totally ignored and bit width is used instead.

I just realized: what code is using acpi_read?
I can't find any user...

So we can either:
  - modify acpi_read/write and implement things as needed -> nobody
    uses it
  - as acpi_read is acpica code we can for now leave this in apei parts
    (still consolidate duplicate code from atomicio.c, remove pre_map
    stuff and the whole atomicio.c file) and still keep apei_acpi_read
    for now in apei-base.c. There we can implement what we like to see
    in acpi_read:
       - if access width is zero -> use bit width to determine how many
         bytes have to be read
       - otherwise use access width

Also an APEI version for apei_check_gar in apei-base.c isn't that bad
for now.

As there are no users of acpi_read and acpi_write yet, it might be a good
idea for now if this function simply reads the amount of bytes as
described above and leave it up to the caller to cut off any bytes
using bit width, bit offset or (for APEI GAR structs only) using the mask.
(As done by current APEI code, unfortunately twice).

> If we make Linux
> ignore/override the bit_width now, we'll build a legacy of machines
> that depend on the fact that we ignore it, and then we would have no
> way to deal with future machines that might expect us to pay attention
> to it.
bit width is not ignored it should get used if access width is zero or
also to cut off bits.
It can get ignored for APEI tables as the mask value already defines
which bits should get used and therefore seem to be ignored on other
OSes.

   Thomas

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

[Myron Stowe]

On Thu, Nov 3, 2011 at 10:18 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Thursday, November 03, 2011 02:53:09 PM Bjorn Helgaas wrote:
>> On Thu, Nov 3, 2011 at 3:16 AM, Thomas Renninger <trenn@suse.de> wrote:
>> > On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
>> >> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
>> >
>> >> Seems like these are BIOS bugs.  Do we know for sure that Windows
>> >> consumes this information that seems to be wrong?  Have you had a
>> >> conversation with the vendor about whether the BIOS is at fault here?
>> > Such closed specifications between a major OS and specific HW vendors
>> > should be forbidden by law and I expect in some countries you'll win
>> > if you contest this contract in a high enough court...
>> > APEI is based on the Windows WHEA specification which only specific
>> > vendors can retrieve from Windows if they sign an NDA contract.
>> > I could imagine there you find details about the GAS structure usage
>> > in WHEA/APEI tables the way Windows like it.
>> >
>> > After looking at quite a lot APEI tables and their bit width, byte access
>> > and mask values, I am pretty sure bit width is ignored on Windows.
>> > Or say, if these tables are used, access width is always correct while
>> > bit width is not.
>> >
>> >> If we make Linux ignore the bit_width, that might "fix" these boxes
>> >> with broken BIOSes, but at the cost of breaking a box that uses
>> >> bit_width correctly.
>> > None of the "broken bit width" boxes I looked at should break if
>> > access width is used.
>>
>> Yes, but bit_width is a standard part of the ACPI generic address
>> structure, and there's nothing to prevent a future BIOS from using it
>> and expecting it to work as documented.
> Yep, but access width is also part of the standard ACPI generic address
> structure and currently gets totally ignored and bit width is used instead.
>
> I just realized: what code is using acpi_read?
> I can't find any user...
>
> So we can either:
>  - modify acpi_read/write and implement things as needed -> nobody
>    uses it
>  - as acpi_read is acpica code we can for now leave this in apei parts
>    (still consolidate duplicate code from atomicio.c, remove pre_map
>    stuff and the whole atomicio.c file) and still keep apei_acpi_read
>    for now in apei-base.c. There we can implement what we like to see
>    in acpi_read:
>       - if access width is zero -> use bit width to determine how many
>         bytes have to be read
>       - otherwise use access width
>
> Also an APEI version for apei_check_gar in apei-base.c isn't that bad
> for now.
>
> As there are no users of acpi_read and acpi_write yet, it might be a good
> idea for now if this function simply reads the amount of bytes as
> described above and leave it up to the caller to cut off any bytes
> using bit width, bit offset or (for APEI GAR structs only) using the mask.
> (As done by current APEI code, unfortunately twice).
>

For now, I would like to continue with converting APEI to remove atomicio.[ch]
preserving the existing atomicio behavior - basically the same patch I posted
a few weeks ago with the additions mentioned last night.

This would leave the more generic code (osl.c) unbiased for now.  We can
then account for all the APEI uniqueness - bit_width/access_width/mask -
in APEI code, not in the more generic code.

As far as I can tell, the two paths each of us want to take do not seem to
interfere with each other.  Do you see, or have, any issues with that approach?

Thanks,
 Myron
>> If we make Linux
>> ignore/override the bit_width now, we'll build a legacy of machines
>> that depend on the fact that we ignore it, and then we would have no
>> way to deal with future machines that might expect us to pay attention
>> to it.
> bit width is not ignored it should get used if access width is zero or
> also to cut off bits.
> It can get ignored for APEI tables as the mask value already defines
> which bits should get used and therefore seem to be ignored on other
> OSes.
>
>   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

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

[Thomas Renninger]

On Thursday 03 November 2011 17:44:06 Myron Stowe wrote:
> On Thu, Nov 3, 2011 at 10:18 AM, Thomas Renninger <trenn@suse.de> wrote:
> > On Thursday, November 03, 2011 02:53:09 PM Bjorn Helgaas wrote:
> >> On Thu, Nov 3, 2011 at 3:16 AM, Thomas Renninger <trenn@suse.de> wrote:
> >> > On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
> >> >> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
> >> >
> >> >> Seems like these are BIOS bugs.  Do we know for sure that Windows
> >> >> consumes this information that seems to be wrong?  Have you had a
> >> >> conversation with the vendor about whether the BIOS is at fault here?
> >> > Such closed specifications between a major OS and specific HW vendors
> >> > should be forbidden by law and I expect in some countries you'll win
> >> > if you contest this contract in a high enough court...
> >> > APEI is based on the Windows WHEA specification which only specific
> >> > vendors can retrieve from Windows if they sign an NDA contract.
> >> > I could imagine there you find details about the GAS structure usage
> >> > in WHEA/APEI tables the way Windows like it.
> >> >
> >> > After looking at quite a lot APEI tables and their bit width, byte access
> >> > and mask values, I am pretty sure bit width is ignored on Windows.
> >> > Or say, if these tables are used, access width is always correct while
> >> > bit width is not.
> >> >
> >> >> If we make Linux ignore the bit_width, that might "fix" these boxes
> >> >> with broken BIOSes, but at the cost of breaking a box that uses
> >> >> bit_width correctly.
> >> > None of the "broken bit width" boxes I looked at should break if
> >> > access width is used.
> >>
> >> Yes, but bit_width is a standard part of the ACPI generic address
> >> structure, and there's nothing to prevent a future BIOS from using it
> >> and expecting it to work as documented.
> > Yep, but access width is also part of the standard ACPI generic address
> > structure and currently gets totally ignored and bit width is used instead.
> >
> > I just realized: what code is using acpi_read?
> > I can't find any user...
> >
> > So we can either:
> >  - modify acpi_read/write and implement things as needed -> nobody
> >    uses it
> >  - as acpi_read is acpica code we can for now leave this in apei parts
> >    (still consolidate duplicate code from atomicio.c, remove pre_map
> >    stuff and the whole atomicio.c file) and still keep apei_acpi_read
> >    for now in apei-base.c. There we can implement what we like to see
> >    in acpi_read:
> >       - if access width is zero -> use bit width to determine how many
> >         bytes have to be read
> >       - otherwise use access width
> >
> > Also an APEI version for apei_check_gar in apei-base.c isn't that bad
> > for now.
> >
> > As there are no users of acpi_read and acpi_write yet, it might be a good
> > idea for now if this function simply reads the amount of bytes as
> > described above and leave it up to the caller to cut off any bytes
> > using bit width, bit offset or (for APEI GAR structs only) using the mask.
> > (As done by current APEI code, unfortunately twice).
> >
> 
> For now, I would like to continue with converting APEI to remove atomicio.[ch]
> preserving the existing atomicio behavior - basically the same patch I posted
> a few weeks ago with the additions mentioned last night.
You mean this:
  struct acpi_generic_address gas;  /* on local stack */
  gas = entry->register_region;
  gas.bit_offset = 0;
  acpi_read(val, &gas);


> This would leave the more generic code (osl.c) unbiased for now.  We can
> then account for all the APEI uniqueness - bit_width/access_width/mask -
> in APEI code, not in the more generic code.
Better don't use acpi_read (and thus also acpi_hw_validate_register()) in a
first step. This is acpica code and modifying it in -rcX releases is not
a good idea. acpi_read/write also might be used on other OSes (even likely,
this would explain why it's unused on Linux) and getting changes in there
could get really difficult.
Also above "gas.bit_offset = 0" is not nice and not needed for now:
Better copy over acpi_atomic_write, acpi_atomic_read and acpi_check_gar()
to apei-base.c (declaring them in apei-internal.h should be enough).
apei_write, apei_read and apei_check_gar should be better names.

I fully agree with:
> I would like to continue with converting APEI to remove atomicio.[ch]
> preserving the existing atomicio behavior

You might want to add "considering access width" as I suggested or similar.
Or I can go on top later.

> As far as I can tell, the two paths each of us want to take do not seem to
> interfere with each other.
We are on the same road.

> Do you see, or have, any issues with that approach?
It's only the "use generic acpi_read/write()" issue.
Let's better try to get rid of apei_read/write/check_gar specific functions
in a second step (the functions I suggested above to get copied from atomicio.c
(from acpi_atomic_read/write and acpi_check_gar()) to apei-base.c and
enhanced with access width considerations) and incorporate this
into acpica after acpica people had more time and a closer look at this.

     Thomas

> Thanks,
>  Myron
> >> If we make Linux
> >> ignore/override the bit_width now, we'll build a legacy of machines
> >> that depend on the fact that we ignore it, and then we would have no
> >> way to deal with future machines that might expect us to pay attention
> >> to it.
> > bit width is not ignored it should get used if access width is zero or
> > also to cut off bits.
> > It can get ignored for APEI tables as the mask value already defines
> > which bits should get used and therefore seem to be ignored on other
> > OSes.
> >
> >   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
> >
> 

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

[Myron Stowe]

On Thu, Nov 3, 2011 at 8:16 PM, Thomas Renninger <trenn@suse.de> wrote:
> On Thursday 03 November 2011 17:44:06 Myron Stowe wrote:
>> On Thu, Nov 3, 2011 at 10:18 AM, Thomas Renninger <trenn@suse.de> wrote:
>> > On Thursday, November 03, 2011 02:53:09 PM Bjorn Helgaas wrote:
>> >> On Thu, Nov 3, 2011 at 3:16 AM, Thomas Renninger <trenn@suse.de> wrote:
>> >> > On Monday, October 31, 2011 04:51:07 PM Bjorn Helgaas wrote:
>> >> >> On Mon, Oct 31, 2011 at 4:33 AM, Thomas Renninger <trenn@suse.de> wrote:
>> >> >
>> >> >> Seems like these are BIOS bugs.  Do we know for sure that Windows
>> >> >> consumes this information that seems to be wrong?  Have you had a
>> >> >> conversation with the vendor about whether the BIOS is at fault here?
>> >> > Such closed specifications between a major OS and specific HW vendors
>> >> > should be forbidden by law and I expect in some countries you'll win
>> >> > if you contest this contract in a high enough court...
>> >> > APEI is based on the Windows WHEA specification which only specific
>> >> > vendors can retrieve from Windows if they sign an NDA contract.
>> >> > I could imagine there you find details about the GAS structure usage
>> >> > in WHEA/APEI tables the way Windows like it.
>> >> >
>> >> > After looking at quite a lot APEI tables and their bit width, byte access
>> >> > and mask values, I am pretty sure bit width is ignored on Windows.
>> >> > Or say, if these tables are used, access width is always correct while
>> >> > bit width is not.
>> >> >
>> >> >> If we make Linux ignore the bit_width, that might "fix" these boxes
>> >> >> with broken BIOSes, but at the cost of breaking a box that uses
>> >> >> bit_width correctly.
>> >> > None of the "broken bit width" boxes I looked at should break if
>> >> > access width is used.
>> >>
>> >> Yes, but bit_width is a standard part of the ACPI generic address
>> >> structure, and there's nothing to prevent a future BIOS from using it
>> >> and expecting it to work as documented.
>> > Yep, but access width is also part of the standard ACPI generic address
>> > structure and currently gets totally ignored and bit width is used instead.
>> >
>> > I just realized: what code is using acpi_read?
>> > I can't find any user...
>> >
>> > So we can either:
>> >  - modify acpi_read/write and implement things as needed -> nobody
>> >    uses it
>> >  - as acpi_read is acpica code we can for now leave this in apei parts
>> >    (still consolidate duplicate code from atomicio.c, remove pre_map
>> >    stuff and the whole atomicio.c file) and still keep apei_acpi_read
>> >    for now in apei-base.c. There we can implement what we like to see
>> >    in acpi_read:
>> >       - if access width is zero -> use bit width to determine how many
>> >         bytes have to be read
>> >       - otherwise use access width
>> >
>> > Also an APEI version for apei_check_gar in apei-base.c isn't that bad
>> > for now.
>> >
>> > As there are no users of acpi_read and acpi_write yet, it might be a good
>> > idea for now if this function simply reads the amount of bytes as
>> > described above and leave it up to the caller to cut off any bytes
>> > using bit width, bit offset or (for APEI GAR structs only) using the mask.
>> > (As done by current APEI code, unfortunately twice).
>> >
>>
>> For now, I would like to continue with converting APEI to remove atomicio.[ch]
>> preserving the existing atomicio behavior - basically the same patch I posted
>> a few weeks ago with the additions mentioned last night.
> You mean this:
>  struct acpi_generic_address gas;  /* on local stack */
>  gas = entry->register_region;
>  gas.bit_offset = 0;
>  acpi_read(val, &gas);
>
>
>> This would leave the more generic code (osl.c) unbiased for now.  We can
>> then account for all the APEI uniqueness - bit_width/access_width/mask -
>> in APEI code, not in the more generic code.
> Better don't use acpi_read (and thus also acpi_hw_validate_register()) in a
> first step. This is acpica code and modifying it in -rcX releases is not
> a good idea. acpi_read/write also might be used on other OSes (even likely,
> this would explain why it's unused on Linux) and getting changes in there
> could get really difficult.

Yes, while I would like to end up with such (converting to acpi_read/acpi_write)
eventually it's obvious now that we would need to augment them to handle
GAS 'bit_offset' fields properly which, as you point out, may be a long
slow road.

Just an FYI; Bjorn pointed out that acpi_read is currently used in Linux in
cpufreq.

> Also above "gas.bit_offset = 0" is not nice and not needed for now:
> Better copy over acpi_atomic_write, acpi_atomic_read and acpi_check_gar()
> to apei-base.c (declaring them in apei-internal.h should be enough).
> apei_write, apei_read and apei_check_gar should be better names.
>
> I fully agree with:
>> I would like to continue with converting APEI to remove atomicio.[ch]
>> preserving the existing atomicio behavior
>
> You might want to add "considering access width" as I suggested or similar.
> Or I can go on top later.
>
>> As far as I can tell, the two paths each of us want to take do not seem to
>> interfere with each other.
> We are on the same road.
>
>> Do you see, or have, any issues with that approach?
> It's only the "use generic acpi_read/write()" issue.
> Let's better try to get rid of apei_read/write/check_gar specific functions
> in a second step (the functions I suggested above to get copied from atomicio.c
> (from acpi_atomic_read/write and acpi_check_gar()) to apei-base.c and
> enhanced with access width considerations) and incorporate this
> into acpica after acpica people had more time and a closer look at this.

Ok, I'll start working on a cut of -v2 tomorrow - it's getting late for me.

Thanks,
 Myron
>
>     Thomas
>
>> Thanks,
>>  Myron
>> >> If we make Linux
>> >> ignore/override the bit_width now, we'll build a legacy of machines
>> >> that depend on the fact that we ignore it, and then we would have no
>> >> way to deal with future machines that might expect us to pay attention
>> >> to it.
>> > bit width is not ignored it should get used if access width is zero or
>> > also to cut off bits.
>> > It can get ignored for APEI tables as the mask value already defines
>> > which bits should get used and therefore seem to be ignored on other
>> > OSes.
>> >
>> >   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

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

[Myron Stowe]

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.
>
> There is another problem. Would be great to get some opinions/feedback
> on it already:
> APEI GAR entries seem to have invalid bit_width entries on some platforms.
> After looking at several tables, I expect that with APEI tables access width
> (in bytes) should get used instead, Windows seem to ignore bit width fields,
> at least for APEI tables.
>
> I have patches but I need to know whether your patches are accepted.
>
> There also is a problem with modifying GAR bit_width field to be able to
> pass it to the generic acpica functions (what your patches are doing).
> The problem is that the structures are located in reserved BIOS areas and
> they should be const and not get modified.
>
> I have 2 solutions for this:
>  1) Make apei_check_gar() pass 2 struct acpi_generic_address:
>     int apei_check_gar(struct acpi_generic_address *reg,
>                    const struct acpi_generic_address *orig, u64 *paddr)
>     orig -> is the one located in reserved mem area, mapped to virtual addr space
>     reg  -> will be a copy of it, possibly with bit_width adjusted which then
>            can be passed to acpi_memory_read/write and friends.
>  2) Allocate memory and copy whole APEI tables
>
> 1. Should have the advantage of less memory waste
> 2. Should have the advantage of a bit nicer code (kalloc and memcpy per table) and
>   if more things like this happen, tables could get adjusted easily.
>   It also has the advantage that GAR structures do not need to get double
>   checked and eventually adjusted at runtime again.
>
> Below is the first patch which goes for 1. More patches may be needed, but I
> as said, I need to know whether your patches get accepted.
> What do you think?

Hi Thomas,

I assume the question as to whether or not my patches are going to get
accepted is more to Len.

I did ping Len and Rafael a couple of weeks ago with the same
question.  Rafael said that he would try to review them soon.  I would
like Rafael to review them and provide any feedback if necessary as he
did quite a lot of optimization and a bug fix or two on my original
series - in which this was the last patch in the series and never
ended up being pulled in - a year ago.

Hopefully we will hear from Len and Rafael soon and you can follow on
as necessary as you do seem to have found some valid issues.  If it
would help, or you need me to, I can re-post my series with some
changes incorporated.

Thanks,
 Myron
>
> Thanks,
>
>    Thomas
>
> ----
> ACPI APEI: Adjust GAR checking code to what exists out there
>
> Comparing different Generic Adress Register definitions of
> different vendors it came out that bit width (at least in APEI
> tables) is sometimes wrong or used different compared to older
> ACPI BIOS definitions (e.g. older FACP tables).
> It looks like Windows ignores the bit width field in
> latest implementations. Either in APEI table parts only
> (I'd say more likely) or in other ACPI parts as well.
>
> Worst case is that an address value to be read from a GAR structure
> can have a 8 bit width definition resulting in:
> ERST: Can not request iomem region <0x              3f-0x              3f>
> while the access width is correct:
> [1B0h 0432  1]                       Action : 0D (Get Error Address Range)
> [1B4h 0436 12]              Register Region : <Generic Address Structure>
> [1B4h 0436  1]                     Space ID : 00 (SystemMemory)
> [1B5h 0437  1]                    Bit Width : 08
> [1B6h 0438  1]                   Bit Offset : 00
> [1B7h 0439  1]         Encoded Access Width : 04 (QWord Access:64)
> [1B8h 0440  8]                      Address : 000000007F8A8032
>
>
> -> Ignore bit width field in APEI GAR checking code.
> Correct bit width value if needed (derived from access width)
> in the GAR structure, so that generic acpi read/write functions
> can still be used.
>
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/acpi/apei/apei-base.c     |   63 +++++++++++++++++++++++++++++-------
>  drivers/acpi/apei/apei-internal.h |    2 +
>  2 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index 6154036..d05f084 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -520,25 +520,53 @@ void apei_resources_release(struct apei_resources *resources)
>  }
>  EXPORT_SYMBOL_GPL(apei_resources_release);
>
> -static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
> +int apei_check_gar(struct acpi_generic_address *reg,
> +                  const struct acpi_generic_address *orig, u64 *paddr)
>  {
> -       u32 width, space_id;
> +       int bit_width, space_id, acc_width, acc_width_bits;
>
> -       width = reg->bit_width;
> +       memcpy(reg, orig, sizeof(struct acpi_generic_address));
> +       bit_width = reg->bit_width;
>        space_id = reg->space_id;
> +       acc_width = reg->access_width;
> +
>        /* Handle possible alignment issues */
>        memcpy(paddr, &reg->address, sizeof(*paddr));
>        if (!*paddr) {
>                pr_warning(FW_BUG APEI_PFX
>                           "Invalid physical address in GAR [0x%llx/%u/%u]\n",
> -                          *paddr, width, space_id);
> +                          *paddr, bit_width, space_id);
>                return -EINVAL;
>        }
>
> -       if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> +       switch (acc_width) {
> +       case 1:
> +               acc_width_bits = 8;
> +               break;
> +       case 2:
> +               acc_width_bits = 16;
> +               break;
> +       case 3:
> +               acc_width_bits = 32;
> +               break;
> +       case 4:
> +               acc_width_bits = 64;
> +               break;
> +       case 0:
> +               /* Never seen this, acc_width should always be correct */
> +               if ((bit_width == 8) || (bit_width == 16) ||
> +                   (bit_width == 32) || (bit_width == 64)) {
> +                       pr_warning(FW_BUG APEI_PFX
> +                                  "Incorrect acc width, using bit width"
> +                                  "[%d]\n", bit_width);
> +                       acc_width = bit_width / 8;
> +                       acc_width_bits = bit_width;
> +                       break;
> +               }
> +       default:
>                pr_warning(FW_BUG APEI_PFX
> -                          "Invalid bit width in GAR [0x%llx/%u/%u]\n",
> -                          *paddr, width, space_id);
> +                          "Invalid access width in GAR [0x%llx/%u/%u]\n",
> +                          *paddr, bit_width, space_id);
>                return -EINVAL;
>        }
>
> @@ -546,19 +574,28 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
>            space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
>                pr_warning(FW_BUG APEI_PFX
>                           "Invalid address space type in GAR [0x%llx/%u/%u]\n",
> -                          *paddr, width, space_id);
> +                          *paddr, bit_width, space_id);
>                return -EINVAL;
>        }
>
> +       /* bit width is not much worth in APEI GAR structures */
> +       if (acc_width_bits != bit_width) {
> +               pr_debug(FW_INFO APEI_PFX
> +                        "Correcting bit width value 0x%x -> 0x%x\n",
> +                        reg->bit_width, acc_width_bits);
> +               reg->bit_width = acc_width_bits;
> +       }
>        return 0;
>  }
> +EXPORT_SYMBOL_GPL(apei_check_gar);
>
>  static int collect_res_callback(struct apei_exec_context *ctx,
>                                struct acpi_whea_header *entry,
>                                void *data)
>  {
>        struct apei_resources *resources = data;
> -       struct acpi_generic_address *reg = &entry->register_region;
> +       struct acpi_generic_address reg;
> +       const struct acpi_generic_address *orig = &entry->register_region;
>        u8 ins = entry->instruction;
>        u64 paddr;
>        int rc;
> @@ -566,17 +603,17 @@ static int collect_res_callback(struct apei_exec_context *ctx,
>        if (!(ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER))
>                return 0;
>
> -       rc = apei_check_gar(reg, &paddr);
> +       rc = apei_check_gar(&reg, orig, &paddr);
>        if (rc)
>                return rc;
>
> -       switch (reg->space_id) {
> +       switch (reg.space_id) {
>        case ACPI_ADR_SPACE_SYSTEM_MEMORY:
>                return apei_res_add(&resources->iomem, paddr,
> -                                   reg->bit_width / 8);
> +                                   reg.bit_width / 8);
>        case ACPI_ADR_SPACE_SYSTEM_IO:
>                return apei_res_add(&resources->ioport, paddr,
> -                                   reg->bit_width / 8);
> +                                   reg.bit_width / 8);
>        default:
>                return -EINVAL;
>        }
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index f57050e..63d43d8 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -82,6 +82,8 @@ int apei_exec_noop(struct apei_exec_context *ctx,
>                   struct acpi_whea_header *entry);
>  int apei_exec_pre_map_gars(struct apei_exec_context *ctx);
>  int apei_exec_post_unmap_gars(struct apei_exec_context *ctx);
> +int apei_check_gar(struct acpi_generic_address *reg,
> +                  const struct acpi_generic_address *orig, u64 *paddr);
>
>  struct apei_resources {
>        struct list_head iomem;
> --
> 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

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

[Rafael J. Wysocki]

On Friday, October 28, 2011, Thomas Renninger 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.
> 
> There is another problem. Would be great to get some opinions/feedback
> on it already:
> APEI GAR entries seem to have invalid bit_width entries on some platforms.
> After looking at several tables, I expect that with APEI tables access width
> (in bytes) should get used instead, Windows seem to ignore bit width fields,
> at least for APEI tables.

There are a couple of things we could do.  First, ignore the widths as Windows
does (but that would make the code a bit fragile).  Second, replace the invalid
widths with something sane when (or right after) loading the APEI tables.

Is there any automatic check we can use to detect if the BIOS-provided widths
are invalid?

> I have patches but I need to know whether your patches are accepted.
> 
> There also is a problem with modifying GAR bit_width field to be able to
> pass it to the generic acpica functions (what your patches are doing).

What patches are you referring to, I don't see anything like this in [1/2]
and [2/2]?

> The problem is that the structures are located in reserved BIOS areas and
> they should be const and not get modified.

But we can cache them, can't we?

> I have 2 solutions for this:
>   1) Make apei_check_gar() pass 2 struct acpi_generic_address:
>      int apei_check_gar(struct acpi_generic_address *reg,
>                     const struct acpi_generic_address *orig, u64 *paddr)
>      orig -> is the one located in reserved mem area, mapped to virtual addr space
>      reg  -> will be a copy of it, possibly with bit_width adjusted which then
>             can be passed to acpi_memory_read/write and friends.
>   2) Allocate memory and copy whole APEI tables
> 
> 1. Should have the advantage of less memory waste
> 2. Should have the advantage of a bit nicer code (kalloc and memcpy per table) and
>    if more things like this happen, tables could get adjusted easily.
>    It also has the advantage that GAR structures do not need to get double
>    checked and eventually adjusted at runtime again.
> 
> Below is the first patch which goes for 1. More patches may be needed, but I
> as said, I need to know whether your patches get accepted.
> What do you think?

I'd go for 2.  Really, it's not like we have a shortage of memory on the
affected systems and it's so much simpler than the alternative that I'm
not sure if the alternative makes sense at all.

Thanks,
Rafael

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

[Thomas Renninger]

On Thursday, September 29, 2011 11:59:08 PM Myron Stowe wrote:
> Late last year I submitted a patch series that re-factored some existing
> work that Huang Ying introduced adding support for accessing ACPI
> generic registers backed by Memory Mapped I/O (MMIO) while within
> interrupt context:
>   Huang Ying's commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262,
>   My series: http://marc.info/?l=linux-acpi&m=128769263327206&w=2.

Ying: What is your opinion about this patchset?

One major change seem to be to use a mutex instead of a spinlock
which looks like a fix as the pre-mapping should never happen in
irq context.

I expect the rest is more or less the same, but double checking
by someone who is more involved in these code paths would be
great.

Thanks,

     Thomas

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

[Huang Ying]

On Fri, 2011-11-04 at 00:31 +0800, Thomas Renninger wrote:
> On Thursday, September 29, 2011 11:59:08 PM Myron Stowe wrote:
> > Late last year I submitted a patch series that re-factored some existing
> > work that Huang Ying introduced adding support for accessing ACPI
> > generic registers backed by Memory Mapped I/O (MMIO) while within
> > interrupt context:
> >   Huang Ying's commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262,
> >   My series: http://marc.info/?l=linux-acpi&m=128769263327206&w=2.
> 
> Ying: What is your opinion about this patchset?

I am OK with the patchset.  We just need some testing.

> One major change seem to be to use a mutex instead of a spinlock
> which looks like a fix as the pre-mapping should never happen in
> irq context.

Sorry, where is the mutex?

> I expect the rest is more or less the same, but double checking
> by someone who is more involved in these code paths would be
> great.

Best Regards,
Huang Ying

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

[Thomas Renninger]

On Friday 04 November 2011 01:56:04 Huang Ying wrote:
> On Fri, 2011-11-04 at 00:31 +0800, Thomas Renninger wrote:
> > On Thursday, September 29, 2011 11:59:08 PM Myron Stowe wrote:
> > > Late last year I submitted a patch series that re-factored some existing
> > > work that Huang Ying introduced adding support for accessing ACPI
> > > generic registers backed by Memory Mapped I/O (MMIO) while within
> > > interrupt context:
> > >   Huang Ying's commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262,
> > >   My series: http://marc.info/?l=linux-acpi&m=128769263327206&w=2.
> > 
> > Ying: What is your opinion about this patchset?
> 
> I am OK with the patchset.
Great, thanks.
> We just need some testing.
Hm, that's what release candidates (-rcX) are for.

> > One major change seem to be to use a mutex instead of a spinlock
> > which looks like a fix as the pre-mapping should never happen in
> > irq context.
> 
> Sorry, where is the mutex?
you used a spinlock in atomicio.c:
static DEFINE_SPINLOCK(acpi_iomaps_lock);
in osl.c a mutex is used:
static DEFINE_MUTEX(acpi_ioremap_lock);

> > I expect the rest is more or less the same, but double checking
> > by someone who is more involved in these code paths would be
> > great.

   Thomas

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

[Huang Ying]

On Fri, 2011-11-04 at 10:24 +0800, Thomas Renninger wrote:
> On Friday 04 November 2011 01:56:04 Huang Ying wrote:
> > On Fri, 2011-11-04 at 00:31 +0800, Thomas Renninger wrote:
> > > On Thursday, September 29, 2011 11:59:08 PM Myron Stowe wrote:
> > > > Late last year I submitted a patch series that re-factored some existing
> > > > work that Huang Ying introduced adding support for accessing ACPI
> > > > generic registers backed by Memory Mapped I/O (MMIO) while within
> > > > interrupt context:
> > > >   Huang Ying's commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262,
> > > >   My series: http://marc.info/?l=linux-acpi&m=128769263327206&w=2.
> > > 
> > > Ying: What is your opinion about this patchset?
> > 
> > I am OK with the patchset.
> Great, thanks.
> > We just need some testing.
> Hm, that's what release candidates (-rcX) are for.
> 
> > > One major change seem to be to use a mutex instead of a spinlock
> > > which looks like a fix as the pre-mapping should never happen in
> > > irq context.
> > 
> > Sorry, where is the mutex?
> you used a spinlock in atomicio.c:
> static DEFINE_SPINLOCK(acpi_iomaps_lock);
> in osl.c a mutex is used:
> static DEFINE_MUTEX(acpi_ioremap_lock);

Thanks.  I think mutex is OK here because it will not be used in IRQ/NMI
handler.

Best Regards,
Huang Ying