diff mbox

[v5,12/20] ACPI / APEI: Don't store CPER records physical address in struct ghes

Message ID 20180626170116.25825-13-james.morse@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

James Morse June 26, 2018, 5:01 p.m. UTC
When CPER records are found the address of the records is stashed
in the struct ghes. Once the records have been processed, this
address is overwritten with zero so that it won't be processed
again without being re-populated by firmware.

This goes wrong if a struct ghes can be processed concurrently,
as can happen at probe time when an NMI occurs.

Avoid this stashing by letting the caller hold the address. A
later patch will do away with the use of ghes->flags in the
read/clear code too.

Signed-off-by: James Morse <james.morse@arm.com>
---
 drivers/acpi/apei/ghes.c | 30 +++++++++++++++---------------
 include/acpi/ghes.h      |  1 -
 2 files changed, 15 insertions(+), 16 deletions(-)

Comments

kernel test robot June 26, 2018, 8:55 p.m. UTC | #1
Hi James,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.18-rc2 next-20180626]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/James-Morse/APEI-in_nmi-rework-and-arm64-SDEI-wire-up/20180627-024229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-i1-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/acpi/apei/ghes.c: In function 'ghes_read_estatus':
>> drivers/acpi/apei/ghes.c:300:17: error: passing argument 1 of 'apei_read' from incompatible pointer type [-Werror=incompatible-pointer-types]
     rc = apei_read(buf_paddr, &g->error_status_address);
                    ^~~~~~~~~
   In file included from drivers/acpi/apei/ghes.c:57:0:
   drivers/acpi/apei/apei-internal.h:80:5: note: expected 'u64 * {aka long long unsigned int *}' but argument is of type 'phys_addr_t * {aka unsigned int *}'
    int apei_read(u64 *val, struct acpi_generic_address *reg);
        ^~~~~~~~~
   cc1: some warnings being treated as errors

vim +/apei_read +300 drivers/acpi/apei/ghes.c

   291	
   292	static int ghes_read_estatus(struct ghes *ghes,
   293				     struct acpi_hest_generic_status *estatus,
   294				     phys_addr_t *buf_paddr, int fixmap_idx)
   295	{
   296		struct acpi_hest_generic *g = ghes->generic;
   297		u32 len;
   298		int rc;
   299	
 > 300		rc = apei_read(buf_paddr, &g->error_status_address);
   301		if (rc) {
   302			if (printk_ratelimit())
   303				pr_warning(FW_WARN GHES_PFX
   304	"Failed to read error status block address for hardware error source: %d.\n",
   305					   g->header.source_id);
   306			return -EIO;
   307		}
   308		if (!*buf_paddr)
   309			return -ENOENT;
   310	
   311		ghes_copy_tofrom_phys(estatus, *buf_paddr,
   312				      sizeof(*estatus), 1, fixmap_idx);
   313		if (!estatus->block_status)
   314			return -ENOENT;
   315	
   316		ghes->flags |= GHES_TO_CLEAR;
   317	
   318		rc = -EIO;
   319		len = cper_estatus_len(estatus);
   320		if (len < sizeof(*estatus))
   321			goto err_read_block;
   322		if (len > ghes->generic->error_block_length)
   323			goto err_read_block;
   324		if (cper_estatus_check_header(estatus))
   325			goto err_read_block;
   326		ghes_copy_tofrom_phys(estatus + 1,
   327				      *buf_paddr + sizeof(*estatus),
   328				      len - sizeof(*estatus), 1, fixmap_idx);
   329		if (cper_estatus_check(estatus))
   330			goto err_read_block;
   331		rc = 0;
   332	
   333	err_read_block:
   334		if (rc && printk_ratelimit())
   335			pr_warning(FW_WARN GHES_PFX
   336				   "Failed to read error status block!\n");
   337		return rc;
   338	}
   339	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 7b412508b3ea..b0054dfad9cc 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -266,7 +266,7 @@  static inline int ghes_severity(int severity)
 	}
 }
 
-static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
+static void ghes_copy_tofrom_phys(void *buffer, phys_addr_t paddr, u32 len,
 				  int from_phys, int fixmap_idx)
 {
 	void __iomem *vaddr;
@@ -291,14 +291,13 @@  static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
 
 static int ghes_read_estatus(struct ghes *ghes,
 			     struct acpi_hest_generic_status *estatus,
-			     int fixmap_idx)
+			     phys_addr_t *buf_paddr, int fixmap_idx)
 {
 	struct acpi_hest_generic *g = ghes->generic;
-	u64 buf_paddr;
 	u32 len;
 	int rc;
 
-	rc = apei_read(&buf_paddr, &g->error_status_address);
+	rc = apei_read(buf_paddr, &g->error_status_address);
 	if (rc) {
 		if (printk_ratelimit())
 			pr_warning(FW_WARN GHES_PFX
@@ -306,15 +305,14 @@  static int ghes_read_estatus(struct ghes *ghes,
 				   g->header.source_id);
 		return -EIO;
 	}
-	if (!buf_paddr)
+	if (!*buf_paddr)
 		return -ENOENT;
 
-	ghes_copy_tofrom_phys(estatus, buf_paddr,
+	ghes_copy_tofrom_phys(estatus, *buf_paddr,
 			      sizeof(*estatus), 1, fixmap_idx);
 	if (!estatus->block_status)
 		return -ENOENT;
 
-	ghes->buffer_paddr = buf_paddr;
 	ghes->flags |= GHES_TO_CLEAR;
 
 	rc = -EIO;
@@ -326,7 +324,7 @@  static int ghes_read_estatus(struct ghes *ghes,
 	if (cper_estatus_check_header(estatus))
 		goto err_read_block;
 	ghes_copy_tofrom_phys(estatus + 1,
-			      buf_paddr + sizeof(*estatus),
+			      *buf_paddr + sizeof(*estatus),
 			      len - sizeof(*estatus), 1, fixmap_idx);
 	if (cper_estatus_check(estatus))
 		goto err_read_block;
@@ -341,12 +339,12 @@  static int ghes_read_estatus(struct ghes *ghes,
 
 static void ghes_clear_estatus(struct ghes *ghes,
 			       struct acpi_hest_generic_status *estatus,
-			       int fixmap_idx)
+			       phys_addr_t buf_paddr, int fixmap_idx)
 {
 	estatus->block_status = 0;
 	if (!(ghes->flags & GHES_TO_CLEAR))
 		return;
-	ghes_copy_tofrom_phys(estatus, ghes->buffer_paddr,
+	ghes_copy_tofrom_phys(estatus, buf_paddr,
 			      sizeof(estatus->block_status), 0, fixmap_idx);
 	ghes->flags &= ~GHES_TO_CLEAR;
 }
@@ -718,10 +716,11 @@  static void __process_error(struct ghes *ghes,
 static int _in_nmi_notify_one(struct ghes *ghes, int fixmap_idx)
 {
 	int sev;
+	phys_addr_t buf_paddr;
 	struct acpi_hest_generic_status *estatus = ghes->estatus;
 
-	if (ghes_read_estatus(ghes, estatus, fixmap_idx)) {
-		ghes_clear_estatus(ghes, estatus, fixmap_idx);
+	if (ghes_read_estatus(ghes, estatus, &buf_paddr, fixmap_idx)) {
+		ghes_clear_estatus(ghes, estatus, buf_paddr, fixmap_idx);
 		return -ENOENT;
 	}
 
@@ -735,7 +734,7 @@  static int _in_nmi_notify_one(struct ghes *ghes, int fixmap_idx)
 		return 0;
 
 	__process_error(ghes, estatus);
-	ghes_clear_estatus(ghes, estatus, fixmap_idx);
+	ghes_clear_estatus(ghes, estatus, buf_paddr, fixmap_idx);
 
 	return 0;
 }
@@ -853,11 +852,12 @@  static int ghes_proc(struct ghes *ghes)
 {
 	int rc;
 	unsigned long flags;
+	phys_addr_t buf_paddr;
 	struct acpi_hest_generic_status *estatus = ghes->estatus;
 
 	spin_lock_irqsave(&ghes_notify_lock_irq, flags);
 
-	rc = ghes_read_estatus(ghes, estatus, FIX_APEI_GHES_IRQ);
+	rc = ghes_read_estatus(ghes, estatus, &buf_paddr, FIX_APEI_GHES_IRQ);
 	if (rc)
 		goto out;
 
@@ -871,7 +871,7 @@  static int ghes_proc(struct ghes *ghes)
 	ghes_do_proc(ghes, estatus);
 
 out:
-	ghes_clear_estatus(ghes, estatus, FIX_APEI_GHES_IRQ);
+	ghes_clear_estatus(ghes, estatus, buf_paddr, FIX_APEI_GHES_IRQ);
 
 	if (rc == -ENOENT)
 		goto unlock;
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 1624e2be485c..3d77452e3a1d 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -22,7 +22,6 @@  struct ghes {
 		struct acpi_hest_generic_v2 *generic_v2;
 	};
 	struct acpi_hest_generic_status *estatus;
-	u64 buffer_paddr;
 	unsigned long flags;
 	union {
 		struct list_head list;