[PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Myron Stowe]

ACPI's system event related IRQ handing accesses specific fixed
hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
which are declared in the FADT.  If these registers are backed by
MMIO, as opposed to I/O port space, accessing them within interrupt
context will incur a panic since acpi_read() and acpi_write() end up
calling ioremap(), which may block to allocate memory - BZ 18012.

This patch series creates a list for maintaining ACPI MMIO remappings
and augments the corresponding mapping and unmapping interface
routines (acpi_os_map_memory() and acpi_os_unmap_memory()) to
dynamically add to, and delete from, the list as necessary.  In
addition to the list, interfaces for mapping and unmapping ACPI
registers, that are backed by MMIO, are added.  Together, they enable
accesses of such registers from within interrupt context.

Huang Ying introduced similar functionality specific to ACPI Platform
Error Interfaces in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262.
Much of this series re-factors Ying's commit - patches 5/7 and
6/7 [*] are practically verbatim copies - providing equivalent
functionality but in a more generalized manner allowing usage in
non-specific contexts.


[*]  The page based coalescing optimization of patch 6/7 is definitely
insightful but I question if the additional complexity, and potential
maintenance, outweigh its value.  I would be interested in others
opinions on this.

There is a behavioral difference that the optimization introduces
although I do not believe it matters in this instance:
  Consider two ioremap() requests occurring that specify the same
  physical address.  Prior to this optimization, two separate ioremap()
  requests would occur and two distinct virtual addresses would be
  returned (multiple calls to ioremap() with the same physical address
  do not return the same virtual address).  With the optimization in
  place, the two ioremap() requests will end up returning the same
  virtual address.


Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18012


---

Myron Stowe (7):
      ACPI: Fix ioremap size for MMIO reads and writes
      ACPI: Maintain a list of ACPI memory mapped I/O remappings
      ACPI: Add interfaces for ioremapping/iounmapping ACPI registers
      ACPI: Pre-map 'system event' related register blocks
      ACPI: Convert simple locking to RCU based locking
      ACPI: Page based coalescing of I/O remappings optimization
      ACPI: Re-factor and remove ./drivers/acpi/atomicio.[ch]


 drivers/acpi/Makefile         |    1 
 drivers/acpi/apei/apei-base.c |   11 +
 drivers/acpi/apei/ghes.c      |    9 -
 drivers/acpi/atomicio.c       |  361 -----------------------------------------
 drivers/acpi/osl.c            |  276 ++++++++++++++++++++++++++-----
 include/acpi/atomicio.h       |   10 -
 include/linux/acpi.h          |    3 
 7 files changed, 242 insertions(+), 429 deletions(-)
 delete mode 100644 drivers/acpi/atomicio.c
 delete mode 100644 include/acpi/atomicio.h

[PATCH 1/7] ACPI: Fix ioremap size for MMIO reads and writes

[Myron Stowe]

The size used for I/O remapping MMIO read and write accesses has not
accounted for the basis of ACPI's Generic Address Structure (GAS)
'Register Bit Width' field which is bits, not bytes.  This patch
adjusts the ioremap() 'size' argument accordingly.

ACPI "Generic Register" reference:
  ACPI Specification, Revision 4.0, Section 5.2.3.1, "Generic Address
  Structure".


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

 drivers/acpi/osl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 65b25a3..58842fb 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -496,7 +496,7 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u32 * value, u32 width)
 	u32 dummy;
 	void __iomem *virt_addr;
 
-	virt_addr = ioremap(phys_addr, width);
+	virt_addr = ioremap(phys_addr, width / 8);
 	if (!value)
 		value = &dummy;
 
@@ -524,7 +524,7 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
 {
 	void __iomem *virt_addr;
 
-	virt_addr = ioremap(phys_addr, width);
+	virt_addr = ioremap(phys_addr, width / 8);
 
 	switch (width) {
 	case 8:

[PATCH 2/7] ACPI: Maintain a list of ACPI memory mapped I/O remappings

[Myron Stowe]

For memory mapped I/O (MMIO) remappings, add a list to maintain the
remappings and augment the corresponding mapping and unmapping interface
routines (acpi_os_map_memory() and acpi_os_unmap_memory()) to
dynamically add to, and delete from, the list.

The current ACPI I/O accessing methods - acpi_read() and acpi_write() -
end up calling ioremap() when accessing MMIO.  This prevents use of these
methods within interrupt context (IRQ and/or NMI), since ioremap() may
block to allocate memory.  Maintaining a list of MMIO remappings enables
accesses to such areas from within interrupt context provided they have
been pre-mapped.


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

 drivers/acpi/osl.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 113 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 58842fb..bd72129 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -95,6 +95,20 @@ struct acpi_res_list {
 static LIST_HEAD(resource_list_head);
 static DEFINE_SPINLOCK(acpi_res_lock);
 
+/*
+ * This list of permanent mappings is for memory that may be accessed from
+ * interrupt context, where we can't do the ioremap().
+ */
+struct acpi_ioremap {
+	struct list_head list;
+	void __iomem *virt;
+	acpi_physical_address phys;
+	acpi_size size;
+};
+
+static LIST_HEAD(acpi_ioremaps);
+static DEFINE_SPINLOCK(acpi_ioremap_lock);
+
 #define	OSI_STRING_LENGTH_MAX 64	/* arbitrary */
 static char osi_additional_string[OSI_STRING_LENGTH_MAX];
 
@@ -260,29 +274,95 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 	}
 }
 
+/* Must be called with 'acpi_ioremap_lock' lock held. */
+static void __iomem *
+acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
+{
+	struct acpi_ioremap *map;
+
+	list_for_each_entry(map, &acpi_ioremaps, list)
+		if (map->phys <= phys &&
+		    phys + size <= map->phys + map->size)
+			return map->virt + (phys - map->phys);
+
+	return NULL;
+}
+
+/* Must be called with 'acpi_ioremap_lock' lock held. */
+static struct acpi_ioremap *
+acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
+{
+	struct acpi_ioremap *map;
+
+	list_for_each_entry(map, &acpi_ioremaps, list)
+		if (map->virt == virt && map->size == size)
+			return map;
+
+	return NULL;
+}
+
 void __iomem *__init_refok
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
+	struct acpi_ioremap *map;
+	unsigned long flags;
+	void __iomem *virt;
+
 	if (phys > ULONG_MAX) {
 		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
 		return NULL;
 	}
-	if (acpi_gbl_permanent_mmap)
-		/*
-		* ioremap checks to ensure this is in reserved space
-		*/
-		return ioremap((unsigned long)phys, size);
-	else
+
+	if (!acpi_gbl_permanent_mmap)
 		return __acpi_map_table((unsigned long)phys, size);
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return NULL;
+
+	virt = ioremap(phys, size);
+	if (!virt) {
+		kfree(map);
+		return NULL;
+	}
+
+	INIT_LIST_HEAD(&map->list);
+	map->virt = virt;
+	map->phys = phys;
+	map->size = size;
+
+	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	list_add_tail(&map->list, &acpi_ioremaps);
+	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+
+	return virt;
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
 void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 {
-	if (acpi_gbl_permanent_mmap)
-		iounmap(virt);
-	else
+	struct acpi_ioremap *map;
+	unsigned long flags;
+
+	if (!acpi_gbl_permanent_mmap) {
 		__acpi_unmap_table(virt, size);
+		return;
+	}
+
+	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	map = acpi_map_lookup_virt(virt, size);
+	if (!map) {
+		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+		printk(KERN_ERR PREFIX "%s: bad address %p\n", __func__, virt);
+		dump_stack();
+		return;
+	}
+
+	list_del(&map->list);
+	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+
+	iounmap(map->virt);
+	kfree(map);
 }
 EXPORT_SYMBOL_GPL(acpi_os_unmap_memory);
 
@@ -495,8 +575,16 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u32 * value, u32 width)
 {
 	u32 dummy;
 	void __iomem *virt_addr;
-
-	virt_addr = ioremap(phys_addr, width / 8);
+	int size = width / 8, unmap = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
+	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+	if (!virt_addr) {
+		virt_addr = ioremap(phys_addr, size);
+		unmap = 1;
+	}
 	if (!value)
 		value = &dummy;
 
@@ -514,7 +602,8 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u32 * value, u32 width)
 		BUG();
 	}
 
-	iounmap(virt_addr);
+	if (unmap)
+		iounmap(virt_addr);
 
 	return AE_OK;
 }
@@ -523,8 +612,16 @@ acpi_status
 acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
 {
 	void __iomem *virt_addr;
-
-	virt_addr = ioremap(phys_addr, width / 8);
+	int size = width / 8, unmap = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
+	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+	if (!virt_addr) {
+		virt_addr = ioremap(phys_addr, size);
+		unmap = 1;
+	}
 
 	switch (width) {
 	case 8:
@@ -540,7 +637,8 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
 		BUG();
 	}
 
-	iounmap(virt_addr);
+	if (unmap)
+		iounmap(virt_addr);
 
 	return AE_OK;
 }

[PATCH 3/7] ACPI: Add interfaces for ioremapping/iounmapping ACPI registers

[Myron Stowe]

Add remapping and unmapping interfaces for ACPI registers that are
backed by memory mapped I/O (MMIO).  These interfaces, along with
the MMIO remapping list, enable accesses of such registers from within
interrupt context.

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@hp.com>
---

 drivers/acpi/osl.c   |   38 ++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h |    3 +++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index bd72129..fc6c5d2 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -372,6 +372,44 @@ void __init early_acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		__acpi_unmap_table(virt, size);
 }
 
+int acpi_os_map_generic_address(struct acpi_generic_address *addr)
+{
+	void __iomem *virt;
+
+	if (addr->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		return 0;
+
+	if (!addr->address || !addr->bit_width)
+		return -EINVAL;
+
+	virt = acpi_os_map_memory(addr->address, addr->bit_width / 8);
+	if (!virt)
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_os_map_generic_address);
+
+void acpi_os_unmap_generic_address(struct acpi_generic_address *addr)
+{
+	void __iomem *virt;
+	unsigned long flags;
+	acpi_size size = addr->bit_width / 8;
+
+	if (addr->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		return;
+
+	if (!addr->address || !addr->bit_width)
+		return;
+
+	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	virt = acpi_map_vaddr_lookup(addr->address, size);
+	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+
+	acpi_os_unmap_memory(virt, size);
+}
+EXPORT_SYMBOL_GPL(acpi_os_unmap_generic_address);
+
 #ifdef ACPI_FUTURE_USAGE
 acpi_status
 acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c227757..7774e6d 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -308,6 +308,9 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 					     u32 *mask, u32 req);
 extern void acpi_early_init(void);
 
+int acpi_os_map_generic_address(struct acpi_generic_address *addr);
+void acpi_os_unmap_generic_address(struct acpi_generic_address *addr);
+
 #else	/* !CONFIG_ACPI */
 
 #define acpi_disabled 1

[PATCH 4/7] ACPI: Pre-map 'system event' related register blocks

[Myron Stowe]

During ACPI initialization, pre-map fixed hardware registers that are
accessed during ACPI's 'system event' related IRQ handing.

ACPI's 'system event' handing accesses specific fixed hardware
registers; namely PM1a event, PM1b event, GPE0, and GPE1 register
blocks which are declared within the FADT.  If these registers are
backed by MMIO, as opposed to I/O port space, accessing them within
interrupt context will cause a panic as acpi_os_read_memory()
depends on ioremap() in such cases - BZ 18012.

By utilizing the functionality provided in the previous two patches -
ACPI: Maintain a list of ACPI memory mapped I/O remappings, and, ACPI:
Add interfaces for ioremapping/iounmapping ACPI registers - accesses
to ACPI MMIO areas will now be safe from within interrupt contexts (IRQ
and/or NMI) provided the area was pre-mapped.  This solves BZ 18012.

ACPI "System Event" reference(s):
  ACPI Specification, Revision 4.0, Section 3 "ACPI Overview",
  3.8 "System Events", 5.6 "ACPI Event Programming Model".

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18012


Reported-by: <bjorn.helgaas@hp.com>
Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/acpi/osl.c |   71 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index fc6c5d2..c63d4cb 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -199,36 +199,6 @@ static int __init acpi_reserve_resources(void)
 }
 device_initcall(acpi_reserve_resources);
 
-acpi_status __init acpi_os_initialize(void)
-{
-	return AE_OK;
-}
-
-acpi_status acpi_os_initialize1(void)
-{
-	kacpid_wq = create_workqueue("kacpid");
-	kacpi_notify_wq = create_workqueue("kacpi_notify");
-	kacpi_hotplug_wq = create_workqueue("kacpi_hotplug");
-	BUG_ON(!kacpid_wq);
-	BUG_ON(!kacpi_notify_wq);
-	BUG_ON(!kacpi_hotplug_wq);
-	return AE_OK;
-}
-
-acpi_status acpi_os_terminate(void)
-{
-	if (acpi_irq_handler) {
-		acpi_os_remove_interrupt_handler(acpi_irq_irq,
-						 acpi_irq_handler);
-	}
-
-	destroy_workqueue(kacpid_wq);
-	destroy_workqueue(kacpi_notify_wq);
-	destroy_workqueue(kacpi_hotplug_wq);
-
-	return AE_OK;
-}
-
 void acpi_os_printf(const char *fmt, ...)
 {
 	va_list args;
@@ -1598,5 +1568,44 @@ acpi_os_validate_address (
 	}
 	return AE_OK;
 }
-
 #endif
+
+acpi_status __init acpi_os_initialize(void)
+{
+	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
+	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
+
+	return AE_OK;
+}
+
+acpi_status acpi_os_initialize1(void)
+{
+	kacpid_wq = create_workqueue("kacpid");
+	kacpi_notify_wq = create_workqueue("kacpi_notify");
+	kacpi_hotplug_wq = create_workqueue("kacpi_hotplug");
+	BUG_ON(!kacpid_wq);
+	BUG_ON(!kacpi_notify_wq);
+	BUG_ON(!kacpi_hotplug_wq);
+	return AE_OK;
+}
+
+acpi_status acpi_os_terminate(void)
+{
+	if (acpi_irq_handler) {
+		acpi_os_remove_interrupt_handler(acpi_irq_irq,
+						 acpi_irq_handler);
+	}
+
+	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
+	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
+	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+
+	destroy_workqueue(kacpid_wq);
+	destroy_workqueue(kacpi_notify_wq);
+	destroy_workqueue(kacpi_hotplug_wq);
+
+	return AE_OK;
+}

[PATCH 5/7] ACPI: Convert simple locking to RCU based locking

[Myron Stowe]

Convert the simple locking introduced earlier for the ACPI MMIO
remappings list to an RCU based locking scheme.


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

 drivers/acpi/osl.c |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index c63d4cb..3282689 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -244,13 +244,13 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 	}
 }
 
-/* Must be called with 'acpi_ioremap_lock' lock held. */
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
 static void __iomem *
 acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry(map, &acpi_ioremaps, list)
+	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
 			return map->virt + (phys - map->phys);
@@ -258,13 +258,13 @@ acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
 	return NULL;
 }
 
-/* Must be called with 'acpi_ioremap_lock' lock held. */
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
 static struct acpi_ioremap *
 acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry(map, &acpi_ioremaps, list)
+	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->virt == virt && map->size == size)
 			return map;
 
@@ -302,7 +302,7 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 	map->size = size;
 
 	spin_lock_irqsave(&acpi_ioremap_lock, flags);
-	list_add_tail(&map->list, &acpi_ioremaps);
+	list_add_tail_rcu(&map->list, &acpi_ioremaps);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
 	return virt;
@@ -328,9 +328,10 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		return;
 	}
 
-	list_del(&map->list);
+	list_del_rcu(&map->list);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
+	synchronize_rcu();
 	iounmap(map->virt);
 	kfree(map);
 }
@@ -584,11 +585,10 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u32 * value, u32 width)
 	u32 dummy;
 	void __iomem *virt_addr;
 	int size = width / 8, unmap = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	rcu_read_lock();
 	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
-	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+	rcu_read_unlock();
 	if (!virt_addr) {
 		virt_addr = ioremap(phys_addr, size);
 		unmap = 1;
@@ -621,11 +621,10 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
 {
 	void __iomem *virt_addr;
 	int size = width / 8, unmap = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	rcu_read_lock();
 	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
-	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+	rcu_read_unlock();
 	if (!virt_addr) {
 		virt_addr = ioremap(phys_addr, size);
 		unmap = 1;

[PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Myron Stowe]

This patch optimizes ACPI MMIO remappings by keeping track of the
remappings on a PAGE_SIZE granularity.

When an ioremap() occurs, the underlying infrastructure works on a 'page'
based granularity.  As such, an ioremap() request for 1 byte for example,
will end up mapping in an entire (PAGE_SIZE) page.  Huang Ying took
advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by
checking if subsequent ioremap() requests reside within any of the list's
existing remappings still in place, and if so, incrementing a reference
count on the existing mapping as opposed to performing another ioremap().


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

 drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3282689..885e222 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -104,6 +104,7 @@ struct acpi_ioremap {
 	void __iomem *virt;
 	acpi_physical_address phys;
 	acpi_size size;
+	struct kref ref;
 };
 
 static LIST_HEAD(acpi_ioremaps);
@@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 }
 
 /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
-static void __iomem *
-acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
+static struct acpi_ioremap *
+acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
-			return map->virt + (phys - map->phys);
+			return map;
+
+	return NULL;
+}
+
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
+static void __iomem *
+acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
+{
+	struct acpi_ioremap *map;
+
+	map = acpi_map_lookup(phys, size);
+	if (map)
+		return map->virt + (phys - map->phys);
 
 	return NULL;
 }
@@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
-		if (map->virt == virt && map->size == size)
+		if (map->virt <= virt &&
+		    virt + size <= map->virt + map->size)
 			return map;
 
 	return NULL;
@@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 void __iomem *__init_refok
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	struct acpi_ioremap *map;
-	unsigned long flags;
+	struct acpi_ioremap *map, *tmp_map;
+	unsigned long flags, pg_sz;
 	void __iomem *virt;
+	phys_addr_t pg_off;
 
 	if (phys > ULONG_MAX) {
 		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
@@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 	if (!map)
 		return NULL;
 
-	virt = ioremap(phys, size);
+	pg_off = round_down(phys, PAGE_SIZE);
+	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
+	virt = ioremap(pg_off, pg_sz);
 	if (!virt) {
 		kfree(map);
 		return NULL;
@@ -298,21 +316,40 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 
 	INIT_LIST_HEAD(&map->list);
 	map->virt = virt;
-	map->phys = phys;
-	map->size = size;
+	map->phys = pg_off;
+	map->size = pg_sz;
+	kref_init(&map->ref);
 
 	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	/* Check if page has already been mapped. */
+	tmp_map = acpi_map_lookup(phys, size);
+	if (tmp_map) {
+		kref_get(&tmp_map->ref);
+		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+		iounmap(map->virt);
+		kfree(map);
+		return tmp_map->virt + (phys - tmp_map->phys);
+	}
 	list_add_tail_rcu(&map->list, &acpi_ioremaps);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
-	return virt;
+	return map->virt + (phys - map->phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
+static void acpi_kref_del_iomap(struct kref *ref)
+{
+	struct acpi_ioremap *map;
+
+	map = container_of(ref, struct acpi_ioremap, ref);
+	list_del_rcu(&map->list);
+}
+
 void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	unsigned long flags;
+	int del;
 
 	if (!acpi_gbl_permanent_mmap) {
 		__acpi_unmap_table(virt, size);
@@ -328,9 +365,12 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		return;
 	}
 
-	list_del_rcu(&map->list);
+	del = kref_put(&map->ref, acpi_kref_del_iomap);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
+	if (!del)
+		return;
+
 	synchronize_rcu();
 	iounmap(map->virt);
 	kfree(map);

Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Huang Ying]

Hi, Myron,

On Fri, 2010-10-22 at 04:24 +0800, Myron Stowe wrote:
> This patch optimizes ACPI MMIO remappings by keeping track of the
> remappings on a PAGE_SIZE granularity.
> 
> When an ioremap() occurs, the underlying infrastructure works on a 'page'
> based granularity.  As such, an ioremap() request for 1 byte for example,
> will end up mapping in an entire (PAGE_SIZE) page.  Huang Ying took
> advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by
> checking if subsequent ioremap() requests reside within any of the list's
> existing remappings still in place, and if so, incrementing a reference
> count on the existing mapping as opposed to performing another ioremap().
> 
> 
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> ---
> 
>  drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3282689..885e222 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -104,6 +104,7 @@ struct acpi_ioremap {
>  	void __iomem *virt;
>  	acpi_physical_address phys;
>  	acpi_size size;
> +	struct kref ref;
>  };
>  
>  static LIST_HEAD(acpi_ioremaps);
> @@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  }
>  
>  /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> -static void __iomem *
> -acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
> +static struct acpi_ioremap *
> +acpi_map_lookup(acpi_physical_address phys, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  
>  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
>  		if (map->phys <= phys &&
>  		    phys + size <= map->phys + map->size)
> -			return map->virt + (phys - map->phys);
> +			return map;
> +
> +	return NULL;
> +}
> +
> +/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> +static void __iomem *
> +acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
> +{
> +	struct acpi_ioremap *map;
> +
> +	map = acpi_map_lookup(phys, size);
> +	if (map)
> +		return map->virt + (phys - map->phys);
>  
>  	return NULL;
>  }
> @@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  	struct acpi_ioremap *map;
>  
>  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> -		if (map->virt == virt && map->size == size)
> +		if (map->virt <= virt &&
> +		    virt + size <= map->virt + map->size)
>  			return map;
>  
>  	return NULL;
> @@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  void __iomem *__init_refok
>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	struct acpi_ioremap *map;
> -	unsigned long flags;
> +	struct acpi_ioremap *map, *tmp_map;
> +	unsigned long flags, pg_sz;
>  	void __iomem *virt;
> +	phys_addr_t pg_off;
>  
>  	if (phys > ULONG_MAX) {
>  		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
> @@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  	if (!map)
>  		return NULL;
>  
> -	virt = ioremap(phys, size);
> +	pg_off = round_down(phys, PAGE_SIZE);
> +	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> +	virt = ioremap(pg_off, pg_sz);
>  	if (!virt) {
>  		kfree(map);
>  		return NULL;

Why not check the existing map (that is, acpi_map_lookup()) before the
ioremap()? In APEI tables, several GAS may be in same page, so we need
ioremap() for the first page only.

Best Regards,
Huang Ying

> @@ -298,21 +316,40 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  
>  	INIT_LIST_HEAD(&map->list);
>  	map->virt = virt;
> -	map->phys = phys;
> -	map->size = size;
> +	map->phys = pg_off;
> +	map->size = pg_sz;
> +	kref_init(&map->ref);
>  
>  	spin_lock_irqsave(&acpi_ioremap_lock, flags);
> +	/* Check if page has already been mapped. */
> +	tmp_map = acpi_map_lookup(phys, size);
> +	if (tmp_map) {
> +		kref_get(&tmp_map->ref);
> +		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
> +		iounmap(map->virt);
> +		kfree(map);
> +		return tmp_map->virt + (phys - tmp_map->phys);
> +	}
>  	list_add_tail_rcu(&map->list, &acpi_ioremaps);
>  	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
>  
> -	return virt;
> +	return map->virt + (phys - map->phys);
>  }
>  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
>  
> +static void acpi_kref_del_iomap(struct kref *ref)
> +{
> +	struct acpi_ioremap *map;
> +
> +	map = container_of(ref, struct acpi_ioremap, ref);
> +	list_del_rcu(&map->list);
> +}
> +
>  void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  	unsigned long flags;
> +	int del;
>  
>  	if (!acpi_gbl_permanent_mmap) {
>  		__acpi_unmap_table(virt, size);
> @@ -328,9 +365,12 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
>  		return;
>  	}
>  
> -	list_del_rcu(&map->list);
> +	del = kref_put(&map->ref, acpi_kref_del_iomap);
>  	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
>  
> +	if (!del)
> +		return;
> +
>  	synchronize_rcu();
>  	iounmap(map->virt);
>  	kfree(map);
> 

Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Myron Stowe]

On Fri, 2010-10-22 at 09:03 +0800, Huang Ying wrote:
> Hi, Myron,
> 
> On Fri, 2010-10-22 at 04:24 +0800, Myron Stowe wrote:
> > This patch optimizes ACPI MMIO remappings by keeping track of the
> > remappings on a PAGE_SIZE granularity.
> > 
> > When an ioremap() occurs, the underlying infrastructure works on a 'page'
> > based granularity.  As such, an ioremap() request for 1 byte for example,
> > will end up mapping in an entire (PAGE_SIZE) page.  Huang Ying took
> > advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by
> > checking if subsequent ioremap() requests reside within any of the list's
> > existing remappings still in place, and if so, incrementing a reference
> > count on the existing mapping as opposed to performing another ioremap().
> > 
> > 
> > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > ---
> > 
> >  drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
> >  1 files changed, 51 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 3282689..885e222 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -104,6 +104,7 @@ struct acpi_ioremap {
> >  	void __iomem *virt;
> >  	acpi_physical_address phys;
> >  	acpi_size size;
> > +	struct kref ref;
> >  };
> >  
> >  static LIST_HEAD(acpi_ioremaps);
> > @@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
> >  }
> >  
> >  /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> > -static void __iomem *
> > -acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
> > +static struct acpi_ioremap *
> > +acpi_map_lookup(acpi_physical_address phys, acpi_size size)
> >  {
> >  	struct acpi_ioremap *map;
> >  
> >  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> >  		if (map->phys <= phys &&
> >  		    phys + size <= map->phys + map->size)
> > -			return map->virt + (phys - map->phys);
> > +			return map;
> > +
> > +	return NULL;
> > +}
> > +
> > +/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> > +static void __iomem *
> > +acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
> > +{
> > +	struct acpi_ioremap *map;
> > +
> > +	map = acpi_map_lookup(phys, size);
> > +	if (map)
> > +		return map->virt + (phys - map->phys);
> >  
> >  	return NULL;
> >  }
> > @@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> >  	struct acpi_ioremap *map;
> >  
> >  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> > -		if (map->virt == virt && map->size == size)
> > +		if (map->virt <= virt &&
> > +		    virt + size <= map->virt + map->size)
> >  			return map;
> >  
> >  	return NULL;
> > @@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> >  void __iomem *__init_refok
> >  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >  {
> > -	struct acpi_ioremap *map;
> > -	unsigned long flags;
> > +	struct acpi_ioremap *map, *tmp_map;
> > +	unsigned long flags, pg_sz;
> >  	void __iomem *virt;
> > +	phys_addr_t pg_off;
> >  
> >  	if (phys > ULONG_MAX) {
> >  		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
> > @@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >  	if (!map)
> >  		return NULL;
> >  
> > -	virt = ioremap(phys, size);
> > +	pg_off = round_down(phys, PAGE_SIZE);
> > +	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > +	virt = ioremap(pg_off, pg_sz);
> >  	if (!virt) {
> >  		kfree(map);
> >  		return NULL;
> 
> Why not check the existing map (that is, acpi_map_lookup()) before the
> ioremap()? In APEI tables, several GAS may be in same page, so we need
> ioremap() for the first page only.

Yes, even in other instances, several registers (GAS) may reside within
the same page(s) of a previously, still active, ioremapping and thus
only one ioremapping needs to be in place (with the subsequent
mapping(s) that fall within the same page(s) of the existing, active,
remapping then only needing to increment its reference).


In the existing implementation - atomicio.c:acpi_pre_map() - there is a
check at the beginning of the routine to see if a new mapping fits
within the page(s) of a previous entry's page(s) that is still active:
__acpi_try_ioremap().  Then later in the same routine, the same
__acpi_try_ioremap() check has to be made again due to a potential mp
race between the initial check and the insertion of a potentially new
entry onto the list.

Since the second check *must* take place, due to the possible race, I
decided to skip the initial check in the re-factored implementation
knowing that such a situation would ultimately still be caught by it
(the second check) and handled correctly: a subsequent, overlapping,
mapping is caught by the "tmp_map = acpi_map_lookup()" below, the
reference to the initial, active, remapping incremented, and the
overlapping mapping is "backed out".

Do you see any issues with such an approach?

Thanks,

Myron
> 
> Best Regards,
> Huang Ying
> 
> > @@ -298,21 +316,40 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >  
> >  	INIT_LIST_HEAD(&map->list);
> >  	map->virt = virt;
> > -	map->phys = phys;
> > -	map->size = size;
> > +	map->phys = pg_off;
> > +	map->size = pg_sz;
> > +	kref_init(&map->ref);
> >  
> >  	spin_lock_irqsave(&acpi_ioremap_lock, flags);
> > +	/* Check if page has already been mapped. */
> > +	tmp_map = acpi_map_lookup(phys, size);
> > +	if (tmp_map) {
> > +		kref_get(&tmp_map->ref);
> > +		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
> > +		iounmap(map->virt);
> > +		kfree(map);
> > +		return tmp_map->virt + (phys - tmp_map->phys);
> > +	}
> >  	list_add_tail_rcu(&map->list, &acpi_ioremaps);
> >  	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
> >  
> > -	return virt;
> > +	return map->virt + (phys - map->phys);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> >  
> > +static void acpi_kref_del_iomap(struct kref *ref)
> > +{
> > +	struct acpi_ioremap *map;
> > +
> > +	map = container_of(ref, struct acpi_ioremap, ref);
> > +	list_del_rcu(&map->list);
> > +}
> > +
> >  void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
> >  {
> >  	struct acpi_ioremap *map;
> >  	unsigned long flags;
> > +	int del;
> >  
> >  	if (!acpi_gbl_permanent_mmap) {
> >  		__acpi_unmap_table(virt, size);
> > @@ -328,9 +365,12 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
> >  		return;
> >  	}
> >  
> > -	list_del_rcu(&map->list);
> > +	del = kref_put(&map->ref, acpi_kref_del_iomap);
> >  	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
> >  
> > +	if (!del)
> > +		return;
> > +
> >  	synchronize_rcu();
> >  	iounmap(map->virt);
> >  	kfree(map);
> > 
> 
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Huang Ying]

On Fri, 2010-10-22 at 11:23 +0800, Myron Stowe wrote:
> On Fri, 2010-10-22 at 09:03 +0800, Huang Ying wrote:
> > Hi, Myron,
> > 
> > On Fri, 2010-10-22 at 04:24 +0800, Myron Stowe wrote:
> > > This patch optimizes ACPI MMIO remappings by keeping track of the
> > > remappings on a PAGE_SIZE granularity.
> > > 
> > > When an ioremap() occurs, the underlying infrastructure works on a 'page'
> > > based granularity.  As such, an ioremap() request for 1 byte for example,
> > > will end up mapping in an entire (PAGE_SIZE) page.  Huang Ying took
> > > advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by
> > > checking if subsequent ioremap() requests reside within any of the list's
> > > existing remappings still in place, and if so, incrementing a reference
> > > count on the existing mapping as opposed to performing another ioremap().
> > > 
> > > 
> > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > ---
> > > 
> > >  drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
> > >  1 files changed, 51 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > index 3282689..885e222 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -104,6 +104,7 @@ struct acpi_ioremap {
> > >  	void __iomem *virt;
> > >  	acpi_physical_address phys;
> > >  	acpi_size size;
> > > +	struct kref ref;
> > >  };
> > >  
> > >  static LIST_HEAD(acpi_ioremaps);
> > > @@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
> > >  }
> > >  
> > >  /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> > > -static void __iomem *
> > > -acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
> > > +static struct acpi_ioremap *
> > > +acpi_map_lookup(acpi_physical_address phys, acpi_size size)
> > >  {
> > >  	struct acpi_ioremap *map;
> > >  
> > >  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> > >  		if (map->phys <= phys &&
> > >  		    phys + size <= map->phys + map->size)
> > > -			return map->virt + (phys - map->phys);
> > > +			return map;
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
> > > +static void __iomem *
> > > +acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
> > > +{
> > > +	struct acpi_ioremap *map;
> > > +
> > > +	map = acpi_map_lookup(phys, size);
> > > +	if (map)
> > > +		return map->virt + (phys - map->phys);
> > >  
> > >  	return NULL;
> > >  }
> > > @@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> > >  	struct acpi_ioremap *map;
> > >  
> > >  	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> > > -		if (map->virt == virt && map->size == size)
> > > +		if (map->virt <= virt &&
> > > +		    virt + size <= map->virt + map->size)
> > >  			return map;
> > >  
> > >  	return NULL;
> > > @@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> > >  void __iomem *__init_refok
> > >  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > >  {
> > > -	struct acpi_ioremap *map;
> > > -	unsigned long flags;
> > > +	struct acpi_ioremap *map, *tmp_map;
> > > +	unsigned long flags, pg_sz;
> > >  	void __iomem *virt;
> > > +	phys_addr_t pg_off;
> > >  
> > >  	if (phys > ULONG_MAX) {
> > >  		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
> > > @@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > >  	if (!map)
> > >  		return NULL;
> > >  
> > > -	virt = ioremap(phys, size);
> > > +	pg_off = round_down(phys, PAGE_SIZE);
> > > +	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > > +	virt = ioremap(pg_off, pg_sz);
> > >  	if (!virt) {
> > >  		kfree(map);
> > >  		return NULL;
> > 
> > Why not check the existing map (that is, acpi_map_lookup()) before the
> > ioremap()? In APEI tables, several GAS may be in same page, so we need
> > ioremap() for the first page only.
> 
> Yes, even in other instances, several registers (GAS) may reside within
> the same page(s) of a previously, still active, ioremapping and thus
> only one ioremapping needs to be in place (with the subsequent
> mapping(s) that fall within the same page(s) of the existing, active,
> remapping then only needing to increment its reference).
> 
> 
> In the existing implementation - atomicio.c:acpi_pre_map() - there is a
> check at the beginning of the routine to see if a new mapping fits
> within the page(s) of a previous entry's page(s) that is still active:
> __acpi_try_ioremap().  Then later in the same routine, the same
> __acpi_try_ioremap() check has to be made again due to a potential mp
> race between the initial check and the insertion of a potentially new
> entry onto the list.
> 
> Since the second check *must* take place,

No. If the first check succeeds, the second check is not necessary, it
is sufficient just to increase the reference count and return the mapped
virtual address. The first check will eliminate
kmalloc/ioremap/iounmap/kfree for physical address has been mapped by
previous calling.

>  due to the possible race, I
> decided to skip the initial check in the re-factored implementation
> knowing that such a situation would ultimately still be caught by it
> (the second check) and handled correctly: a subsequent, overlapping,
> mapping is caught by the "tmp_map = acpi_map_lookup()" below, the
> reference to the initial, active, remapping incremented, and the
> overlapping mapping is "backed out".
> 
> Do you see any issues with such an approach?

Best Regards,
Huang Ying

Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Myron Stowe]

On Fri, 2010-10-22 at 13:17 +0800, Huang Ying wrote:
> On Fri, 2010-10-22 at 11:23 +0800, Myron Stowe wrote:
> > On Fri, 2010-10-22 at 09:03 +0800, Huang Ying wrote:
> > > Hi, Myron,
> > > 
<snip>
> > > 
> > > Why not check the existing map (that is, acpi_map_lookup()) before the
> > > ioremap()? In APEI tables, several GAS may be in same page, so we need
> > > ioremap() for the first page only.
> > 
> > Yes, even in other instances, several registers (GAS) may reside within
> > the same page(s) of a previously, still active, ioremapping and thus
> > only one ioremapping needs to be in place (with the subsequent
> > mapping(s) that fall within the same page(s) of the existing, active,
> > remapping then only needing to increment its reference).
> > 
> > 
> > In the existing implementation - atomicio.c:acpi_pre_map() - there is a
> > check at the beginning of the routine to see if a new mapping fits
> > within the page(s) of a previous entry's page(s) that is still active:
> > __acpi_try_ioremap().  Then later in the same routine, the same
> > __acpi_try_ioremap() check has to be made again due to a potential mp
> > race between the initial check and the insertion of a potentially new
> > entry onto the list.
> > 
> > Since the second check *must* take place,
> 
> No. If the first check succeeds, the second check is not necessary, it
> is sufficient just to increase the reference count and return the mapped
> virtual address. The first check will eliminate
> kmalloc/ioremap/iounmap/kfree for physical address has been mapped by
> previous calling.

Hay Haung:

Let's take two scenarios and see if I can express myself better why I
believe that the second check must take place.  Let's also stick with
the existing atomicio.c:acpi_pre_map() implementation for now just to
keep things simpler.


Scenario 1:  Assume that an MMIO remapping is already in place - mapping
A.  At some later point in time another call is made to map in some
other MMIO area and this mapping - mapping B - resides within the same
page(s) of mapping A which is still active, and thus, still on the list.

In this scenario, the first __acpi_try_ioremap() check will detect that
mapping B resides within mapping A, increment the reference of mapping
A, and then acpi_pre_map() short circuits out.  Things are handled
correctly and the first check nicely short circuits out so the second
check was never even reached.

Now let's take another scenario, Scenario 2: Assume that an MMIO
remapping is occurring on some processor of a MP system to put mapping A
into place (so the the processor is processing code somewhere between
"pg_off = ..." and "kref_init()").  While the processor is in the
process of putting mapping A into place it becomes blocked for any of a
number of reasons.  Another processor now runs and puts a mapping into
place - mapping C - that is equal to or greater in size than mapping A
which also encompasses the same page(s) of mapping A.  So now, at this
point in time, mapping C is on the list but mapping A is not.

At some later point in time the processor running mapping A will
continue.  It acquires the spin_lock immediately following the
"kref_init()" and must now check again to see if some other processor
put a similar mapping into place.  In this scenario, exactly such has
happened - which is why I think you put this check in place to begin
with.  The MP race is properly detected by the second
__acpi_try_ioremap() check which increments the reference of mapping C
and mapping A is backed out (iounmap/kfree).

So with the existing atomicio.c:acpi_pre_map() implementation there are
two __acpi_try_ioremap() checks.  The first check handles situations
like Scenario 1 and the second check handles situations like Scenario 2.

It was situations like Scenario 2 that I was thinking of in my initial
response when I said that the second check *must* take place.  I agree,
as indicated above, in situations like Scenario 1 the second check is
never even reached.

Hopefully I have now expressed better why I believe the second check
*must* take place?


Now back to your original question - why did I not include the first
check in the re-factoring?

My thinking was along the lines of: "Yes, the first check is nice in
that it short circuits additional remappings that match existing
mappings.  Thinking further, consider that since the second check must
still occur due to situations like Scenario 2, and, since the second
check would not only catch situations like Scenario 2 but also
additionally catches any additional remappings matching existing ones in
place like Scenario 1 - why not just keep the code simpler/smaller and
not include the first check?"

I agree that such a tack does have trade offs.  I chose to keep the code
simpler/smaller at the expense of giving up the "short circuit" benefits
that the first check provides in situations like Scenario 1.  The code,
without the first check still handles situations like Scenario 1 - just
not as efficiently (i.e. it incurs the expense of
"kmalloc/ioremap/iounmap/kfree for physical address has been mapped by
previous calling" as you properly pointed out.

With this explanation of my actions do you see any issues with such an
approach? 


Thanks for questioning as it always tends to lead towards a better
solution in the end.

Take care,

Myron
> 
> >  due to the possible race, I
> > decided to skip the initial check in the re-factored implementation
> > knowing that such a situation would ultimately still be caught by it
> > (the second check) and handled correctly: a subsequent, overlapping,
> > mapping is caught by the "tmp_map = acpi_map_lookup()" below, the
> > reference to the initial, active, remapping incremented, and the
> > overlapping mapping is "backed out".
> > 
> > Do you see any issues with such an approach?
> 
> Best Regards,
> Huang Ying
> 
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Huang Ying]

Hi, Myron,

On Sat, 2010-10-23 at 00:27 +0800, Myron Stowe wrote:
> On Fri, 2010-10-22 at 13:17 +0800, Huang Ying wrote:
> > On Fri, 2010-10-22 at 11:23 +0800, Myron Stowe wrote:
> > > On Fri, 2010-10-22 at 09:03 +0800, Huang Ying wrote:
> > > > Hi, Myron,
> > > > 
> <snip>
> > > > 
> > > > Why not check the existing map (that is, acpi_map_lookup()) before the
> > > > ioremap()? In APEI tables, several GAS may be in same page, so we need
> > > > ioremap() for the first page only.
> > > 
> > > Yes, even in other instances, several registers (GAS) may reside within
> > > the same page(s) of a previously, still active, ioremapping and thus
> > > only one ioremapping needs to be in place (with the subsequent
> > > mapping(s) that fall within the same page(s) of the existing, active,
> > > remapping then only needing to increment its reference).
> > > 
> > > 
> > > In the existing implementation - atomicio.c:acpi_pre_map() - there is a
> > > check at the beginning of the routine to see if a new mapping fits
> > > within the page(s) of a previous entry's page(s) that is still active:
> > > __acpi_try_ioremap().  Then later in the same routine, the same
> > > __acpi_try_ioremap() check has to be made again due to a potential mp
> > > race between the initial check and the insertion of a potentially new
> > > entry onto the list.
> > > 
> > > Since the second check *must* take place,
> > 
> > No. If the first check succeeds, the second check is not necessary, it
> > is sufficient just to increase the reference count and return the mapped
> > virtual address. The first check will eliminate
> > kmalloc/ioremap/iounmap/kfree for physical address has been mapped by
> > previous calling.
> 
> Hay Haung:
> 
> Let's take two scenarios and see if I can express myself better why I
> believe that the second check must take place.  Let's also stick with
> the existing atomicio.c:acpi_pre_map() implementation for now just to
> keep things simpler.
> 
> 
> Scenario 1:  Assume that an MMIO remapping is already in place - mapping
> A.  At some later point in time another call is made to map in some
> other MMIO area and this mapping - mapping B - resides within the same
> page(s) of mapping A which is still active, and thus, still on the list.
> 
> In this scenario, the first __acpi_try_ioremap() check will detect that
> mapping B resides within mapping A, increment the reference of mapping
> A, and then acpi_pre_map() short circuits out.  Things are handled
> correctly and the first check nicely short circuits out so the second
> check was never even reached.
> 
> Now let's take another scenario, Scenario 2: Assume that an MMIO
> remapping is occurring on some processor of a MP system to put mapping A
> into place (so the the processor is processing code somewhere between
> "pg_off = ..." and "kref_init()").  While the processor is in the
> process of putting mapping A into place it becomes blocked for any of a
> number of reasons.  Another processor now runs and puts a mapping into
> place - mapping C - that is equal to or greater in size than mapping A
> which also encompasses the same page(s) of mapping A.  So now, at this
> point in time, mapping C is on the list but mapping A is not.
> 
> At some later point in time the processor running mapping A will
> continue.  It acquires the spin_lock immediately following the
> "kref_init()" and must now check again to see if some other processor
> put a similar mapping into place.  In this scenario, exactly such has
> happened - which is why I think you put this check in place to begin
> with.  The MP race is properly detected by the second
> __acpi_try_ioremap() check which increments the reference of mapping C
> and mapping A is backed out (iounmap/kfree).
> 
> So with the existing atomicio.c:acpi_pre_map() implementation there are
> two __acpi_try_ioremap() checks.  The first check handles situations
> like Scenario 1 and the second check handles situations like Scenario 2.
> 
> It was situations like Scenario 2 that I was thinking of in my initial
> response when I said that the second check *must* take place.  I agree,
> as indicated above, in situations like Scenario 1 the second check is
> never even reached.
> 
> Hopefully I have now expressed better why I believe the second check
> *must* take place?
> 
> 
> Now back to your original question - why did I not include the first
> check in the re-factoring?
> 
> My thinking was along the lines of: "Yes, the first check is nice in
> that it short circuits additional remappings that match existing
> mappings.  Thinking further, consider that since the second check must
> still occur due to situations like Scenario 2, and, since the second
> check would not only catch situations like Scenario 2 but also
> additionally catches any additional remappings matching existing ones in
> place like Scenario 1 - why not just keep the code simpler/smaller and
> not include the first check?"
> 
> I agree that such a tack does have trade offs.  I chose to keep the code
> simpler/smaller at the expense of giving up the "short circuit" benefits
> that the first check provides in situations like Scenario 1.  The code,
> without the first check still handles situations like Scenario 1 - just
> not as efficiently (i.e. it incurs the expense of
> "kmalloc/ioremap/iounmap/kfree for physical address has been mapped by
> previous calling" as you properly pointed out.
> 
> With this explanation of my actions do you see any issues with such an
> approach? 

Now, I understand your idea. But I still think it is better to keep the
first check. Because it has some benefit with the cost of just a few
lines of simple code. I think it should have no much harm to code
readability.

Best Regards,
Huang Ying

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

[Myron Stowe]

The functionality of 'atomicio.c' has been re-factored into
'./drivers/acpi/osl.c' which now provides equivalent functionality but
in a more generalized manner allowing usage in non-specific contexts.

Calls to the following 'atomicio.c' routines are mapped to calls within
'osl.c' and existing calls within the CA, based on the refactoring:
  acpi_pre_map_gar()     ->  acpi_os_ioremap_generic_address()
  acpi_post_unmap_gar()  ->  acpi_os_iounmap_generic_address()
  acpi_atomic_read()     ->  acpi_read()
  acpi_atomic_write()    ->  acpi_write()


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

 drivers/acpi/Makefile         |    1 
 drivers/acpi/apei/apei-base.c |   11 +
 drivers/acpi/apei/ghes.c      |    9 -
 drivers/acpi/atomicio.c       |  361 -----------------------------------------
 include/acpi/atomicio.h       |   10 -
 5 files changed, 9 insertions(+), 383 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 3d031d0..bf598aa 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 73fd0c7..cd362d5 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -40,7 +40,6 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
-#include <acpi/atomicio.h>
 
 #include "apei-internal.h"
 
@@ -70,7 +69,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 +115,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;
 }
@@ -242,7 +241,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;
 }
@@ -275,7 +274,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 385a605..afa7668 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -44,7 +44,6 @@
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <acpi/apei.h>
-#include <acpi/atomicio.h>
 #include <acpi/hed.h>
 #include <asm/mce.h>
 
@@ -102,7 +101,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 		return ERR_PTR(-ENOMEM);
 	ghes->generic = generic;
 	INIT_LIST_HEAD(&ghes->list);
-	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;
@@ -122,7 +121,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);
@@ -131,7 +130,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 {
@@ -183,7 +182,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 8f8bd73..0000000
--- a/drivers/acpi/atomicio.c
+++ /dev/null
@@ -1,361 +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 vaddr + (paddr - pg_off);
-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;
-	case 64:
-		*val = readq(addr);
-		break;
-	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;
-	case 64:
-		writeq(val, addr);
-		break;
-	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/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Shaohua Li]

On Fri, 2010-10-22 at 04:23 +0800, Myron Stowe wrote:
> ACPI's system event related IRQ handing accesses specific fixed
> hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
> which are declared in the FADT.  If these registers are backed by
> MMIO, as opposed to I/O port space, accessing them within interrupt
> context will incur a panic since acpi_read() and acpi_write() end up
> calling ioremap(), which may block to allocate memory - BZ 18012.
since you just access several bytes mmio in interrupt context, can't you
use kmap_atomic_pfn() here?

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Huang Ying]

On Fri, 2010-10-22 at 10:43 +0800, Li, Shaohua wrote:
> On Fri, 2010-10-22 at 04:23 +0800, Myron Stowe wrote:
> > ACPI's system event related IRQ handing accesses specific fixed
> > hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
> > which are declared in the FADT.  If these registers are backed by
> > MMIO, as opposed to I/O port space, accessing them within interrupt
> > context will incur a panic since acpi_read() and acpi_write() end up
> > calling ioremap(), which may block to allocate memory - BZ 18012.
> since you just access several bytes mmio in interrupt context, can't you
> use kmap_atomic_pfn() here?

On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires
struct page for the physical address. But the MMIO address may have no
struct page.

Best Regards
Huang Ying

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Shaohua Li]

On Fri, 2010-10-22 at 10:57 +0800, Huang, Ying wrote:
> On Fri, 2010-10-22 at 10:43 +0800, Li, Shaohua wrote:
> > On Fri, 2010-10-22 at 04:23 +0800, Myron Stowe wrote:
> > > ACPI's system event related IRQ handing accesses specific fixed
> > > hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
> > > which are declared in the FADT.  If these registers are backed by
> > > MMIO, as opposed to I/O port space, accessing them within interrupt
> > > context will incur a panic since acpi_read() and acpi_write() end up
> > > calling ioremap(), which may block to allocate memory - BZ 18012.
> > since you just access several bytes mmio in interrupt context, can't you
> > use kmap_atomic_pfn() here?
> 
> On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires
> struct page for the physical address. But the MMIO address may have no
> struct page.
ok, can we add a new entry in fix map and the entry is dedicated for
mmio mapping? The ioremap list looks like a hack.

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Huang Ying]

On Fri, 2010-10-22 at 11:16 +0800, Li, Shaohua wrote:
> On Fri, 2010-10-22 at 10:57 +0800, Huang, Ying wrote:
> > On Fri, 2010-10-22 at 10:43 +0800, Li, Shaohua wrote:
> > > On Fri, 2010-10-22 at 04:23 +0800, Myron Stowe wrote:
> > > > ACPI's system event related IRQ handing accesses specific fixed
> > > > hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
> > > > which are declared in the FADT.  If these registers are backed by
> > > > MMIO, as opposed to I/O port space, accessing them within interrupt
> > > > context will incur a panic since acpi_read() and acpi_write() end up
> > > > calling ioremap(), which may block to allocate memory - BZ 18012.
> > > since you just access several bytes mmio in interrupt context, can't you
> > > use kmap_atomic_pfn() here?
> > 
> > On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires
> > struct page for the physical address. But the MMIO address may have no
> > struct page.
> ok, can we add a new entry in fix map and the entry is dedicated for
> mmio mapping? The ioremap list looks like a hack.

Do ioremap in atomic context is so easy. It is not only just allocating
some virtual address space, you need care about the PAT too to prevent
address is mapped both cached and uncached. Please take a look at
reserve_memtype() which is used in ioremap(). Pre-mapping makes thing
easier in fact.

Best Regards,
Huang Ying

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Bjorn Helgaas]

On Thursday, October 21, 2010 09:16:12 pm Shaohua Li wrote:
> On Fri, 2010-10-22 at 10:57 +0800, Huang, Ying wrote:
> > On Fri, 2010-10-22 at 10:43 +0800, Li, Shaohua wrote:
> > > On Fri, 2010-10-22 at 04:23 +0800, Myron Stowe wrote:
> > > > ACPI's system event related IRQ handing accesses specific fixed
> > > > hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1
> > > > which are declared in the FADT.  If these registers are backed by
> > > > MMIO, as opposed to I/O port space, accessing them within interrupt
> > > > context will incur a panic since acpi_read() and acpi_write() end up
> > > > calling ioremap(), which may block to allocate memory - BZ 18012.
> > > since you just access several bytes mmio in interrupt context, can't you
> > > use kmap_atomic_pfn() here?
> > 
> > On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires
> > struct page for the physical address. But the MMIO address may have no
> > struct page.

I really like this idea because we could get rid of the list and
all the RCU stuff, but I don't see how to make it work yet.

On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires
a struct page.  An ia64-specific version that doesn't need a struct
page would be trivial, but I still don't know how to make it work on
x86_64.

> ok, can we add a new entry in fix map and the entry is dedicated for
> mmio mapping? The ioremap list looks like a hack.

Fixmap is arch-specific and there's no ia64 implementation, so I
don't think we can use it here.

Bjorn

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Andi Kleen]

> On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires
> a struct page.  An ia64-specific version that doesn't need a struct
> page would be trivial, but I still don't know how to make it work on
> x86_64.

Doing a struct page less kmap_atomic() shouldn't be too hard 
on x86 either.

Just to avoid problems with concurrency you likely need per 
CPU mappings.

-Andi

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Huang Ying]

On Mon, 2010-10-25 at 16:34 +0800, Andi Kleen wrote:
> > On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires
> > a struct page.  An ia64-specific version that doesn't need a struct
> > page would be trivial, but I still don't know how to make it work on
> > x86_64.
> 
> Doing a struct page less kmap_atomic() shouldn't be too hard 
> on x86 either.
> 
> Just to avoid problems with concurrency you likely need per 
> CPU mappings.

One issue is PAT management, like in general ioremap implementation
(reserve_memtype).

Best Regards,
Huang Ying

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Andi Kleen]

On Mon, Oct 25, 2010 at 04:43:55PM +0800, Huang Ying wrote:
> On Mon, 2010-10-25 at 16:34 +0800, Andi Kleen wrote:
> > > On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires
> > > a struct page.  An ia64-specific version that doesn't need a struct
> > > page would be trivial, but I still don't know how to make it work on
> > > x86_64.
> > 
> > Doing a struct page less kmap_atomic() shouldn't be too hard 
> > on x86 either.
> > 
> > Just to avoid problems with concurrency you likely need per 
> > CPU mappings.
> 
> One issue is PAT management, like in general ioremap implementation
> (reserve_memtype).

That's true, didn't consider that.
Making that fully lockless is probably too hard.

-Andi

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Len Brown]

applied to acpi-test

My comfort-level with these changes is not high.

I'm open to your recommendations on if they are
too late/risky for this merge window -- or too
important to miss this merge window.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Myron Stowe]

On Sun, 2010-10-24 at 23:38 -0400, Len Brown wrote:
> applied to acpi-test
> 
> My comfort-level with these changes is not high.
> 
> I'm open to your recommendations on if they are
> too late/risky for this merge window -- or too
> important to miss this merge window.

Hi Len:

I don't track your work/integration methods so perhaps I inadvertently
just hit the end of one of your merge windows.

To your points, I understand your comfort-level concerns.  Most of this
patch series is re-factoring but the testing of the base functionality
provided to date has been limited to APEI so it's probably likely that
not much has really occurred.

It would be fine to skip this merge window and get some more run time.

Myron

> 
> thanks,
> Len Brown, Intel Open Source Technology Center
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Andi Kleen]

On Sun, Oct 24, 2010 at 11:38:22PM -0400, Len Brown wrote:
> applied to acpi-test
> 
> My comfort-level with these changes is not high.
> 
> I'm open to your recommendations on if they are
> too late/risky for this merge window -- or too
> important to miss this merge window.

The NMI handling is fairly important  by itself,
on the other hand it has been broken for a long time.

But yes the patches were late unfortunately.

I still think it would be better to push them out
earlier than later simply to get more testing
on more systems.  That's essentially what they need
and unfortunately the only really good way to get
that is being part of a release.

-Andi

Re: [PATCH 0/7] ACPI: Memory Mapped I/O (MMIO) pre-mapping

[Len Brown]

> On Sun, Oct 24, 2010 at 11:38:22PM -0400, Len Brown wrote:
> > applied to acpi-test
> > 
> > My comfort-level with these changes is not high.
> > 
> > I'm open to your recommendations on if they are
> > too late/risky for this merge window -- or too
> > important to miss this merge window.
> 
> The NMI handling is fairly important  by itself,
> on the other hand it has been broken for a long time.
> 
> But yes the patches were late unfortunately.
> 
> I still think it would be better to push them out
> earlier than later simply to get more testing
> on more systems.  That's essentially what they need
> and unfortunately the only really good way to get
> that is being part of a release.

I agree with Andi.

This bit is upstream now.
if we have troubles with it, then we can
still revert/repair during 2.6.37 rc.

thanks,
Len Brown, Intel Open Source Technology Center