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

[Thomas Renninger <trenn@suse.de>]

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