[PATCH v9 0/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[PATCH v9 0/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Shiju Jose]

Presently the vendor drivers are unable to do the recovery for the
vendor specific recoverable HW errors because APEI driver does not
support reporting the error to the vendor drivers.

patch set
1. add new interface to the APEI driver for reporting the 
   vendor specific non-fatal HW errors to the drivers.

2. add driver to handle HiSilicon hip PCIe controller's errors.

V9:
1. Fixed 2 improvements suggested by the kbuild test robot. 
1.1 Change ghes_gdata_pool_init() as static function.
1.2. Removed using buffer to store the error data for
     logging in the hisi_pcie_handle_error()

V8:
1. Removed reporting the standard errors through the interface
   because of the conflict with the recent patches in the
   memory error handling path.
2. Fix comments by Dan Carpenter.
   
V7:
1. Add changes in the APEI driver suggested by Borislav Petkov, for
   queuing up all the non-fatal HW errors to the work queue and
   notify the registered kernel drivers from the bottom half using
   blocking notifier, common interface for both standard and
   vendor-spcific errors.
2. Fix for further feedbacks in v5 HIP PCIe error handler driver
   by Bjorn Helgaas.

V6:
1. Fix few changes in the patch subject line suggested by Bjorn Helgaas.

V5:
1. Fix comments from James Morse.
1.1 Changed the notification method to use the atomic_notifier_chain.
1.2 Add the error handled status for the user space.  

V4:
1. Fix for the following smatch warning in the PCIe error driver,
   reported by kbuild test robot<lkp@intel.com>:
   warn: should '((((1))) << (9 + i))' be a 64 bit type?
   if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
	^^^ This should be BIT_ULL() because it goes up to 9 + 32.

V3:
1. Fix the comments from Bjorn Helgaas.

V2:
1. Changes in the HiSilicon PCIe controller's error handling driver
   for the comments from Bjorn Helgaas.
   
2. Changes in the APEI interface to support reporting the vendor error
   for module with multiple devices, but use the same section type.
   In the error handler will use socket id/sub module id etc to distinguish
   the device.

V1:  
1. Fix comments from James Morse.

2. add driver to handle HiSilicon hip08 PCIe controller's errors,
   which is an application of the above interface.

Shiju Jose (1):
  ACPI / APEI: Add support to notify the vendor specific HW errors

Yicong Yang (1):
  PCI: hip: Add handling of HiSilicon HIP PCIe controller errors

 drivers/acpi/apei/ghes.c                 | 130 +++++++++-
 drivers/pci/controller/Kconfig           |   8 +
 drivers/pci/controller/Makefile          |   1 +
 drivers/pci/controller/pcie-hisi-error.c | 305 +++++++++++++++++++++++
 include/acpi/ghes.h                      |  28 +++
 5 files changed, 471 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/controller/pcie-hisi-error.c

-- 
2.17.1

[PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Shiju Jose]

Add support to notify the vendor specific non-fatal HW errors
to the drivers for the error recovery.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/acpi/apei/ghes.c | 130 ++++++++++++++++++++++++++++++++++++++-
 include/acpi/ghes.h      |  28 +++++++++
 2 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 24c9642e8fc7..854d8115cdfc 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -33,6 +33,7 @@
 #include <linux/irq_work.h>
 #include <linux/llist.h>
 #include <linux/genalloc.h>
+#include <linux/kfifo.h>
 #include <linux/pci.h>
 #include <linux/pfn.h>
 #include <linux/aer.h>
@@ -63,6 +64,11 @@
 #define GHES_ESTATUS_CACHES_SIZE	4
 
 #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
+
+#define GHES_EVENT_RING_SIZE	256
+#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
+#define GHES_GDATA_POOL_MIN_SIZE	65536
+
 /* Prevent too many caches are allocated because of RCU */
 #define GHES_ESTATUS_CACHE_ALLOCED_MAX	(GHES_ESTATUS_CACHES_SIZE * 3 / 2)
 
@@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
  */
 static DEFINE_SPINLOCK(ghes_notify_lock_irq);
 
+struct ghes_event_entry {
+	struct acpi_hest_generic_data *gdata;
+	int error_severity;
+};
+
+static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
+		    GHES_EVENT_RING_SIZE);
+
+static DEFINE_SPINLOCK(ghes_event_ring_lock);
+
+static struct gen_pool *ghes_gdata_pool;
+static unsigned long ghes_gdata_pool_size_request;
+
 static struct gen_pool *ghes_estatus_pool;
 static unsigned long ghes_estatus_pool_size_request;
 
@@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
 	return -ENOMEM;
 }
 
+static int ghes_gdata_pool_init(void)
+{
+	unsigned long addr, len;
+	int rc;
+
+	ghes_gdata_pool = gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
+	if (!ghes_gdata_pool)
+		return -ENOMEM;
+
+	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
+		ghes_gdata_pool_size_request = GHES_GDATA_POOL_MIN_SIZE;
+
+	len = ghes_gdata_pool_size_request;
+	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
+	if (!addr)
+		goto err_pool_alloc;
+
+	vmalloc_sync_mappings();
+
+	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
+	if (rc)
+		goto err_pool_add;
+
+	return 0;
+
+err_pool_add:
+	vfree((void *)addr);
+
+err_pool_alloc:
+	gen_pool_destroy(ghes_gdata_pool);
+
+	return -ENOMEM;
+}
+
 static int map_gen_v2(struct ghes *ghes)
 {
 	return apei_map_generic_address(&ghes->generic_v2->read_ack_register);
@@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 		goto err_unmap_status_addr;
 	}
 
+	ghes_gdata_pool_size_request += generic->records_to_preallocate *
+					generic->max_sections_per_record *
+					generic->max_raw_data_length;
+
 	return ghes;
 
 err_unmap_status_addr:
@@ -490,6 +547,68 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 #endif
 }
 
+static BLOCKING_NOTIFIER_HEAD(ghes_event_notify_list);
+
+/**
+ * ghes_register_event_notifier - register an event notifier
+ * for the non-fatal HW errors.
+ * @nb: pointer to the notifier_block structure of the event handler.
+ *
+ * return 0 : SUCCESS, non-zero : FAIL
+ */
+int ghes_register_event_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&ghes_event_notify_list, nb);
+}
+EXPORT_SYMBOL_GPL(ghes_register_event_notifier);
+
+/**
+ * ghes_unregister_event_notifier - unregister the previously
+ * registered event notifier.
+ * @nb: pointer to the notifier_block structure of the event handler.
+ */
+void ghes_unregister_event_notifier(struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&ghes_event_notify_list, nb);
+}
+EXPORT_SYMBOL_GPL(ghes_unregister_event_notifier);
+
+static void ghes_event_work_func(struct work_struct *work)
+{
+	struct ghes_event_entry entry;
+	u32 len;
+
+	while (kfifo_get(&ghes_event_ring, &entry)) {
+		blocking_notifier_call_chain(&ghes_event_notify_list,
+					     entry.error_severity,
+					     entry.gdata);
+		len = acpi_hest_get_record_size(entry.gdata);
+		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata, len);
+	}
+}
+
+static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
+
+static void ghes_handle_non_standard_event(struct acpi_hest_generic_data *gdata,
+					   int sev)
+{
+	u32 len;
+	struct ghes_event_entry event_entry;
+
+	len = acpi_hest_get_record_size(gdata);
+	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
+	if (event_entry.gdata) {
+		memcpy(event_entry.gdata, gdata, len);
+		event_entry.error_severity = sev;
+
+		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
+					&ghes_event_ring_lock))
+			schedule_work(&ghes_event_work);
+		else
+			pr_warn(GHES_PFX "ghes event queue full\n");
+	}
+}
+
 static void ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {
@@ -527,6 +646,7 @@ static void ghes_do_proc(struct ghes *ghes,
 		} else {
 			void *err = acpi_hest_get_payload(gdata);
 
+			ghes_handle_non_standard_event(gdata, sev);
 			log_non_standard_event(sec_type, fru_id, fru_text,
 					       sec_sev, err,
 					       gdata->error_data_length);
@@ -1334,7 +1454,7 @@ static int __init ghes_init(void)
 
 	rc = platform_driver_register(&ghes_platform_driver);
 	if (rc)
-		goto err;
+		goto exit;
 
 	rc = apei_osc_setup();
 	if (rc == 0 && osc_sb_apei_support_acked)
@@ -1346,8 +1466,16 @@ static int __init ghes_init(void)
 	else
 		pr_info(GHES_PFX "Failed to enable APEI firmware first mode.\n");
 
+	rc = ghes_gdata_pool_init();
+	if (rc) {
+		pr_warn(GHES_PFX "ghes_gdata_pool_init failed\n");
+		goto err;
+	}
+
 	return 0;
 err:
+	platform_driver_unregister(&ghes_platform_driver);
+exit:
 	return rc;
 }
 device_initcall(ghes_init);
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index e3f1cddb4ac8..a3dd82069069 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -50,6 +50,34 @@ enum {
 	GHES_SEV_PANIC = 0x3,
 };
 
+
+#ifdef CONFIG_ACPI_APEI_GHES
+/**
+ * ghes_register_event_notifier - register an event notifier
+ * for the non-fatal HW errors.
+ * @nb: pointer to the notifier_block structure of the event notifier.
+ *
+ * Return : 0 - SUCCESS, non-zero - FAIL.
+ */
+int ghes_register_event_notifier(struct notifier_block *nb);
+
+/**
+ * ghes_unregister_event_notifier - unregister the previously
+ * registered event notifier.
+ * @nb: pointer to the notifier_block structure of the event notifier.
+ */
+void ghes_unregister_event_notifier(struct notifier_block *nb);
+#else
+static inline int ghes_register_event_notifier(struct notifier_block *nb)
+{
+	return -ENODEV;
+}
+
+static inline void ghes_unregister_event_notifier(struct notifier_block *nb)
+{
+}
+#endif
+
 int ghes_estatus_pool_init(int num_ghes);
 
 /* From drivers/edac/ghes_edac.c */
-- 
2.17.1

Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Bjorn Helgaas]

On Mon, Jun 15, 2020 at 10:53:11AM +0100, Shiju Jose wrote:
> Add support to notify the vendor specific non-fatal HW errors
> to the drivers for the error recovery.

This doesn't actually say anything about what this patch does.  Sure,
it "adds support," but it doesn't say anything about how that support
works or how to use it.

This should say something about a FIFO and it should mention that an
event handler registered with ghes_register_event_notifier() will be
called with each vendor-specific error record.

> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/acpi/apei/ghes.c | 130 ++++++++++++++++++++++++++++++++++++++-
>  include/acpi/ghes.h      |  28 +++++++++
>  2 files changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 24c9642e8fc7..854d8115cdfc 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -33,6 +33,7 @@
>  #include <linux/irq_work.h>
>  #include <linux/llist.h>
>  #include <linux/genalloc.h>
> +#include <linux/kfifo.h>
>  #include <linux/pci.h>
>  #include <linux/pfn.h>
>  #include <linux/aer.h>
> @@ -63,6 +64,11 @@
>  #define GHES_ESTATUS_CACHES_SIZE	4
>  
>  #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
> +
> +#define GHES_EVENT_RING_SIZE	256
> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
> +#define GHES_GDATA_POOL_MIN_SIZE	65536

Don't drop these new #defines right in the middle of the GHES_ESTATUS
block.  The ESTATUS ones are all related, and these new ones are
something separate.

These new names should be related somehow.  The names don't make it
clear that GHES_EVENT and GHES_GDATA are related.  IIUC GHES_GDATA is
space for storing GHES structures, and GHES_EVENT is a FIFO of struct
ghes_event_entry, each of which points to one of those GHES
structures.

>  /* Prevent too many caches are allocated because of RCU */
>  #define GHES_ESTATUS_CACHE_ALLOCED_MAX	(GHES_ESTATUS_CACHES_SIZE * 3 / 2)
>  
> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>   */
>  static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>  
> +struct ghes_event_entry {
> +	struct acpi_hest_generic_data *gdata;
> +	int error_severity;
> +};
> +
> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
> +		    GHES_EVENT_RING_SIZE);
> +
> +static DEFINE_SPINLOCK(ghes_event_ring_lock);
> +
> +static struct gen_pool *ghes_gdata_pool;
> +static unsigned long ghes_gdata_pool_size_request;
> +
>  static struct gen_pool *ghes_estatus_pool;
>  static unsigned long ghes_estatus_pool_size_request;
>  
> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
>  	return -ENOMEM;
>  }
>  
> +static int ghes_gdata_pool_init(void)
> +{
> +	unsigned long addr, len;
> +	int rc;
> +
> +	ghes_gdata_pool = gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
> +	if (!ghes_gdata_pool)
> +		return -ENOMEM;
> +
> +	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
> +		ghes_gdata_pool_size_request = GHES_GDATA_POOL_MIN_SIZE;
> +
> +	len = ghes_gdata_pool_size_request;
> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> +	if (!addr)
> +		goto err_pool_alloc;
> +
> +	vmalloc_sync_mappings();
> +
> +	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
> +	if (rc)
> +		goto err_pool_add;
> +
> +	return 0;
> +
> +err_pool_add:
> +	vfree((void *)addr);
> +
> +err_pool_alloc:
> +	gen_pool_destroy(ghes_gdata_pool);
> +
> +	return -ENOMEM;
> +}
> +
>  static int map_gen_v2(struct ghes *ghes)
>  {
>  	return apei_map_generic_address(&ghes->generic_v2->read_ack_register);
> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  		goto err_unmap_status_addr;
>  	}
>  
> +	ghes_gdata_pool_size_request += generic->records_to_preallocate *
> +					generic->max_sections_per_record *
> +					generic->max_raw_data_length;
> +
>  	return ghes;
>  
>  err_unmap_status_addr:
> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
>  #endif
>  }
>  
> +static BLOCKING_NOTIFIER_HEAD(ghes_event_notify_list);
> +
> +/**
> + * ghes_register_event_notifier - register an event notifier
> + * for the non-fatal HW errors.
> + * @nb: pointer to the notifier_block structure of the event handler.
> + *
> + * return 0 : SUCCESS, non-zero : FAIL
> + */
> +int ghes_register_event_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&ghes_event_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(ghes_register_event_notifier);
> +
> +/**
> + * ghes_unregister_event_notifier - unregister the previously
> + * registered event notifier.
> + * @nb: pointer to the notifier_block structure of the event handler.
> + */
> +void ghes_unregister_event_notifier(struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&ghes_event_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(ghes_unregister_event_notifier);
> +
> +static void ghes_event_work_func(struct work_struct *work)
> +{
> +	struct ghes_event_entry entry;
> +	u32 len;
> +
> +	while (kfifo_get(&ghes_event_ring, &entry)) {
> +		blocking_notifier_call_chain(&ghes_event_notify_list,
> +					     entry.error_severity,
> +					     entry.gdata);
> +		len = acpi_hest_get_record_size(entry.gdata);
> +		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata, len);
> +	}
> +}
> +
> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
> +
> +static void ghes_handle_non_standard_event(struct acpi_hest_generic_data *gdata,
> +					   int sev)
> +{
> +	u32 len;
> +	struct ghes_event_entry event_entry;
> +
> +	len = acpi_hest_get_record_size(gdata);
> +	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
> +	if (event_entry.gdata) {

Make this:

  if (!event_entry.gdata) {
    pr_warn(...);
    return;
  }

and then you won't have to indent the usual case below.

> +		memcpy(event_entry.gdata, gdata, len);
> +		event_entry.error_severity = sev;
> +
> +		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
> +					&ghes_event_ring_lock))
> +			schedule_work(&ghes_event_work);
> +		else
> +			pr_warn(GHES_PFX "ghes event queue full\n");

GHES_PFX is already "GHES: ", so no need to repeat "ghes" in your
message.  You can use that space for something more useful, like the
source ID, section type, severity, etc.  Dmesg strings that are
completely constant are not as useful as they could be.

> +	}
> +}
> +
>  static void ghes_do_proc(struct ghes *ghes,
>  			 const struct acpi_hest_generic_status *estatus)
>  {
> @@ -527,6 +646,7 @@ static void ghes_do_proc(struct ghes *ghes,
>  		} else {
>  			void *err = acpi_hest_get_payload(gdata);
>  
> +			ghes_handle_non_standard_event(gdata, sev);
>  			log_non_standard_event(sec_type, fru_id, fru_text,
>  					       sec_sev, err,
>  					       gdata->error_data_length);
> @@ -1334,7 +1454,7 @@ static int __init ghes_init(void)
>  
>  	rc = platform_driver_register(&ghes_platform_driver);
>  	if (rc)
> -		goto err;
> +		goto exit;
>  
>  	rc = apei_osc_setup();
>  	if (rc == 0 && osc_sb_apei_support_acked)
> @@ -1346,8 +1466,16 @@ static int __init ghes_init(void)
>  	else
>  		pr_info(GHES_PFX "Failed to enable APEI firmware first mode.\n");
>  
> +	rc = ghes_gdata_pool_init();
> +	if (rc) {
> +		pr_warn(GHES_PFX "ghes_gdata_pool_init failed\n");

I don't think this message is really meaningful to a user.  Maybe
something about non-fatal vendor-specific errors not being logged?

> +		goto err;

I'm not sure this is an error that should cause the whole GHES driver
to be unregistered.  After all, the driver *used* to work fine even
without this vendor-specific functionality.  Seems like we ought to be
able to fall back to the previous behavior even if we can't allocate
the gdata pool.

> +	}
> +
>  	return 0;
>  err:
> +	platform_driver_unregister(&ghes_platform_driver);
> +exit:
>  	return rc;
>  }
>  device_initcall(ghes_init);
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index e3f1cddb4ac8..a3dd82069069 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -50,6 +50,34 @@ enum {
>  	GHES_SEV_PANIC = 0x3,
>  };
>  
> +

Don't add an extra blank line here.

> +#ifdef CONFIG_ACPI_APEI_GHES
> +/**
> + * ghes_register_event_notifier - register an event notifier
> + * for the non-fatal HW errors.
> + * @nb: pointer to the notifier_block structure of the event notifier.
> + *
> + * Return : 0 - SUCCESS, non-zero - FAIL.
> + */
> +int ghes_register_event_notifier(struct notifier_block *nb);
> +
> +/**
> + * ghes_unregister_event_notifier - unregister the previously
> + * registered event notifier.
> + * @nb: pointer to the notifier_block structure of the event notifier.
> + */
> +void ghes_unregister_event_notifier(struct notifier_block *nb);
> +#else
> +static inline int ghes_register_event_notifier(struct notifier_block *nb)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline void ghes_unregister_event_notifier(struct notifier_block *nb)
> +{
> +}
> +#endif
> +
>  int ghes_estatus_pool_init(int num_ghes);
>  
>  /* From drivers/edac/ghes_edac.c */
> -- 
> 2.17.1
> 
> 

RE: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Shiju Jose]

Hi Bjorn,

Thanks for reviewing.

I will make changes for your suggestions in both patches.

Regards,
Shiju  

>-----Original Message-----
>From: Bjorn Helgaas [mailto:helgaas@kernel.org]
>Sent: 16 June 2020 23:57
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; bp@alien8.de;
>james.morse@arm.com; lenb@kernel.org; tony.luck@intel.com;
>dan.carpenter@oracle.com; zhangliguang@linux.alibaba.com;
>andriy.shevchenko@linux.intel.com; Wangkefeng (OS Kernel Lab)
><wangkefeng.wang@huawei.com>; jroedel@suse.de; yangyicong
><yangyicong@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>
>Subject: Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor
>specific HW errors
>
>On Mon, Jun 15, 2020 at 10:53:11AM +0100, Shiju Jose wrote:
>> Add support to notify the vendor specific non-fatal HW errors to the
>> drivers for the error recovery.
>
>This doesn't actually say anything about what this patch does.  Sure, it "adds
>support," but it doesn't say anything about how that support works or how
>to use it.
>
>This should say something about a FIFO and it should mention that an event
>handler registered with ghes_register_event_notifier() will be called with
>each vendor-specific error record.
>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/acpi/apei/ghes.c | 130
>++++++++++++++++++++++++++++++++++++++-
>>  include/acpi/ghes.h      |  28 +++++++++
>>  2 files changed, 157 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
>> 24c9642e8fc7..854d8115cdfc 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/irq_work.h>
>>  #include <linux/llist.h>
>>  #include <linux/genalloc.h>
>> +#include <linux/kfifo.h>
>>  #include <linux/pci.h>
>>  #include <linux/pfn.h>
>>  #include <linux/aer.h>
>> @@ -63,6 +64,11 @@
>>  #define GHES_ESTATUS_CACHES_SIZE	4
>>
>>  #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
>> +
>> +#define GHES_EVENT_RING_SIZE	256
>> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
>> +#define GHES_GDATA_POOL_MIN_SIZE	65536
>
>Don't drop these new #defines right in the middle of the GHES_ESTATUS
>block.  The ESTATUS ones are all related, and these new ones are something
>separate.
>
>These new names should be related somehow.  The names don't make it clear
>that GHES_EVENT and GHES_GDATA are related.  IIUC GHES_GDATA is space
>for storing GHES structures, and GHES_EVENT is a FIFO of struct
>ghes_event_entry, each of which points to one of those GHES structures.
>
>>  /* Prevent too many caches are allocated because of RCU */
>>  #define GHES_ESTATUS_CACHE_ALLOCED_MAX
>	(GHES_ESTATUS_CACHES_SIZE * 3 / 2)
>>
>> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>>   */
>>  static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>>
>> +struct ghes_event_entry {
>> +	struct acpi_hest_generic_data *gdata;
>> +	int error_severity;
>> +};
>> +
>> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
>> +		    GHES_EVENT_RING_SIZE);
>> +
>> +static DEFINE_SPINLOCK(ghes_event_ring_lock);
>> +
>> +static struct gen_pool *ghes_gdata_pool; static unsigned long
>> +ghes_gdata_pool_size_request;
>> +
>>  static struct gen_pool *ghes_estatus_pool;  static unsigned long
>> ghes_estatus_pool_size_request;
>>
>> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
>>  	return -ENOMEM;
>>  }
>>
>> +static int ghes_gdata_pool_init(void) {
>> +	unsigned long addr, len;
>> +	int rc;
>> +
>> +	ghes_gdata_pool =
>gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
>> +	if (!ghes_gdata_pool)
>> +		return -ENOMEM;
>> +
>> +	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
>> +		ghes_gdata_pool_size_request =
>GHES_GDATA_POOL_MIN_SIZE;
>> +
>> +	len = ghes_gdata_pool_size_request;
>> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> +	if (!addr)
>> +		goto err_pool_alloc;
>> +
>> +	vmalloc_sync_mappings();
>> +
>> +	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
>> +	if (rc)
>> +		goto err_pool_add;
>> +
>> +	return 0;
>> +
>> +err_pool_add:
>> +	vfree((void *)addr);
>> +
>> +err_pool_alloc:
>> +	gen_pool_destroy(ghes_gdata_pool);
>> +
>> +	return -ENOMEM;
>> +}
>> +
>>  static int map_gen_v2(struct ghes *ghes)  {
>>  	return
>> apei_map_generic_address(&ghes->generic_v2->read_ack_register);
>> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct
>acpi_hest_generic *generic)
>>  		goto err_unmap_status_addr;
>>  	}
>>
>> +	ghes_gdata_pool_size_request += generic->records_to_preallocate *
>> +					generic->max_sections_per_record *
>> +					generic->max_raw_data_length;
>> +
>>  	return ghes;
>>
>>  err_unmap_status_addr:
>> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct
>> acpi_hest_generic_data *gdata)  #endif  }
>>
>> +static BLOCKING_NOTIFIER_HEAD(ghes_event_notify_list);
>> +
>> +/**
>> + * ghes_register_event_notifier - register an event notifier
>> + * for the non-fatal HW errors.
>> + * @nb: pointer to the notifier_block structure of the event handler.
>> + *
>> + * return 0 : SUCCESS, non-zero : FAIL  */ int
>> +ghes_register_event_notifier(struct notifier_block *nb) {
>> +	return blocking_notifier_chain_register(&ghes_event_notify_list,
>> +nb); } EXPORT_SYMBOL_GPL(ghes_register_event_notifier);
>> +
>> +/**
>> + * ghes_unregister_event_notifier - unregister the previously
>> + * registered event notifier.
>> + * @nb: pointer to the notifier_block structure of the event handler.
>> + */
>> +void ghes_unregister_event_notifier(struct notifier_block *nb)
>> +{
>> +	blocking_notifier_chain_unregister(&ghes_event_notify_list, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(ghes_unregister_event_notifier);
>> +
>> +static void ghes_event_work_func(struct work_struct *work)
>> +{
>> +	struct ghes_event_entry entry;
>> +	u32 len;
>> +
>> +	while (kfifo_get(&ghes_event_ring, &entry)) {
>> +		blocking_notifier_call_chain(&ghes_event_notify_list,
>> +					     entry.error_severity,
>> +					     entry.gdata);
>> +		len = acpi_hest_get_record_size(entry.gdata);
>> +		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata,
>len);
>> +	}
>> +}
>> +
>> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
>> +
>> +static void ghes_handle_non_standard_event(struct
>acpi_hest_generic_data *gdata,
>> +					   int sev)
>> +{
>> +	u32 len;
>> +	struct ghes_event_entry event_entry;
>> +
>> +	len = acpi_hest_get_record_size(gdata);
>> +	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
>> +	if (event_entry.gdata) {
>
>Make this:
>
>  if (!event_entry.gdata) {
>    pr_warn(...);
>    return;
>  }
>
>and then you won't have to indent the usual case below.
>
>> +		memcpy(event_entry.gdata, gdata, len);
>> +		event_entry.error_severity = sev;
>> +
>> +		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
>> +					&ghes_event_ring_lock))
>> +			schedule_work(&ghes_event_work);
>> +		else
>> +			pr_warn(GHES_PFX "ghes event queue full\n");
>
>GHES_PFX is already "GHES: ", so no need to repeat "ghes" in your
>message.  You can use that space for something more useful, like the
>source ID, section type, severity, etc.  Dmesg strings that are
>completely constant are not as useful as they could be.
>
>> +	}
>> +}
>> +
>>  static void ghes_do_proc(struct ghes *ghes,
>>  			 const struct acpi_hest_generic_status *estatus)
>>  {
>> @@ -527,6 +646,7 @@ static void ghes_do_proc(struct ghes *ghes,
>>  		} else {
>>  			void *err = acpi_hest_get_payload(gdata);
>>
>> +			ghes_handle_non_standard_event(gdata, sev);
>>  			log_non_standard_event(sec_type, fru_id, fru_text,
>>  					       sec_sev, err,
>>  					       gdata->error_data_length);
>> @@ -1334,7 +1454,7 @@ static int __init ghes_init(void)
>>
>>  	rc = platform_driver_register(&ghes_platform_driver);
>>  	if (rc)
>> -		goto err;
>> +		goto exit;
>>
>>  	rc = apei_osc_setup();
>>  	if (rc == 0 && osc_sb_apei_support_acked)
>> @@ -1346,8 +1466,16 @@ static int __init ghes_init(void)
>>  	else
>>  		pr_info(GHES_PFX "Failed to enable APEI firmware first
>mode.\n");
>>
>> +	rc = ghes_gdata_pool_init();
>> +	if (rc) {
>> +		pr_warn(GHES_PFX "ghes_gdata_pool_init failed\n");
>
>I don't think this message is really meaningful to a user.  Maybe
>something about non-fatal vendor-specific errors not being logged?
>
>> +		goto err;
>
>I'm not sure this is an error that should cause the whole GHES driver
>to be unregistered.  After all, the driver *used* to work fine even
>without this vendor-specific functionality.  Seems like we ought to be
>able to fall back to the previous behavior even if we can't allocate
>the gdata pool.
>
>> +	}
>> +
>>  	return 0;
>>  err:
>> +	platform_driver_unregister(&ghes_platform_driver);
>> +exit:
>>  	return rc;
>>  }
>>  device_initcall(ghes_init);
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index e3f1cddb4ac8..a3dd82069069 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -50,6 +50,34 @@ enum {
>>  	GHES_SEV_PANIC = 0x3,
>>  };
>>
>> +
>
>Don't add an extra blank line here.
>
>> +#ifdef CONFIG_ACPI_APEI_GHES
>> +/**
>> + * ghes_register_event_notifier - register an event notifier
>> + * for the non-fatal HW errors.
>> + * @nb: pointer to the notifier_block structure of the event notifier.
>> + *
>> + * Return : 0 - SUCCESS, non-zero - FAIL.
>> + */
>> +int ghes_register_event_notifier(struct notifier_block *nb);
>> +
>> +/**
>> + * ghes_unregister_event_notifier - unregister the previously
>> + * registered event notifier.
>> + * @nb: pointer to the notifier_block structure of the event notifier.
>> + */
>> +void ghes_unregister_event_notifier(struct notifier_block *nb);
>> +#else
>> +static inline int ghes_register_event_notifier(struct notifier_block *nb)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>> +static inline void ghes_unregister_event_notifier(struct notifier_block
>*nb)
>> +{
>> +}
>> +#endif
>> +
>>  int ghes_estatus_pool_init(int num_ghes);
>>
>>  /* From drivers/edac/ghes_edac.c */
>> --
>> 2.17.1
>>
>>

Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[James Morse]

Hi Shiju,

On 15/06/2020 10:53, Shiju Jose wrote:
> Add support to notify the vendor specific non-fatal HW errors
> to the drivers for the error recovery.

This doesn't apply cleanly to v5.8-rc1... thanks for waiting for the merge window to
finish, but please rebase onto the latest and greatest kernel!

I'm glad the notifier chains for stuff that should be built-in has gone.
(In my opinion, the RAS code should be moving in the direction of having less code run
between being told of an error, and the handler running. Notifier chains for things like
memory-errors was moving in the wrong direction!)


The Kfifo and pool are adding complexity I don't think you need.
Please make it clear from the naming this is for vendor records. (what is an event?)

The memcpy() for the records is annoying, but eliminating it takes some really invasive
changes. Lets live with it for now.


> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 24c9642e8fc7..854d8115cdfc 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -63,6 +64,11 @@
>  #define GHES_ESTATUS_CACHES_SIZE	4
>  
>  #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
> +
> +#define GHES_EVENT_RING_SIZE	256
> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
> +#define GHES_GDATA_POOL_MIN_SIZE	65536

Huh. Another pool of memory, and we don't know if this will ever be used.
Can we allocate from ghes_estatus_pool instead?

ghes_estatus_pool is already scaled with the number of error sources firmware describes in
ghes_estatus_pool_init(), so it should be big enough.

ghes_estatus_pool already has multiple users, estatus_nodes for work deferred from NMI
come from here, as do ghes_estatus_caches for the low-pass filter thing.


> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>   */
>  static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>  
> +struct ghes_event_entry {

ghes_vendor_record_entry ?

> +	struct acpi_hest_generic_data *gdata;
> +	int error_severity;
> +};

> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
> +		    GHES_EVENT_RING_SIZE);
> +
> +static DEFINE_SPINLOCK(ghes_event_ring_lock);

Do you need the FIFO behaviour?
If you put a work_struct in the struct and schedule_work() that, these would run in any
order, and it would be less code.


> +static struct gen_pool *ghes_gdata_pool;
> +static unsigned long ghes_gdata_pool_size_request;
> +
>  static struct gen_pool *ghes_estatus_pool;
>  static unsigned long ghes_estatus_pool_size_request;

Please use the existing ghes_estatus_pool.


> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)

[...]

> +static int ghes_gdata_pool_init(void)
> +{
> +	unsigned long addr, len;
> +	int rc;
> +
> +	ghes_gdata_pool = gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
> +	if (!ghes_gdata_pool)
> +		return -ENOMEM;
> +
> +	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
> +		ghes_gdata_pool_size_request = GHES_GDATA_POOL_MIN_SIZE;
> +
> +	len = ghes_gdata_pool_size_request;
> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> +	if (!addr)
> +		goto err_pool_alloc;

> +	vmalloc_sync_mappings();
(This isn't needed anymore. See commit 73f693c3a705 ("mm: remove
vmalloc_sync_(un)mappings()"))


> +	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
> +	if (rc)
> +		goto err_pool_add;
> +
> +	return 0;
> +
> +err_pool_add:
> +	vfree((void *)addr);
> +
> +err_pool_alloc:
> +	gen_pool_destroy(ghes_gdata_pool);
> +
> +	return -ENOMEM;
> +}

But: using ghes_estatus_pool would avoid this duplication.


> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  		goto err_unmap_status_addr;
>  	}
>  
> +	ghes_gdata_pool_size_request += generic->records_to_preallocate *
> +					generic->max_sections_per_record *
> +					generic->max_raw_data_length;
> +

Careful, I think ghes_probe() can run in parallel on different CPUs. You can certainly
unbind/rebind it from user-space.

I recall these max this/that/preallocate stuff are junk values on some platform.
You'd at least need to cap it to sane maximum value.

But: Using ghes_estatus_pool would use ghes_estatus_pool_init()'s sizes, which allocates
64K for each error source.

History: https://www.spinics.net/lists/linux-acpi/msg84238.html


> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)

[...]

> +static void ghes_event_work_func(struct work_struct *work)
> +{
> +	struct ghes_event_entry entry;
> +	u32 len;
> +
> +	while (kfifo_get(&ghes_event_ring, &entry)) {
> +		blocking_notifier_call_chain(&ghes_event_notify_list,
> +					     entry.error_severity,
> +					     entry.gdata);
> +		len = acpi_hest_get_record_size(entry.gdata);
> +		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata, len);
> +	}
> +}
> +
> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
> +
> +static void ghes_handle_non_standard_event(struct acpi_hest_generic_data *gdata,
> +					   int sev)
> +{
> +	u32 len;

> +	struct ghes_event_entry event_entry;

> +	len = acpi_hest_get_record_size(gdata);
> +	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
> +	if (event_entry.gdata) {
> +		memcpy(event_entry.gdata, gdata, len);
> +		event_entry.error_severity = sev;
> +
> +		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,

... event_entry is on the stack ...


> +					&ghes_event_ring_lock))
> +			schedule_work(&ghes_event_work);
> +		else
> +			pr_warn(GHES_PFX "ghes event queue full\n");
> +	}
> +}


I think the kfifo is adding un-needed complexity here.


> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index e3f1cddb4ac8..a3dd82069069 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -50,6 +50,34 @@ enum {

> +#ifdef CONFIG_ACPI_APEI_GHES
> +/**
> + * ghes_register_event_notifier - register an event notifier
> + * for the non-fatal HW errors.
> + * @nb: pointer to the notifier_block structure of the event notifier.
> + *
> + * Return : 0 - SUCCESS, non-zero - FAIL.
> + */
> +int ghes_register_event_notifier(struct notifier_block *nb);
> +
> +/**
> + * ghes_unregister_event_notifier - unregister the previously
> + * registered event notifier.
> + * @nb: pointer to the notifier_block structure of the event notifier.
> + */
> +void ghes_unregister_event_notifier(struct notifier_block *nb);
> +#else

Please make it clear from the names these are for vendor events, that the kernel would
otherwise ignore. It looks like these are for everything. Drivers have no business trying
to handle the errors that are handled by things like memory_failure().

~

I would post a version of this to illustrate, but there are comments on patch 2 too.

Something like:
http://www.linux-arm.org/git?p=linux-jm.git;a=commitdiff;h=9c6859f3146001cd9f8edfaf965232cb99c7dc42

(caveat emptor: I've only build tested it)


Thanks,

James

RE: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Shiju Jose]

Hi James,

Thanks for reviewing the patch and the modifications.
 
>-----Original Message-----
>From: James Morse [mailto:james.morse@arm.com]
>Sent: 18 June 2020 19:20
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; bp@alien8.de; lenb@kernel.org;
>tony.luck@intel.com; dan.carpenter@oracle.com;
>zhangliguang@linux.alibaba.com; andriy.shevchenko@linux.intel.com;
>Wangkefeng (OS Kernel Lab) <wangkefeng.wang@huawei.com>;
>jroedel@suse.de; yangyicong <yangyicong@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>
>Subject: Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor
>specific HW errors
>
>Hi Shiju,
>
>On 15/06/2020 10:53, Shiju Jose wrote:
>> Add support to notify the vendor specific non-fatal HW errors to the
>> drivers for the error recovery.
>
>This doesn't apply cleanly to v5.8-rc1... thanks for waiting for the merge
>window to finish, but please rebase onto the latest and greatest kernel!

V10 was posted based on v5.8-rc1.
>
>I'm glad the notifier chains for stuff that should be built-in has gone.
>(In my opinion, the RAS code should be moving in the direction of having less
>code run between being told of an error, and the handler running. Notifier
>chains for things like memory-errors was moving in the wrong direction!)
>
>
>The Kfifo and pool are adding complexity I don't think you need.
>Please make it clear from the naming this is for vendor records. (what is an
>event?)
>
>The memcpy() for the records is annoying, but eliminating it takes some
>really invasive changes. Lets live with it for now.
Ok.

>
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
>> 24c9642e8fc7..854d8115cdfc 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -63,6 +64,11 @@
>>  #define GHES_ESTATUS_CACHES_SIZE	4
>>
>>  #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
>> +
>> +#define GHES_EVENT_RING_SIZE	256
>> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
>> +#define GHES_GDATA_POOL_MIN_SIZE	65536
>
>Huh. Another pool of memory, and we don't know if this will ever be used.
>Can we allocate from ghes_estatus_pool instead?
>
>ghes_estatus_pool is already scaled with the number of error sources
>firmware describes in ghes_estatus_pool_init(), so it should be big enough.
>
>ghes_estatus_pool already has multiple users, estatus_nodes for work
>deferred from NMI come from here, as do ghes_estatus_caches for the low-
>pass filter thing.

Ok.
>
>
>> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>>   */
>>  static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>>
>> +struct ghes_event_entry {
>
>ghes_vendor_record_entry ?
>
>> +	struct acpi_hest_generic_data *gdata;
>> +	int error_severity;
>> +};
>
>> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
>> +		    GHES_EVENT_RING_SIZE);
>> +
>> +static DEFINE_SPINLOCK(ghes_event_ring_lock);
>
>Do you need the FIFO behaviour?
>If you put a work_struct in the struct and schedule_work() that, these would
>run in any order, and it would be less code.
>
>
>> +static struct gen_pool *ghes_gdata_pool; static unsigned long
>> +ghes_gdata_pool_size_request;
>> +
>>  static struct gen_pool *ghes_estatus_pool;  static unsigned long
>> ghes_estatus_pool_size_request;
>
>Please use the existing ghes_estatus_pool.
>
>
>> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
>
>[...]
>
>> +static int ghes_gdata_pool_init(void) {
>> +	unsigned long addr, len;
>> +	int rc;
>> +
>> +	ghes_gdata_pool =
>gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
>> +	if (!ghes_gdata_pool)
>> +		return -ENOMEM;
>> +
>> +	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
>> +		ghes_gdata_pool_size_request =
>GHES_GDATA_POOL_MIN_SIZE;
>> +
>> +	len = ghes_gdata_pool_size_request;
>> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> +	if (!addr)
>> +		goto err_pool_alloc;
>
>> +	vmalloc_sync_mappings();
>(This isn't needed anymore. See commit 73f693c3a705 ("mm: remove
>vmalloc_sync_(un)mappings()"))
>
>
>> +	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
>> +	if (rc)
>> +		goto err_pool_add;
>> +
>> +	return 0;
>> +
>> +err_pool_add:
>> +	vfree((void *)addr);
>> +
>> +err_pool_alloc:
>> +	gen_pool_destroy(ghes_gdata_pool);
>> +
>> +	return -ENOMEM;
>> +}
>
>But: using ghes_estatus_pool would avoid this duplication.
>
>
>> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct
>acpi_hest_generic *generic)
>>  		goto err_unmap_status_addr;
>>  	}
>>
>> +	ghes_gdata_pool_size_request += generic->records_to_preallocate *
>> +					generic->max_sections_per_record *
>> +					generic->max_raw_data_length;
>> +
>
>Careful, I think ghes_probe() can run in parallel on different CPUs. You can
>certainly unbind/rebind it from user-space.
>
>I recall these max this/that/preallocate stuff are junk values on some
>platform.
>You'd at least need to cap it to sane maximum value.
>
>But: Using ghes_estatus_pool would use ghes_estatus_pool_init()'s sizes,
>which allocates 64K for each error source.
>
>History: https://www.spinics.net/lists/linux-acpi/msg84238.html
>
>
>> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct
>> acpi_hest_generic_data *gdata)
>
>[...]
>
>> +static void ghes_event_work_func(struct work_struct *work) {
>> +	struct ghes_event_entry entry;
>> +	u32 len;
>> +
>> +	while (kfifo_get(&ghes_event_ring, &entry)) {
>> +		blocking_notifier_call_chain(&ghes_event_notify_list,
>> +					     entry.error_severity,
>> +					     entry.gdata);
>> +		len = acpi_hest_get_record_size(entry.gdata);
>> +		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata,
>len);
>> +	}
>> +}
>> +
>> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
>> +
>> +static void ghes_handle_non_standard_event(struct
>acpi_hest_generic_data *gdata,
>> +					   int sev)
>> +{
>> +	u32 len;
>
>> +	struct ghes_event_entry event_entry;
>
>> +	len = acpi_hest_get_record_size(gdata);
>> +	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
>> +	if (event_entry.gdata) {
>> +		memcpy(event_entry.gdata, gdata, len);
>> +		event_entry.error_severity = sev;
>> +
>> +		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
>
>... event_entry is on the stack ...
>
Ok.

>
>> +					&ghes_event_ring_lock))
>> +			schedule_work(&ghes_event_work);
>> +		else
>> +			pr_warn(GHES_PFX "ghes event queue full\n");
>> +	}
>> +}
>
>
>I think the kfifo is adding un-needed complexity here.
Ok.
>
>
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
>> e3f1cddb4ac8..a3dd82069069 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -50,6 +50,34 @@ enum {
>
[...]
>> +void ghes_unregister_event_notifier(struct notifier_block *nb); #else
>
>Please make it clear from the names these are for vendor events, that the
>kernel would otherwise ignore. It looks like these are for everything. Drivers
>have no business trying to handle the errors that are handled by things like
>memory_failure().
>
>~
>
>I would post a version of this to illustrate, but there are comments on patch 2
>too.
>
>Something like:
>http://www.linux-arm.org/git?p=linux-
>jm.git;a=commitdiff;h=9c6859f3146001cd9f8edfaf965232cb99c7dc42
>
>(caveat emptor: I've only build tested it)

I tested your changes and worked fine.
Should I send this patch along with the updated patch 2?

>
>
>Thanks,
>
>James

Thanks,
Shiju