Presently kernel does not support reporting the vendor specific HW errors,
in the non-standard format, to the vendor drivers for the recovery.
This patch set add this support and also move the existing handler
functions for the standard errors to the new callback method.
Also the CCIX RAS patches could be move to the proposed callback method.
https://www.spinics.net/lists/linux-edac/msg10508.html
https://patchwork.kernel.org/patch/10979491/
Shiju Jose (4):
ACPI: APEI: Add support to notify the vendor specific HW errors
ACPI: APEI: Add ghes_handle_memory_failure to the new notification
method
ACPI: APEI: Add ghes_handle_aer to the new notification method
ACPI: APEI: Add log_arm_hw_error to the new notification method
drivers/acpi/apei/ghes.c | 170 +++++++++++++++++++++++++++++++++++++++++------
drivers/ras/ras.c | 5 +-
include/acpi/ghes.h | 47 +++++++++++++
include/linux/ras.h | 7 +-
4 files changed, 205 insertions(+), 24 deletions(-)
--
1.9.1
Presently the vendor specific HW errors, in the non-standard format,
are not reported to the vendor drivers for the recovery.
This patch adds support to notify the vendor specific HW errors to the
registered kernel drivers.
Signed-off-by: Shiju Jose <[email protected]>
---
drivers/acpi/apei/ghes.c | 118 +++++++++++++++++++++++++++++++++++++++++++++--
include/acpi/ghes.h | 47 +++++++++++++++++++
2 files changed, 160 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index a66e00f..374d197 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -477,6 +477,77 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
#endif
}
+struct ghes_error_notify {
+ struct list_head list;
+ struct rcu_head rcu_head;
+ guid_t sec_type; /* guid of the error record */
+ error_handle handle; /* error handler function */
+ void *data; /* handler driver's private data if any */
+};
+
+/* List to store the registered error handling functions */
+static DEFINE_MUTEX(ghes_error_notify_mutex);
+static LIST_HEAD(ghes_error_notify_list);
+static refcount_t ghes_ref_count;
+
+/**
+ * ghes_error_notify_register - register an error handling function
+ * for the hw errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @handle: pointer to the error handling function.
+ * @data: handler driver's private data.
+ *
+ * return 0 : SUCCESS, non-zero : FAIL
+ */
+int ghes_error_notify_register(guid_t sec_type, error_handle handle, void *data)
+{
+ struct ghes_error_notify *err_notify;
+
+ mutex_lock(&ghes_error_notify_mutex);
+ err_notify = kzalloc(sizeof(*err_notify), GFP_KERNEL);
+ if (!err_notify)
+ return -ENOMEM;
+
+ err_notify->handle = handle;
+ guid_copy(&err_notify->sec_type, &sec_type);
+ err_notify->data = data;
+ list_add_rcu(&err_notify->list, &ghes_error_notify_list);
+ mutex_unlock(&ghes_error_notify_mutex);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ghes_error_notify_register);
+
+/**
+ * ghes_error_notify_unregister - unregister an error handling function.
+ * @sec_type: sec_type of the corresponding CPER.
+ * @handle: pointer to the error handling function.
+ *
+ * return none.
+ */
+void ghes_error_notify_unregister(guid_t sec_type, error_handle handle)
+{
+ struct ghes_error_notify *err_notify;
+ bool found = 0;
+
+ mutex_lock(&ghes_error_notify_mutex);
+ rcu_read_lock();
+ list_for_each_entry_rcu(err_notify, &ghes_error_notify_list, list) {
+ if (guid_equal(&err_notify->sec_type, &sec_type) &&
+ err_notify->handle == handle) {
+ list_del_rcu(&err_notify->list);
+ found = 1;
+ break;
+ }
+ }
+ rcu_read_unlock();
+ synchronize_rcu();
+ mutex_unlock(&ghes_error_notify_mutex);
+ if (found)
+ kfree(err_notify);
+}
+EXPORT_SYMBOL_GPL(ghes_error_notify_unregister);
+
static void ghes_do_proc(struct ghes *ghes,
const struct acpi_hest_generic_status *estatus)
{
@@ -485,6 +556,8 @@ static void ghes_do_proc(struct ghes *ghes,
guid_t *sec_type;
guid_t *fru_id = &NULL_UUID_LE;
char *fru_text = "";
+ bool is_notify = 0;
+ struct ghes_error_notify *err_notify;
sev = ghes_severity(estatus->error_severity);
apei_estatus_for_each_section(estatus, gdata) {
@@ -512,11 +585,29 @@ static void ghes_do_proc(struct ghes *ghes,
log_arm_hw_error(err);
} else {
- void *err = acpi_hest_get_payload(gdata);
-
- log_non_standard_event(sec_type, fru_id, fru_text,
- sec_sev, err,
- gdata->error_data_length);
+ rcu_read_lock();
+ list_for_each_entry_rcu(err_notify,
+ &ghes_error_notify_list, list) {
+ if (guid_equal(&err_notify->sec_type,
+ sec_type)) {
+ /* The notification is called in the
+ * interrupt context, thus the handler
+ * functions should be take care of it.
+ */
+ err_notify->handle(gdata, sev,
+ err_notify->data);
+ is_notify = 1;
+ }
+ }
+ rcu_read_unlock();
+
+ if (!is_notify) {
+ void *err = acpi_hest_get_payload(gdata);
+
+ log_non_standard_event(sec_type, fru_id,
+ fru_text, sec_sev, err,
+ gdata->error_data_length);
+ }
}
}
}
@@ -1217,6 +1308,11 @@ static int ghes_probe(struct platform_device *ghes_dev)
ghes_edac_register(ghes, &ghes_dev->dev);
+ if (!refcount_read(&ghes_ref_count))
+ refcount_set(&ghes_ref_count, 1);
+ else
+ refcount_inc(&ghes_ref_count);
+
/* Handle any pending errors right away */
spin_lock_irqsave(&ghes_notify_lock_irq, flags);
ghes_proc(ghes);
@@ -1237,6 +1333,7 @@ static int ghes_remove(struct platform_device *ghes_dev)
int rc;
struct ghes *ghes;
struct acpi_hest_generic *generic;
+ struct ghes_error_notify *err_notify, *tmp;
ghes = platform_get_drvdata(ghes_dev);
generic = ghes->generic;
@@ -1279,6 +1376,17 @@ static int ghes_remove(struct platform_device *ghes_dev)
ghes_fini(ghes);
+ if (refcount_dec_and_test(&ghes_ref_count) &&
+ !list_empty(&ghes_error_notify_list)) {
+ mutex_lock(&ghes_error_notify_mutex);
+ list_for_each_entry_safe(err_notify, tmp,
+ &ghes_error_notify_list, list) {
+ list_del_rcu(&err_notify->list);
+ kfree_rcu(err_notify, rcu_head);
+ }
+ mutex_unlock(&ghes_error_notify_mutex);
+ }
+
ghes_edac_unregister(ghes);
kfree(ghes);
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index e3f1cdd..d480537 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -50,6 +50,53 @@ enum {
GHES_SEV_PANIC = 0x3,
};
+/**
+ * error_handle - error handling function for the hw errors.
+ * This handle function is called in the interrupt context.
+ * @gdata: acpi_hest_generic_data.
+ * @sev: error severity of the entire error event defined in the
+ * ACPI spec table generic error status block.
+ * @data: handler driver's private data.
+ *
+ * return : none.
+ */
+typedef void (*error_handle)(struct acpi_hest_generic_data *gdata, int sev,
+ void *data);
+
+#ifdef CONFIG_ACPI_APEI_GHES
+/**
+ * ghes_error_notify_register - register an error handling function
+ * for the hw errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @handle: pointer to the error handling function.
+ * @data: handler driver's private data.
+ *
+ * return : 0 - SUCCESS, non-zero - FAIL.
+ */
+int ghes_error_notify_register(guid_t sec_type, error_handle handle,
+ void *data);
+
+/**
+ * ghes_error_notify_unregister - unregister an error handling function
+ * for the hw errors.
+ * @sec_type: sec_type of the corresponding CPER.
+ * @handle: pointer to the error handling function.
+ *
+ * return none.
+ */
+void ghes_error_notify_unregister(guid_t sec_type, error_handle handle);
+
+#else
+int ghes_error_notify_register(guid_t sec_type, error_handle handle, void *data)
+{
+ return -ENODEV;
+}
+
+void ghes_error_notify_unregister(guid_t sec_type, error_handle handle)
+{
+}
+#endif
+
int ghes_estatus_pool_init(int num_ghes);
/* From drivers/edac/ghes_edac.c */
--
1.9.1
This patch adds ghes_handle_memory_failure to the new error
notification method.
Signed-off-by: Shiju Jose <[email protected]>
---
drivers/acpi/apei/ghes.c | 51 ++++++++++++++++++++++++++++++++++--------------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 374d197..4400d56 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -401,14 +401,18 @@ static void ghes_clear_estatus(struct ghes *ghes,
ghes_ack_error(ghes->generic_v2);
}
-static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
+static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
+ int sev, void *data)
{
-#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
+ struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
+ int sec_sev = ghes_severity(gdata->error_severity);
unsigned long pfn;
int flags = -1;
- int sec_sev = ghes_severity(gdata->error_severity);
- struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
+ ghes_edac_report_mem_error(sev, mem_err);
+ arch_apei_report_mem_error(sev, mem_err);
+
+#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
return;
@@ -569,15 +573,7 @@ static void ghes_do_proc(struct ghes *ghes,
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
fru_text = gdata->fru_text;
- if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
- struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
-
- ghes_edac_report_mem_error(sev, mem_err);
-
- arch_apei_report_mem_error(sev, mem_err);
- ghes_handle_memory_failure(gdata, sev);
- }
- else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
+ if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
ghes_handle_aer(gdata);
}
else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
@@ -1190,11 +1186,25 @@ static int apei_sdei_unregister_ghes(struct ghes *ghes)
return sdei_unregister_ghes(ghes);
}
+struct ghes_err_handler_tab {
+ guid_t sec_type;
+ error_handle handle;
+};
+
+static struct ghes_err_handler_tab handler_tab[] = {
+ {
+ .sec_type = CPER_SEC_PLATFORM_MEM,
+ .handle = ghes_handle_memory_failure,
+ },
+ { /* sentinel */ }
+};
+
static int ghes_probe(struct platform_device *ghes_dev)
{
struct acpi_hest_generic *generic;
struct ghes *ghes = NULL;
unsigned long flags;
+ int i;
int rc = -EINVAL;
@@ -1308,9 +1318,20 @@ static int ghes_probe(struct platform_device *ghes_dev)
ghes_edac_register(ghes, &ghes_dev->dev);
- if (!refcount_read(&ghes_ref_count))
+ if (!refcount_read(&ghes_ref_count)) {
refcount_set(&ghes_ref_count, 1);
- else
+ /* register handler functions for the standard errors.
+ * This may be done from the corresponding drivers.
+ */
+ for (i = 0; handler_tab[i].handle; i++) {
+ if (ghes_error_notify_register(handler_tab[i].sec_type,
+ handler_tab[i].handle, NULL)) {
+ ghes_edac_unregister(ghes);
+ platform_set_drvdata(ghes_dev, NULL);
+ goto err;
+ }
+ }
+ } else
refcount_inc(&ghes_ref_count);
/* Handle any pending errors right away */
--
1.9.1
This patch adds ghes_handle_aer to the new error notification method.
Signed-off-by: Shiju Jose <[email protected]>
---
drivers/acpi/apei/ghes.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 4400d56..ffc309c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -450,7 +450,8 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
* GHES_SEV_PANIC does not make it to this handling since the kernel must
* panic.
*/
-static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
+static void ghes_handle_aer(struct acpi_hest_generic_data *gdata,
+ int sev, void *data)
{
#ifdef CONFIG_ACPI_APEI_PCIEAER
struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
@@ -573,10 +574,7 @@ static void ghes_do_proc(struct ghes *ghes,
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
fru_text = gdata->fru_text;
- if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
- ghes_handle_aer(gdata);
- }
- else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
+ if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
log_arm_hw_error(err);
@@ -1196,6 +1194,10 @@ struct ghes_err_handler_tab {
.sec_type = CPER_SEC_PLATFORM_MEM,
.handle = ghes_handle_memory_failure,
},
+ {
+ .sec_type = CPER_SEC_PCIE,
+ .handle = ghes_handle_aer,
+ },
{ /* sentinel */ }
};
--
1.9.1
This patch adds log_arm_hw_error to the new error notification
method.
Signed-off-by: Shiju Jose <[email protected]>
---
drivers/acpi/apei/ghes.c | 47 ++++++++++++++++++++++-------------------------
drivers/ras/ras.c | 5 ++++-
include/linux/ras.h | 7 +++++--
3 files changed, 31 insertions(+), 28 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index ffc309c..013fea0 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -574,34 +574,27 @@ static void ghes_do_proc(struct ghes *ghes,
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
fru_text = gdata->fru_text;
- if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
- struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
-
- log_arm_hw_error(err);
- } else {
- rcu_read_lock();
- list_for_each_entry_rcu(err_notify,
- &ghes_error_notify_list, list) {
- if (guid_equal(&err_notify->sec_type,
- sec_type)) {
- /* The notification is called in the
- * interrupt context, thus the handler
- * functions should be take care of it.
- */
- err_notify->handle(gdata, sev,
- err_notify->data);
- is_notify = 1;
- }
+ rcu_read_lock();
+ list_for_each_entry_rcu(err_notify, &ghes_error_notify_list,
+ list) {
+ if (guid_equal(&err_notify->sec_type, sec_type)) {
+ /* The notification is called in the
+ * interrupt context, thus the handler
+ * functions should be take care of it.
+ */
+ err_notify->handle(gdata, sev,
+ err_notify->data);
+ is_notify = 1;
}
- rcu_read_unlock();
+ }
+ rcu_read_unlock();
- if (!is_notify) {
- void *err = acpi_hest_get_payload(gdata);
+ if (!is_notify) {
+ void *err = acpi_hest_get_payload(gdata);
- log_non_standard_event(sec_type, fru_id,
- fru_text, sec_sev, err,
- gdata->error_data_length);
- }
+ log_non_standard_event(sec_type, fru_id,
+ fru_text, sec_sev, err,
+ gdata->error_data_length);
}
}
}
@@ -1198,6 +1191,10 @@ struct ghes_err_handler_tab {
.sec_type = CPER_SEC_PCIE,
.handle = ghes_handle_aer,
},
+ {
+ .sec_type = CPER_SEC_PROC_ARM,
+ .handle = log_arm_hw_error,
+ },
{ /* sentinel */ }
};
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index 95540ea..7ec3eeb 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -21,8 +21,11 @@ void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id,
trace_non_standard_event(sec_type, fru_id, fru_text, sev, err, len);
}
-void log_arm_hw_error(struct cper_sec_proc_arm *err)
+void log_arm_hw_error(struct acpi_hest_generic_data *gdata,
+ int sev, void *data)
{
+ struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
+
trace_arm_event(err);
}
diff --git a/include/linux/ras.h b/include/linux/ras.h
index 7c3debb..05b662d 100644
--- a/include/linux/ras.h
+++ b/include/linux/ras.h
@@ -5,6 +5,7 @@
#include <asm/errno.h>
#include <linux/uuid.h>
#include <linux/cper.h>
+#include <acpi/ghes.h>
#ifdef CONFIG_DEBUG_FS
int ras_userspace_consumers(void);
@@ -29,7 +30,8 @@ static inline void __init cec_init(void) { }
void log_non_standard_event(const guid_t *sec_type,
const guid_t *fru_id, const char *fru_text,
const u8 sev, const u8 *err, const u32 len);
-void log_arm_hw_error(struct cper_sec_proc_arm *err);
+void log_arm_hw_error(struct acpi_hest_generic_data *gdata,
+ int sev, void *data);
#else
static inline void
log_non_standard_event(const guid_t *sec_type,
@@ -37,7 +39,8 @@ void log_non_standard_event(const guid_t *sec_type,
const u8 sev, const u8 *err, const u32 len)
{ return; }
static inline void
-log_arm_hw_error(struct cper_sec_proc_arm *err) { return; }
+log_arm_hw_error(struct acpi_hest_generic_data *gdata,
+ int sev, void *data) { return; }
#endif
#endif /* __RAS_H__ */
--
1.9.1
Hi,
On 12/08/2019 11:11, Shiju Jose wrote:
> Presently kernel does not support reporting the vendor specific HW errors,
> in the non-standard format, to the vendor drivers for the recovery.
'non standard' here is probably a little jarring to the casual reader. You're referring to
the UEFI spec's "N.2.3 Non-standard Section Body", which refers to any section type
published somewhere other than the UEFI spec.
These still have to have a GUID to identify them, so they still have the same section
header format.
> This patch set add this support and also move the existing handler
> functions for the standard errors to the new callback method.
Could you give an example of where this would be useful? You're adding an API with no
caller to justify its existence.
GUIDs should only belong to one driver.
I don't think we should call drivers for something described as a fatal error. (which is
the case with what you have here)
> Also the CCIX RAS patches could be move to the proposed callback method.
Presumably for any vendor-specific stuff?
Thanks,
James
Hi,
On 12/08/2019 11:11, Shiju Jose wrote:
> Presently the vendor specific HW errors, in the non-standard format,
> are not reported to the vendor drivers for the recovery.
>
> This patch adds support to notify the vendor specific HW errors to the
> registered kernel drivers.
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a66e00f..374d197 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -477,6 +477,77 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
> #endif
> }
>
> +struct ghes_error_notify {
> + struct list_head list;> + struct rcu_head rcu_head;
> + guid_t sec_type; /* guid of the error record */
> + error_handle handle; /* error handler function */
ghes_error_handler_t error_handler; ?
> + void *data; /* handler driver's private data if any */
> +};
> +
> +/* List to store the registered error handling functions */
> +static DEFINE_MUTEX(ghes_error_notify_mutex);
> +static LIST_HEAD(ghes_error_notify_list);
> +static refcount_t ghes_ref_count;
I don't think this refcount is needed.
> +/**
> + * ghes_error_notify_register - register an error handling function
> + * for the hw errors.
> + * @sec_type: sec_type of the corresponding CPER to be notified.
> + * @handle: pointer to the error handling function.
> + * @data: handler driver's private data.
> + *
> + * return 0 : SUCCESS, non-zero : FAIL
> + */
> +int ghes_error_notify_register(guid_t sec_type, error_handle handle, void *data)
> +{
> + struct ghes_error_notify *err_notify;
> +
> + mutex_lock(&ghes_error_notify_mutex);
> + err_notify = kzalloc(sizeof(*err_notify), GFP_KERNEL);
> + if (!err_notify)
> + return -ENOMEM;
Leaving the mutex locked.
You may as well allocate the memory before taking the lock.
> +
> + err_notify->handle = handle;
> + guid_copy(&err_notify->sec_type, &sec_type);
> + err_notify->data = data;
> + list_add_rcu(&err_notify->list, &ghes_error_notify_list);
> + mutex_unlock(&ghes_error_notify_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ghes_error_notify_register);
Could we leave exporting this to modules until there is a user?
> +/**
> + * ghes_error_notify_unregister - unregister an error handling function.
> + * @sec_type: sec_type of the corresponding CPER.
> + * @handle: pointer to the error handling function.
> + *
> + * return none.
> + */
> +void ghes_error_notify_unregister(guid_t sec_type, error_handle handle)
Why do we need the handle(r) a second time? Surely there can only be one callback for a
given guid.
> +{
> + struct ghes_error_notify *err_notify;
> + bool found = 0;
> +
> + mutex_lock(&ghes_error_notify_mutex);
> + rcu_read_lock();
> + list_for_each_entry_rcu(err_notify, &ghes_error_notify_list, list) {
> + if (guid_equal(&err_notify->sec_type, &sec_type) &&
> + err_notify->handle == handle) {
> + list_del_rcu(&err_notify->list);
> + found = 1;
> + break;
> + }
> + }
> + rcu_read_unlock();
> + synchronize_rcu();
Is this for the kfree()? Please keep them together so its obvious what its for.
Putting it outside the mutex will also save any contended waiter some time.
> + mutex_unlock(&ghes_error_notify_mutex);
> + if (found)
> + kfree(err_notify);
> +}
> +EXPORT_SYMBOL_GPL(ghes_error_notify_unregister);
> +
> static void ghes_do_proc(struct ghes *ghes,
> const struct acpi_hest_generic_status *estatus)
> {> @@ -512,11 +585,29 @@ static void ghes_do_proc(struct ghes *ghes,
>
> log_arm_hw_error(err);
> } else {
> - void *err = acpi_hest_get_payload(gdata);
> -
> - log_non_standard_event(sec_type, fru_id, fru_text,
> - sec_sev, err,
> - gdata->error_data_length);
> + rcu_read_lock();
> + list_for_each_entry_rcu(err_notify,
> + &ghes_error_notify_list, list) {
> + if (guid_equal(&err_notify->sec_type,
> + sec_type)) {
> + /* The notification is called in the
> + * interrupt context, thus the handler
> + * functions should be take care of it.
> + */
I read this as "the handler will be called", which doesn't seem to be a useful comment.
> + err_notify->handle(gdata, sev,
> + err_notify->data);
> + is_notify = 1;
break;
> + }
> + }
> + rcu_read_unlock();
> + if (!is_notify) {
if (!found) Seems more natural.
> + void *err = acpi_hest_get_payload(gdata);
> +
> + log_non_standard_event(sec_type, fru_id,
> + fru_text, sec_sev, err,
> + gdata->error_data_length);
> + }
This is tricky to read as its so bunched up. Please pull it out into a separate function.
ghes_handle_non_standard_event() ?
Because you skip log_non_standard_event(), rasdaemon will no longer see these in
user-space. For any kernel consumer of these, we need to know we aren't breaking the
user-space component.
> }
> }
> }
> @@ -1217,6 +1308,11 @@ static int ghes_probe(struct platform_device *ghes_dev)
>
> ghes_edac_register(ghes, &ghes_dev->dev);
>
> + if (!refcount_read(&ghes_ref_count))
> + refcount_set(&ghes_ref_count, 1);
What stops this from racing with itself if two ghes platform devices are probed at the
same time?
If the refcount needs initialising, please do it in ghes_init()....
> + else
> + refcount_inc(&ghes_ref_count);
.. but I don't think this refcount is needed.
> /* Handle any pending errors right away */
> spin_lock_irqsave(&ghes_notify_lock_irq, flags);
> ghes_proc(ghes);
> @@ -1279,6 +1376,17 @@ static int ghes_remove(struct platform_device *ghes_dev)
>
> ghes_fini(ghes);
>
> + if (refcount_dec_and_test(&ghes_ref_count) &&
> + !list_empty(&ghes_error_notify_list)) {
> + mutex_lock(&ghes_error_notify_mutex);> + list_for_each_entry_safe(err_notify, tmp,
> + &ghes_error_notify_list, list) {
> + list_del_rcu(&err_notify->list);
> + kfree_rcu(err_notify, rcu_head);
> + }
> + mutex_unlock(&ghes_error_notify_mutex);
> + }
... If someone unregisters, and re-registers all the GHES platform devices, the last one
out flushes the vendor-specific error handlers away. Then we re-probe the devices again,
but this time the vendor-specific error handlers don't work.
As you have an add/remove API for drivers, its up to drivers to cleanup when they are
removed. The comings and goings of GHES platform devices isn't relevant.
> ghes_edac_unregister(ghes);
>
> kfree(ghes);
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index e3f1cdd..d480537 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -50,6 +50,53 @@ enum {
> GHES_SEV_PANIC = 0x3,
> };
>
> +/**
> + * error_handle - error handling function for the hw errors.
Fatal errors get dealt with earlier, so drivers will never see them.
| error handling function for non-fatal hardware errors.
> + * This handle function is called in the interrupt context.
As this overrides ghes's logging of the error, we should mention:
| The handler is responsible for any logging of the error.
> + * @gdata: acpi_hest_generic_data.
> + * @sev: error severity of the entire error event defined in the
> + * ACPI spec table generic error status block.
> + * @data: handler driver's private data.
> + *
> + * return : none.
> + */
> +typedef void (*error_handle)(struct acpi_hest_generic_data *gdata, int sev,
> + void *data);
Thanks,
James
Hi,
On 12/08/2019 11:11, Shiju Jose wrote:
> This patch adds ghes_handle_memory_failure to the new error
> notification method.
The commit message doesn't answer the question: why?
The existing code works. This just looks like additional churn.
Given a user, I think the vendor specific example is useful. I don't think making this
thing more pluggable is a good idea.
Thanks,
James
Hi James,
>-----Original Message-----
>From: James Morse [mailto:[email protected]]
>Sent: 21 August 2019 18:24
>To: Shiju Jose <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>Linuxarm <[email protected]>; Jonathan Cameron
><[email protected]>; tanxiaofei <[email protected]>
>Subject: Re: [PATCH RFC 1/4] ACPI: APEI: Add support to notify the vendor
>specific HW errors
>
>Hi,
>
>On 12/08/2019 11:11, Shiju Jose wrote:
>> Presently the vendor specific HW errors, in the non-standard format,
>> are not reported to the vendor drivers for the recovery.
>>
>> This patch adds support to notify the vendor specific HW errors to the
>> registered kernel drivers.
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
>> a66e00f..374d197 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -477,6 +477,77 @@ static void ghes_handle_aer(struct
>> acpi_hest_generic_data *gdata) #endif }
>>
>> +struct ghes_error_notify {
>> + struct list_head list;> + struct rcu_head rcu_head;
>> + guid_t sec_type; /* guid of the error record */
>
>> + error_handle handle; /* error handler function */
>
>ghes_error_handler_t error_handler; ?
Sure.
>
>
>> + void *data; /* handler driver's private data if any */ };
>> +
>> +/* List to store the registered error handling functions */ static
>> +DEFINE_MUTEX(ghes_error_notify_mutex);
>> +static LIST_HEAD(ghes_error_notify_list);
>
>> +static refcount_t ghes_ref_count;
>
>I don't think this refcount is needed.
refcount was added to register standard error handlers with this notification method one time when
multiple ghes platform devices are probed.
>
>
>> +/**
>> + * ghes_error_notify_register - register an error handling function
>> + * for the hw errors.
>> + * @sec_type: sec_type of the corresponding CPER to be notified.
>> + * @handle: pointer to the error handling function.
>> + * @data: handler driver's private data.
>> + *
>> + * return 0 : SUCCESS, non-zero : FAIL */ int
>> +ghes_error_notify_register(guid_t sec_type, error_handle handle, void
>> +*data) {
>> + struct ghes_error_notify *err_notify;
>> +
>> + mutex_lock(&ghes_error_notify_mutex);
>> + err_notify = kzalloc(sizeof(*err_notify), GFP_KERNEL);
>> + if (!err_notify)
>> + return -ENOMEM;
>
>Leaving the mutex locked.
>You may as well allocate the memory before taking the lock.
Good spot. I will fix.
>
>
>> +
>> + err_notify->handle = handle;
>> + guid_copy(&err_notify->sec_type, &sec_type);
>> + err_notify->data = data;
>> + list_add_rcu(&err_notify->list, &ghes_error_notify_list);
>> + mutex_unlock(&ghes_error_notify_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(ghes_error_notify_register);
>
>Could we leave exporting this to modules until there is a user?
>
>
>> +/**
>> + * ghes_error_notify_unregister - unregister an error handling function.
>> + * @sec_type: sec_type of the corresponding CPER.
>> + * @handle: pointer to the error handling function.
>> + *
>> + * return none.
>> + */
>> +void ghes_error_notify_unregister(guid_t sec_type, error_handle
>> +handle)
>
>Why do we need the handle(r) a second time? Surely there can only be one
>callback for a given guid.
There is a possibility of sharing the guid between drivers if the non-standard error section format is common
for more than one devices if the error data to be reported is in the same format.
>
>
>> +{
>> + struct ghes_error_notify *err_notify;
>> + bool found = 0;
>> +
>> + mutex_lock(&ghes_error_notify_mutex);
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(err_notify, &ghes_error_notify_list, list) {
>> + if (guid_equal(&err_notify->sec_type, &sec_type) &&
>> + err_notify->handle == handle) {
>> + list_del_rcu(&err_notify->list);
>> + found = 1;
>> + break;
>> + }
>> + }
>> + rcu_read_unlock();
>
>> + synchronize_rcu();
>
>Is this for the kfree()? Please keep them together so its obvious what its for.
>Putting it outside the mutex will also save any contended waiter some time.
Yes. I will move synchronize_rcu () just before kfree.
>
>
>> + mutex_unlock(&ghes_error_notify_mutex);
>> + if (found)
>> + kfree(err_notify);
>> +}
>> +EXPORT_SYMBOL_GPL(ghes_error_notify_unregister);
>> +
>
>> static void ghes_do_proc(struct ghes *ghes,
>> const struct acpi_hest_generic_status *estatus) {>
>@@ -512,11
>> +585,29 @@ static void ghes_do_proc(struct ghes *ghes,
>>
>> log_arm_hw_error(err);
>> } else {
>> - void *err = acpi_hest_get_payload(gdata);
>> -
>> - log_non_standard_event(sec_type, fru_id, fru_text,
>> - sec_sev, err,
>> - gdata->error_data_length);
>
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(err_notify,
>> + &ghes_error_notify_list, list) {
>> + if (guid_equal(&err_notify->sec_type,
>> + sec_type)) {
>
>> + /* The notification is called in the
>> + * interrupt context, thus the handler
>> + * functions should be take care of it.
>> + */
>
>I read this as "the handler will be called", which doesn't seem to be a useful
>comment.
Ok. I will correct the comment.
>
>
>> + err_notify->handle(gdata, sev,
>> + err_notify->data);
>> + is_notify = 1;
>
> break;
>
>> + }
>> + }
>> + rcu_read_unlock();
>
>> + if (!is_notify) {
>
>if (!found) Seems more natural.
Ok. I will change to "is_notify" to "found".
>
>
>> + void *err = acpi_hest_get_payload(gdata);
>> +
>> + log_non_standard_event(sec_type, fru_id,
>> + fru_text, sec_sev, err,
>> + gdata->error_data_length);
>> + }
>
>This is tricky to read as its so bunched up. Please pull it out into a separate
>function.
>ghes_handle_non_standard_event() ?
Ok. I will add to new ghes_handle_non_standard_event() function.
>
>
>Because you skip log_non_standard_event(), rasdaemon will no longer see
>these in user-space. For any kernel consumer of these, we need to know we
>aren't breaking the user-space component.
>
>
>> }
>> }
>> }
>> @@ -1217,6 +1308,11 @@ static int ghes_probe(struct platform_device
>> *ghes_dev)
>>
>> ghes_edac_register(ghes, &ghes_dev->dev);
>>
>> + if (!refcount_read(&ghes_ref_count))
>> + refcount_set(&ghes_ref_count, 1);
>
>What stops this from racing with itself if two ghes platform devices are probed
>at the same time?
yes. It is an issue.
>
>If the refcount needs initialising, please do it in ghes_init()....
refcount was added to register the standard error handlers to the notification
method only for the first time when the ghes device probed multiple times.
I will check is it possible to avoid using refcount by moving the above registration
of standard error handlers to the ghes_init().
>
>> + else
>> + refcount_inc(&ghes_ref_count);
>
>.. but I don't think this refcount is needed.
>
>
>> /* Handle any pending errors right away */
>> spin_lock_irqsave(&ghes_notify_lock_irq, flags);
>> ghes_proc(ghes);
>
>> @@ -1279,6 +1376,17 @@ static int ghes_remove(struct platform_device
>> *ghes_dev)
>>
>> ghes_fini(ghes);
>>
>> + if (refcount_dec_and_test(&ghes_ref_count) &&
>> + !list_empty(&ghes_error_notify_list)) {
>> + mutex_lock(&ghes_error_notify_mutex);> +
> list_for_each_entry_safe(err_notify, tmp,
>> + &ghes_error_notify_list, list) {
>> + list_del_rcu(&err_notify->list);
>> + kfree_rcu(err_notify, rcu_head);
>> + }
>> + mutex_unlock(&ghes_error_notify_mutex);
>> + }
>
>... If someone unregisters, and re-registers all the GHES platform devices, the
>last one out flushes the vendor-specific error handlers away. Then we re-probe
>the devices again, but this time the vendor-specific error handlers don't work.
>
>As you have an add/remove API for drivers, its up to drivers to cleanup when
>they are removed. The comings and goings of GHES platform devices isn't
>relevant.
Ok. Got it. I will either keep the unregister for the standard error handlers only
if the standard error handlers can be part of the notification method or remove completely
if the registration can be done in the ghes_init().
>
>
>> ghes_edac_unregister(ghes);
>>
>> kfree(ghes);
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
>> e3f1cdd..d480537 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -50,6 +50,53 @@ enum {
>> GHES_SEV_PANIC = 0x3,
>> };
>>
>> +/**
>> + * error_handle - error handling function for the hw errors.
>
>Fatal errors get dealt with earlier, so drivers will never see them.
>| error handling function for non-fatal hardware errors.
Ok. I will change the comment as recoverable HW errors.
>
>
>> + * This handle function is called in the interrupt context.
>
>As this overrides ghes's logging of the error, we should mention:
>| The handler is responsible for any logging of the error.
Ok. I will add in the comment.
>
>
>> + * @gdata: acpi_hest_generic_data.
>> + * @sev: error severity of the entire error event defined in the
>> + * ACPI spec table generic error status block.
>> + * @data: handler driver's private data.
>> + *
>> + * return : none.
>> + */
>> +typedef void (*error_handle)(struct acpi_hest_generic_data *gdata, int sev,
>> + void *data);
>
>
>Thanks,
>
>James
Thanks,
Shiju
Hi James,
>-----Original Message-----
>From: James Morse [mailto:[email protected]]
>Sent: 21 August 2019 18:23
>To: Shiju Jose <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>Linuxarm <[email protected]>; Jonathan Cameron
><[email protected]>; tanxiaofei <[email protected]>
>Subject: Re: [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to
>the new notification method
>
>Hi,
>
>On 12/08/2019 11:11, Shiju Jose wrote:
>> This patch adds ghes_handle_memory_failure to the new error
>> notification method.
>
>The commit message doesn't answer the question: why?
>
>The existing code works. This just looks like additional churn.
>Given a user, I think the vendor specific example is useful. I don't think making
>this thing more pluggable is a good idea.
This was intended to replace the number of if(guid_equal(...)) else if(guid_equal(...)) checks in the ghes_do_proc() , which would grow when new UEFI defined error sections would be added in the future.
>
>
>Thanks,
>
>James
Thanks,
Shiju
Hi James,
Thanks for the feedback.
>-----Original Message-----
>From: [email protected] [mailto:linux-acpi-
>[email protected]] On Behalf Of James Morse
>Sent: 21 August 2019 18:23
>To: Shiju Jose <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>Linuxarm <[email protected]>; Jonathan Cameron
><[email protected]>; tanxiaofei <[email protected]>
>Subject: Re: [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor
>specific HW errors
>
>Hi,
>
>On 12/08/2019 11:11, Shiju Jose wrote:
>> Presently kernel does not support reporting the vendor specific HW
>> errors, in the non-standard format, to the vendor drivers for the recovery.
>
>'non standard' here is probably a little jarring to the casual reader. You're
>referring to the UEFI spec's "N.2.3 Non-standard Section Body", which refers to
>any section type published somewhere other than the UEFI spec.
OK. I will change it.
>
>These still have to have a GUID to identify them, so they still have the same
>section header format.
Yes.
>
>
>> This patch set add this support and also move the existing handler
>> functions for the standard errors to the new callback method.
>
>Could you give an example of where this would be useful? You're adding an API
>with no caller to justify its existence.
One such example is handling the local errors occurred in a device controller, such as PCIe.
>
>
>GUIDs should only belong to one driver.
UEFI spec's N.2.3 Non-standard Section Body mentioned, "The type (e.g. format) of a non-standard section is identified by the GUID populated in the Section Descriptor's Section Type field."
There is a possibility to define common non-standard error section format which will be used for more than one driver if the error data to be reported is in the same format. Then can the same GUID belong to multiple drivers?
>
>I don't think we should call drivers for something described as a fatal error.
>(which is the case with what you have here)
The notification is intended only for the recoverable errors as the ghes_proc() call panic for the fatal errors in the early stage.
>
>
>> Also the CCIX RAS patches could be move to the proposed callback method.
>
>Presumably for any vendor-specific stuff?
This information was related to the proposal to replace the number of if(guid_equal(...)) else if(guid_equal(...)) checks in the ghes_do_proc() for the existing UEFI spec defined error sections(such as PCIe, Memory, ARM HW error) by registering the corresponding handler functions to the proposed notification method. The same apply to the CCIX error sections and any other error sections defined by the UEFI spec in the future.
>
>
>Thanks,
>
>James
Thanks,
Shiju
Hi Shiju,
On 22/08/2019 17:56, Shiju Jose wrote:
> James Morse wrote:
>> On 12/08/2019 11:11, Shiju Jose wrote:
>>> Presently kernel does not support reporting the vendor specific HW
>>> errors, in the non-standard format, to the vendor drivers for the recovery.
>>
>> 'non standard' here is probably a little jarring to the casual reader. You're
>> referring to the UEFI spec's "N.2.3 Non-standard Section Body", which refers to
>> any section type published somewhere other than the UEFI spec.
>>> This patch set add this support and also move the existing handler
>>> functions for the standard errors to the new callback method.
>>
>> Could you give an example of where this would be useful? You're adding an API
>> with no caller to justify its existence.
> One such example is handling the local errors occurred in a device controller, such as PCIe.
Could we have the example in the form of patches? (sorry, I wasn't clear)
I don't think its realistic that a PCIe device driver would want to know about errors on
other devices in the system. (SAS-HBA meet the GPU).
PCIe's has AER for handling errors that (may have) occurred on a PCIe link, and this has
its own CPER records.
>> GUIDs should only belong to one driver.
> UEFI spec's N.2.3 Non-standard Section Body mentioned, "The type (e.g. format) of a
> non-standard section is identified by the GUID populated in the Section Descriptor's
> Section Type field."
> There is a possibility to define common non-standard error section format
I agree the GUID describes the format of the error record,
> which will
> be used for more than one driver if the error data to be reported is in the same format.
> Then can the same GUID belong to multiple drivers?
... but here we disagree.
CPER has a component/block-diagram view of the system. It describes a Memory error or an
error with a PCIe endpoint. An error record affects one component.
If you wanted to describe an error caused by a failed transaction between a PCIe device
and memory, you would need two of these records, and its guesswork as to what happened
between them.
But the PCIe device has no business poking around in the memory error. Even if it did APEI
would be the wrong place to do this as its not the only caller of memory_failure().
>>> Also the CCIX RAS patches could be move to the proposed callback method.
>>
>> Presumably for any vendor-specific stuff?
> This information was related to the proposal to replace the number of if(guid_equal(...)) else
> if(guid_equal(...)) checks in the ghes_do_proc() for the existing UEFI spec defined error
> sections(such as PCIe, Memory, ARM HW error)
'the standard ones'
> by registering the corresponding handler functions to the proposed notification method.
I really don't like this. Registering a handler for 'memory corruption' would require
walking a list of dynamically allocated pointers. Can there be more than one entry? Can
random drivers block memory_failure() while they allocate more memory to send packets over
USB? What if it loops?
For the standard error sources the kernel needs to run 'the' handler as quickly as
possible, with a minimum of code/memory-access in the meantime. It already takes too long.
Thanks,
James
> The same apply to the CCIX error sections and any other
> error sections defined by the UEFI spec in the future.