Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors
       [not found] <Shiju Jose>
@ 2019-08-12 10:11 ` Shiju Jose
  2019-08-12 10:11   ` [PATCH RFC 1/4] " Shiju Jose
                     ` (4 more replies)
  2020-01-15 11:01 ` [RFC PATCH 0/2] " Shiju Jose
  2020-01-24 12:39 ` [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2 siblings, 5 replies; 23+ messages in thread
From: Shiju Jose @ 2019-08-12 10:11 UTC (permalink / raw)
  To: linux-acpi, linux-edac, linux-kernel, rjw, lenb, james.morse,
	tony.luck, bp, baicar
  Cc: linuxarm, jonathan.cameron, tanxiaofei, Shiju Jose

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH RFC 1/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
@ 2019-08-12 10:11   ` " Shiju Jose
  2019-08-21 17:23     ` James Morse
  2019-08-12 10:11   ` [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method Shiju Jose
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 23+ messages in thread
From: Shiju Jose @ 2019-08-12 10:11 UTC (permalink / raw)
  To: linux-acpi, linux-edac, linux-kernel, rjw, lenb, james.morse,
	tony.luck, bp, baicar
  Cc: linuxarm, jonathan.cameron, tanxiaofei, Shiju Jose

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 <shiju.jose@huawei.com>
---
 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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2019-08-12 10:11   ` [PATCH RFC 1/4] " Shiju Jose
@ 2019-08-12 10:11   ` Shiju Jose
  2019-08-21 17:22     ` James Morse
  2019-08-12 10:11   ` [PATCH RFC 3/4] ACPI: APEI: Add ghes_handle_aer " Shiju Jose
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 23+ messages in thread
From: Shiju Jose @ 2019-08-12 10:11 UTC (permalink / raw)
  To: linux-acpi, linux-edac, linux-kernel, rjw, lenb, james.morse,
	tony.luck, bp, baicar
  Cc: linuxarm, jonathan.cameron, tanxiaofei, Shiju Jose

This patch adds ghes_handle_memory_failure to the new error
notification method.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH RFC 3/4] ACPI: APEI: Add ghes_handle_aer to the new notification method
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2019-08-12 10:11   ` [PATCH RFC 1/4] " Shiju Jose
  2019-08-12 10:11   ` [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method Shiju Jose
@ 2019-08-12 10:11   ` " Shiju Jose
  2019-08-12 10:11   ` [PATCH RFC 4/4] ACPI: APEI: Add log_arm_hw_error " Shiju Jose
  2019-08-21 17:22   ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors James Morse
  4 siblings, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2019-08-12 10:11 UTC (permalink / raw)
  To: linux-acpi, linux-edac, linux-kernel, rjw, lenb, james.morse,
	tony.luck, bp, baicar
  Cc: linuxarm, jonathan.cameron, tanxiaofei, Shiju Jose

This patch adds ghes_handle_aer to the new error notification method.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH RFC 4/4] ACPI: APEI: Add log_arm_hw_error to the new notification method
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
                     ` (2 preceding siblings ...)
  2019-08-12 10:11   ` [PATCH RFC 3/4] ACPI: APEI: Add ghes_handle_aer " Shiju Jose
@ 2019-08-12 10:11   ` " Shiju Jose
  2019-08-21 17:22   ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors James Morse
  4 siblings, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2019-08-12 10:11 UTC (permalink / raw)
  To: linux-acpi, linux-edac, linux-kernel, rjw, lenb, james.morse,
	tony.luck, bp, baicar
  Cc: linuxarm, jonathan.cameron, tanxiaofei, Shiju Jose

This patch adds log_arm_hw_error to the new error notification
method.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
                     ` (3 preceding siblings ...)
  2019-08-12 10:11   ` [PATCH RFC 4/4] ACPI: APEI: Add log_arm_hw_error " Shiju Jose
@ 2019-08-21 17:22   ` James Morse
  2019-08-22 16:56     ` Shiju Jose
  4 siblings, 1 reply; 23+ messages in thread
From: James Morse @ 2019-08-21 17:22 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, linuxarm, jonathan.cameron, tanxiaofei

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method
  2019-08-12 10:11   ` [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method Shiju Jose
@ 2019-08-21 17:22     ` James Morse
  2019-08-22 16:57       ` Shiju Jose
  0 siblings, 1 reply; 23+ messages in thread
From: James Morse @ 2019-08-21 17:22 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, linuxarm, jonathan.cameron, tanxiaofei

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH RFC 1/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-12 10:11   ` [PATCH RFC 1/4] " Shiju Jose
@ 2019-08-21 17:23     ` James Morse
  2019-08-22 16:57       ` Shiju Jose
  0 siblings, 1 reply; 23+ messages in thread
From: James Morse @ 2019-08-21 17:23 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, linuxarm, jonathan.cameron, tanxiaofei

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-21 17:22   ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors James Morse
@ 2019-08-22 16:56     ` Shiju Jose
  2019-10-03 17:21       ` James Morse
  0 siblings, 1 reply; 23+ messages in thread
From: Shiju Jose @ 2019-08-22 16:56 UTC (permalink / raw)
  To: James Morse
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, Linuxarm, Jonathan Cameron, tanxiaofei

Hi James, 

Thanks for the feedback.

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of James Morse
>Sent: 21 August 2019 18:23
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org;
>tony.luck@intel.com; bp@alien8.de; baicar@os.amperecomputing.com;
>Linuxarm <linuxarm@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>
>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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method
  2019-08-21 17:22     ` James Morse
@ 2019-08-22 16:57       ` Shiju Jose
  0 siblings, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2019-08-22 16:57 UTC (permalink / raw)
  To: James Morse
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, Linuxarm, Jonathan Cameron, tanxiaofei

Hi James,

>-----Original Message-----
>From: James Morse [mailto:james.morse@arm.com]
>Sent: 21 August 2019 18:23
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org;
>tony.luck@intel.com; bp@alien8.de; baicar@os.amperecomputing.com;
>Linuxarm <linuxarm@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>
>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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH RFC 1/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-21 17:23     ` James Morse
@ 2019-08-22 16:57       ` Shiju Jose
  0 siblings, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2019-08-22 16:57 UTC (permalink / raw)
  To: James Morse
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, Linuxarm, Jonathan Cameron, tanxiaofei

Hi James,

>-----Original Message-----
>From: James Morse [mailto:james.morse@arm.com]
>Sent: 21 August 2019 18:24
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org;
>tony.luck@intel.com; bp@alien8.de; baicar@os.amperecomputing.com;
>Linuxarm <linuxarm@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>
>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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors
  2019-08-22 16:56     ` Shiju Jose
@ 2019-10-03 17:21       ` James Morse
  0 siblings, 0 replies; 23+ messages in thread
From: James Morse @ 2019-10-03 17:21 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-edac, linux-kernel, rjw, lenb, tony.luck, bp,
	baicar, Linuxarm, Jonathan Cameron, tanxiaofei

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.  



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors
       [not found] <Shiju Jose>
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
@ 2020-01-15 11:01 ` " Shiju Jose
  2020-01-15 11:01   ` [RFC PATCH 1/2] " Shiju Jose
  2020-01-15 11:01   ` [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors Shiju Jose
  2020-01-24 12:39 ` [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2 siblings, 2 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-15 11:01 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

Presently the vendor drivers are unable to do the recovery for the vendor
specific HW errors, reported to the APEI driver in the vendor defined sections,
because APEI driver does not support reporting the same to the vendor drivers.

This patch set
1. add an interface to the APEI driver to enable the vendor
drivers to register the event handling functions for the corresponding
vendor specific HW errors.

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

Changes from the previous version
1. Fix comments from James Morse.

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

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

Yicong Yang (1):
  PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's
    errors

 drivers/acpi/apei/ghes.c                       | 110 ++++++++-
 drivers/pci/controller/Kconfig                 |   8 +
 drivers/pci/controller/Makefile                |   1 +
 drivers/pci/controller/pcie-hisi-hip08-error.c | 323 +++++++++++++++++++++++++
 include/acpi/ghes.h                            |  49 ++++
 5 files changed, 486 insertions(+), 5 deletions(-)
 create mode 100644 drivers/pci/controller/pcie-hisi-hip08-error.c

-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] ACPI: APEI: Add support to notify the vendor specific HW errors
  2020-01-15 11:01 ` [RFC PATCH 0/2] " Shiju Jose
@ 2020-01-15 11:01   ` " Shiju Jose
  2020-01-15 11:01   ` [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors Shiju Jose
  1 sibling, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-15 11:01 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

Presently APEI does not support reporting the vendor specific
HW errors, received in the vendor defined table entries, to the
vendor drivers for any recovery.

This patch adds the support to register and unregister the
error handling function for the vendor specific HW errors and
notify the registered kernel driver.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/acpi/apei/ghes.c | 110 ++++++++++++++++++++++++++++++++++++++++++++---
 include/acpi/ghes.h      |  49 +++++++++++++++++++++
 2 files changed, 154 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 8906c80..3ba43b0 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -490,6 +490,103 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 #endif
 }
 
+struct ghes_event_notify {
+	struct list_head list;
+	struct rcu_head	rcu_head;
+	guid_t sec_type; /* guid of the error record */
+	ghes_event_handler_t event_handler; /* event handler function */
+	void *data; /* handler driver's private data if any */
+};
+
+/* List to store the registered event handling functions */
+static DEFINE_MUTEX(ghes_event_notify_mutex);
+static LIST_HEAD(ghes_event_handler_list);
+
+/**
+ * ghes_register_event_handler - register an event handling
+ * function for the non-fatal HW errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @event_handler: pointer to the error handling function.
+ * @data: handler driver's private data.
+ *
+ * return 0 : SUCCESS, non-zero : FAIL
+ */
+int ghes_register_event_handler(guid_t sec_type,
+				ghes_event_handler_t event_handler,
+				void *data)
+{
+	struct ghes_event_notify *event_notify;
+
+	event_notify = kzalloc(sizeof(*event_notify), GFP_KERNEL);
+	if (!event_notify)
+		return -ENOMEM;
+
+	event_notify->event_handler = event_handler;
+	guid_copy(&event_notify->sec_type, &sec_type);
+	event_notify->data = data;
+
+	mutex_lock(&ghes_event_notify_mutex);
+	list_add_rcu(&event_notify->list, &ghes_event_handler_list);
+	mutex_unlock(&ghes_event_notify_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ghes_register_event_handler);
+
+/**
+ * ghes_unregister_event_handler - unregister the previously
+ * registered event handling function.
+ * @sec_type: sec_type of the corresponding CPER.
+ */
+void ghes_unregister_event_handler(guid_t sec_type)
+{
+	struct ghes_event_notify *event_notify;
+	bool found = false;
+
+	mutex_lock(&ghes_event_notify_mutex);
+	rcu_read_lock();
+	list_for_each_entry_rcu(event_notify,
+				&ghes_event_handler_list, list) {
+		if (guid_equal(&event_notify->sec_type, &sec_type)) {
+			list_del_rcu(&event_notify->list);
+			found = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+	mutex_unlock(&ghes_event_notify_mutex);
+
+	if (!found) {
+		pr_err("Tried to unregister a GHES event handler that has not been registered\n");
+		return;
+	}
+
+	synchronize_rcu();
+	kfree(event_notify);
+}
+EXPORT_SYMBOL_GPL(ghes_unregister_event_handler);
+
+static int ghes_handle_non_standard_event(guid_t *sec_type,
+	struct acpi_hest_generic_data *gdata, int sev)
+{
+	struct ghes_event_notify *event_notify;
+	bool found = false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(event_notify,
+				&ghes_event_handler_list, list) {
+		if (guid_equal(&event_notify->sec_type, sec_type)) {
+			event_notify->event_handler(gdata, sev,
+						    event_notify->data);
+			found = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return found;
+}
+
 static void ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {
@@ -525,11 +622,14 @@ 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);
+			if (!ghes_handle_non_standard_event(sec_type, gdata,
+							    sev)) {
+				void *err = acpi_hest_get_payload(gdata);
+
+				log_non_standard_event(sec_type, fru_id,
+						       fru_text, sec_sev, err,
+						       gdata->error_data_length);
+			}
 		}
 	}
 }
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index e3f1cdd..2564860 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -50,6 +50,55 @@ enum {
 	GHES_SEV_PANIC = 0x3,
 };
 
+/**
+ * typedef ghes_event_handler_t - event handling function
+ * for the non-fatal HW errors.
+ *
+ * @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.
+ *
+ * The error handling function is responsible for logging error and
+ * this function would be called in the interrupt context.
+ */
+typedef void (*ghes_event_handler_t)(struct acpi_hest_generic_data *gdata,
+				     int sev, void *data);
+
+#ifdef CONFIG_ACPI_APEI_GHES
+/**
+ * ghes_register_event_handler - register an event handling
+ * function for the non-fatal HW errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @event_handler: pointer to the event handling function.
+ * @data: handler driver's private data.
+ *
+ * Return : 0 - SUCCESS, non-zero - FAIL.
+ */
+int ghes_register_event_handler(guid_t sec_type,
+				ghes_event_handler_t event_handler,
+				void *data);
+
+/**
+ * ghes_unregister_event_handler - unregister the previously
+ * registered event handling function.
+ * @sec_type: sec_type of the corresponding CPER.
+ */
+void ghes_unregister_event_handler(guid_t sec_type);
+
+#else
+int ghes_register_event_handler(guid_t sec_type,
+				ghes_event_handler_t event_handler,
+				void *data)
+{
+	return -ENODEV;
+}
+
+void ghes_unregister_event_handler(guid_t sec_type)
+{
+}
+#endif
+
 int ghes_estatus_pool_init(int num_ghes);
 
 /* From drivers/edac/ghes_edac.c */
-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors
  2020-01-15 11:01 ` [RFC PATCH 0/2] " Shiju Jose
  2020-01-15 11:01   ` [RFC PATCH 1/2] " Shiju Jose
@ 2020-01-15 11:01   ` Shiju Jose
  2020-01-15 14:13     ` Bjorn Helgaas
  1 sibling, 1 reply; 23+ messages in thread
From: Shiju Jose @ 2020-01-15 11:01 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

From: Yicong Yang <yangyicong@hisilicon.com>

The hip08 error handle driver logs and reports PCIe controller's
recoverable errors.
Perform root port reset and restore link status for the recovery.

Following are some of the PCIe controller's recoverable errors
1. completion transmission timeout error.
2. CRS retry counter over the threshold error.
3. ECC 2 bit errors
4. AXI bresponse/rresponse errors etc.

RFC: The appropriate location for this driver may be discussed.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/pci/controller/Kconfig                 |   8 +
 drivers/pci/controller/Makefile                |   1 +
 drivers/pci/controller/pcie-hisi-hip08-error.c | 323 +++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-hisi-hip08-error.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index c77069c..0ee99b8 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -260,6 +260,14 @@ config PCI_HYPERV_INTERFACE
 	  The Hyper-V PCI Interface is a helper driver allows other drivers to
 	  have a common interface with the Hyper-V PCI frontend driver.
 
+config PCIE_HISI_HIP08_ERR_HANDLER
+	depends on ARM64 || COMPILE_TEST
+	depends on (ACPI && PCI_QUIRKS)
+	bool "HiSilicon hip08 Soc PCIe local error handling driver"
+	help
+	  Say Y here if you want PCIe error handling support
+	  for the PCIe local(controller) errors on HiSilicon hip08 SoC
+
 source "drivers/pci/controller/dwc/Kconfig"
 source "drivers/pci/controller/cadence/Kconfig"
 endmenu
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 3d4f597..ac9852f 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
 obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
 obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
+obj-$(CONFIG_PCIE_HISI_HIP08_ERR_HANDLER) += pcie-hisi-hip08-error.o
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/
 
diff --git a/drivers/pci/controller/pcie-hisi-hip08-error.c b/drivers/pci/controller/pcie-hisi-hip08-error.c
new file mode 100644
index 0000000..6f5d002
--- /dev/null
+++ b/drivers/pci/controller/pcie-hisi-hip08-error.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe driver for handling PCIe local errors occurred on
+ * HiSilicon hip08 PCIe controller.
+ *
+ * Copyright (c) 2018-2019 Hisilicon Limited.
+ */
+
+#include <linux/acpi.h>
+#include <acpi/ghes.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/list.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock.h>
+
+#include "../pci.h"
+
+#define HISI_PCIE_ERR_RECOVER_RING_SIZE           16
+#define	HISI_PCIE_ERR_INFO_SIZE	1024
+
+/* HISI PCIe Local error definitions */
+#define HISI_PCIE_ERR_MISC_REGS	33
+
+#define HISI_PCIE_SUB_MODULE_ID_AP	0
+#define HISI_PCIE_SUB_MODULE_ID_TL	1
+#define HISI_PCIE_SUB_MODULE_ID_MAC	2
+#define HISI_PCIE_SUB_MODULE_ID_DL	3
+#define HISI_PCIE_SUB_MODULE_ID_SDI	4
+
+#define HISI_PCIE_LOCAL_VALID_VERSION		BIT(0)
+#define HISI_PCIE_LOCAL_VALID_SOC_ID		BIT(1)
+#define HISI_PCIE_LOCAL_VALID_SOCKET_ID		BIT(2)
+#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID		BIT(3)
+#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID	BIT(4)
+#define HISI_PCIE_LOCAL_VALID_CORE_ID		BIT(5)
+#define HISI_PCIE_LOCAL_VALID_PORT_ID		BIT(6)
+#define HISI_PCIE_LOCAL_VALID_ERR_TYPE		BIT(7)
+#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY	BIT(8)
+#define HISI_PCIE_LOCAL_VALID_ERR_MISC		9
+
+#define HISI_ERR_SEV_RECOVERABLE	0
+#define HISI_ERR_SEV_FATAL		1
+#define HISI_ERR_SEV_CORRECTED		2
+#define HISI_ERR_SEV_NONE		3
+
+guid_t hip08_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
+				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
+
+#define HISI_PCIE_CORE_ID(v)             ((v) >> 3)
+#define HISI_PCIE_PORT_ID(core, v)       ((v >> 1) + (core << 3))
+#define HISI_PCIE_CORE_PORT_ID(v)        ((v % 8) << 1)
+#define HISI_PCIE_ROOT_BUSNR(v)          ((v) ? 0x80 : 0)
+
+struct hisi_pcie_local_err_data {
+	uint64_t   val_bits;
+	uint8_t    version;
+	uint8_t    soc_id;
+	uint8_t    socket_id;
+	uint8_t    nimbus_id;
+	uint8_t    sub_module_id;
+	uint8_t    core_id;
+	uint8_t    port_id;
+	uint8_t    err_severity;
+	uint16_t   err_type;
+	uint8_t    reserv[2];
+	uint32_t   err_misc[HISI_PCIE_ERR_MISC_REGS];
+};
+
+struct pcie_err_info {
+	struct hisi_pcie_local_err_data err_data;
+	struct platform_device *pdev;
+};
+
+static char *pcie_local_sub_module_name(uint8_t id)
+{
+	switch (id) {
+	case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer";
+	case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer";
+	case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer";
+	case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer";
+	case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer";
+	}
+
+	return "unknown";
+}
+
+static char *err_severity(uint8_t err_sev)
+{
+	switch (err_sev) {
+	case HISI_ERR_SEV_RECOVERABLE: return "recoverable";
+	case HISI_ERR_SEV_FATAL: return "fatal";
+	case HISI_ERR_SEV_CORRECTED: return "corrected";
+	case HISI_ERR_SEV_NONE: return "none";
+	}
+
+	return "unknown";
+}
+
+static struct pci_dev *hisi_hip08_pcie_get_rp(u32 chip_id, u32 port_id)
+{
+	u32 devfn = PCI_DEVFN(port_id, 0);
+	u32 busnr = HISI_PCIE_ROOT_BUSNR(chip_id);
+
+	return pci_get_domain_bus_and_slot(0, busnr, devfn);
+}
+
+static int hisi_hip08_pcie_port_acpi_reset(struct platform_device *pdev,
+					u32 chip_id, u32 port_id)
+{
+	struct device *dev = &(pdev->dev);
+	struct acpi_object_list arg_list;
+	union acpi_object arg[3];
+
+	arg[0].type = ACPI_TYPE_INTEGER;
+	arg[0].integer.value = chip_id;
+	arg[1].type = ACPI_TYPE_INTEGER;
+	arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
+	arg[2].type = ACPI_TYPE_INTEGER;
+	arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
+
+	arg_list.count = 3;
+	arg_list.pointer = arg;
+	/* Call the ACPI handle to reset root port  */
+	if (ACPI_HANDLE(dev)) {
+		unsigned long long data = 0;
+		acpi_status s;
+
+		s = acpi_evaluate_integer(ACPI_HANDLE(dev),
+				"RST", &arg_list, &data);
+
+		if (ACPI_FAILURE(s)) {
+			dev_err(dev, "No Reset method\n");
+			return -EIO;
+		}
+
+		if (data) {
+			dev_err(dev, "Failed to Reset\n");
+			return -EIO;
+		}
+
+	} else {
+		dev_err(dev, "No Reset method\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int hisi_hip08_pcie_port_reset(struct platform_device *dev,
+				      u32 chip_id, u32 port_id)
+{
+	struct pci_dev *pdev;
+	struct pci_bus *root_bus;
+
+	pdev = hisi_hip08_pcie_get_rp(chip_id, port_id);
+	if (!pdev) {
+		dev_info(&(dev->dev), "Fail to get root port device\n");
+		return -ENODEV;
+	}
+	root_bus = pdev->bus;
+
+	pci_stop_and_remove_bus_device_locked(pdev);
+
+	if (hisi_hip08_pcie_port_acpi_reset(dev, chip_id, port_id))
+		return -EIO;
+	ssleep(1UL);
+
+	/* add root port and downstream devices */
+	pci_lock_rescan_remove();
+	pci_rescan_bus(root_bus);
+	pci_unlock_rescan_remove();
+
+	return 0;
+}
+
+static void pcie_local_error_handle(const struct hisi_pcie_local_err_data *err,
+				    struct platform_device *pdev)
+{
+	char buf[HISI_PCIE_ERR_INFO_SIZE];
+	char *p = buf, *end = buf + sizeof(buf);
+	struct device *dev = &(pdev->dev);
+	uint32_t i;
+	int rc;
+
+	if (err->val_bits == 0) {
+		dev_warn(dev, "%s: no valid error information\n", __func__);
+		return;
+	}
+
+	/* Logging */
+	p += snprintf(p, end - p, "[ Table version=%d ", err->version);
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOC_ID)
+		p += snprintf(p, end - p, "SOC ID=%d ", err->soc_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOCKET_ID)
+		p += snprintf(p, end - p, "socket ID=%d ", err->socket_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_NIMBUS_ID)
+		p += snprintf(p, end - p, "nimbus ID=%d ", err->nimbus_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID)
+		p += snprintf(p, end - p, "sub module=%s ",
+			      pcie_local_sub_module_name(err->sub_module_id));
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_CORE_ID)
+		p += snprintf(p, end - p, "core ID=core%d ", err->core_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_PORT_ID)
+		p += snprintf(p, end - p, "port ID=port%d ", err->port_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_SEVERITY)
+		p += snprintf(p, end - p, "error severity=%s ",
+			      err_severity(err->err_severity));
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_TYPE)
+		p += snprintf(p, end - p, "error type=0x%x ", err->err_type);
+
+	p += snprintf(p, end - p, "]\n");
+	dev_info(dev, "\nHISI HIP08: PCIe local error\n");
+	dev_info(dev, "%s\n", buf);
+
+	dev_info(dev, "Reg Dump:\n");
+	for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
+		if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
+			dev_info(dev,
+				 "ERR_MISC_%d=0x%x\n", i, err->err_misc[i]);
+	}
+
+	/* Recovery for the PCIe local errors */
+	if (err->err_severity == HISI_ERR_SEV_RECOVERABLE) {
+		/* try reset PCI port for the error recovery */
+		rc = hisi_hip08_pcie_port_reset(pdev, err->socket_id,
+				HISI_PCIE_PORT_ID(err->core_id, err->port_id));
+		if (rc) {
+			dev_info(dev, "fail to do hip08 pcie port reset\n");
+			return;
+		}
+	}
+}
+
+static DEFINE_KFIFO(pcie_err_recover_ring, struct pcie_err_info,
+		    HISI_PCIE_ERR_RECOVER_RING_SIZE);
+static DEFINE_SPINLOCK(pcie_err_recover_ring_lock);
+
+static void pcie_local_err_recover_work_func(struct work_struct *work)
+{
+	struct pcie_err_info pcie_err_entry;
+
+	while (kfifo_get(&pcie_err_recover_ring, &pcie_err_entry)) {
+		pcie_local_error_handle(&pcie_err_entry.err_data,
+					pcie_err_entry.pdev);
+	}
+}
+
+static DECLARE_WORK(pcie_err_recover_work, pcie_local_err_recover_work_func);
+
+
+static void hip08_pcie_local_error_handle(struct acpi_hest_generic_data *gdata,
+					  int sev, void *data)
+{
+	const struct hisi_pcie_local_err_data *err_data =
+					acpi_hest_get_payload(gdata);
+	struct pcie_err_info err_info;
+	struct platform_device *pdev = data;
+	struct device *dev = &(pdev->dev);
+
+	memcpy(&err_info.err_data, err_data, sizeof(*err_data));
+	err_info.pdev = pdev;
+
+	if (kfifo_in_spinlocked(&pcie_err_recover_ring, &err_info, 1,
+				&pcie_err_recover_ring_lock))
+		schedule_work(&pcie_err_recover_work);
+	else
+		dev_warn(dev, "Buffer overflow when recovering PCIe local error\n");
+}
+
+static int hisi_hip08_pcie_err_handler_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+
+	if (ghes_register_event_handler(hip08_pcie_sec_type,
+					hip08_pcie_local_error_handle,
+					pdev)) {
+		dev_err(&(pdev->dev), "%s : ghes_register_event_handler fail\n",
+			__func__);
+		return ret;
+}
+
+	return 0;
+}
+
+static int hisi_hip08_pcie_err_handler_remove(struct platform_device *pdev)
+{
+	ghes_unregister_event_handler(hip08_pcie_sec_type);
+
+	return 0;
+}
+
+static const struct acpi_device_id hip08_pcie_acpi_match[] = {
+	{ "HISI0361", 0 },
+	{ }
+};
+
+static struct platform_driver hisi_hip08_pcie_err_handler_driver = {
+	.driver = {
+		.name	= "hisi-hip08-pcie-err-handler",
+		.acpi_match_table = hip08_pcie_acpi_match,
+	},
+	.probe		= hisi_hip08_pcie_err_handler_probe,
+	.remove		= hisi_hip08_pcie_err_handler_remove,
+};
+module_platform_driver(hisi_hip08_pcie_err_handler_driver);
+
+MODULE_DESCRIPTION("HiSilicon HIP08 PCIe controller error handling driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors
  2020-01-15 11:01   ` [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors Shiju Jose
@ 2020-01-15 14:13     ` Bjorn Helgaas
  2020-01-17  9:40       ` Shiju Jose
  0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2020-01-15 14:13 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx, linuxarm,
	jonathan.cameron, tanxiaofei, yangyicong

Follow convention for subject line.

On Wed, Jan 15, 2020 at 11:01:40AM +0000, Shiju Jose wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> The hip08 error handle driver logs and reports PCIe controller's
> recoverable errors.
> Perform root port reset and restore link status for the recovery.
> 
> Following are some of the PCIe controller's recoverable errors
> 1. completion transmission timeout error.
> 2. CRS retry counter over the threshold error.
> 3. ECC 2 bit errors
> 4. AXI bresponse/rresponse errors etc.
> 
> RFC: The appropriate location for this driver may be discussed.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/pci/controller/Kconfig                 |   8 +
>  drivers/pci/controller/Makefile                |   1 +
>  drivers/pci/controller/pcie-hisi-hip08-error.c | 323 +++++++++++++++++++++++++

Seems like this driver and its Kconfig should be near
drivers/pci/controller/dwc/pcie-hisi.c.

>  3 files changed, 332 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-hisi-hip08-error.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index c77069c..0ee99b8 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -260,6 +260,14 @@ config PCI_HYPERV_INTERFACE
>  	  The Hyper-V PCI Interface is a helper driver allows other drivers to
>  	  have a common interface with the Hyper-V PCI frontend driver.
>  
> +config PCIE_HISI_HIP08_ERR_HANDLER

Config symbol is too long, maybe CONFIG_PCI_HISI_ERR or similar (to be
parallel with existing CONFIG_PCI_HISI).  Both should probably be
"CONFIG_PCIE", not "CONFIG_PCI".  I can't remember why CONFIG_PCI_HISI
is that way.

> +	depends on ARM64 || COMPILE_TEST
> +	depends on (ACPI && PCI_QUIRKS)
> +	bool "HiSilicon hip08 Soc PCIe local error handling driver"
> +	help
> +	  Say Y here if you want PCIe error handling support
> +	  for the PCIe local(controller) errors on HiSilicon hip08 SoC
> +
>  source "drivers/pci/controller/dwc/Kconfig"
>  source "drivers/pci/controller/cadence/Kconfig"
>  endmenu
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 3d4f597..ac9852f 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
>  obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
>  obj-$(CONFIG_VMD) += vmd.o
> +obj-$(CONFIG_PCIE_HISI_HIP08_ERR_HANDLER) += pcie-hisi-hip08-error.o
>  # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>  obj-y				+= dwc/
>  
> diff --git a/drivers/pci/controller/pcie-hisi-hip08-error.c b/drivers/pci/controller/pcie-hisi-hip08-error.c
> new file mode 100644
> index 0000000..6f5d002
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-hisi-hip08-error.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe driver for handling PCIe local errors occurred on
> + * HiSilicon hip08 PCIe controller.

"PCIe" occurs too many times in this sentence.  Strictly speaking this
is not a "PCIe driver"; it's a driver for an ACPI device that reports
hip08-related errors.  Hopefully we don't need a separate driver for
every hip* device, so maybe the "hip08" name is too specific.

> + * Copyright (c) 2018-2019 Hisilicon Limited.

Capitalize "HiSilicon" consistently.  Also "hip08"; previous practice
in drivers/pci is "Hip05", "Hip06", so use that unless HiSilicon
itself does it differently.

> +#include <linux/acpi.h>
> +#include <acpi/ghes.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/kfifo.h>
> +#include <linux/spinlock.h>
> +
> +#include "../pci.h"
> +
> +#define HISI_PCIE_ERR_RECOVER_RING_SIZE           16
> +#define	HISI_PCIE_ERR_INFO_SIZE	1024
> +
> +/* HISI PCIe Local error definitions */
> +#define HISI_PCIE_ERR_MISC_REGS	33
> +
> +#define HISI_PCIE_SUB_MODULE_ID_AP	0
> +#define HISI_PCIE_SUB_MODULE_ID_TL	1
> +#define HISI_PCIE_SUB_MODULE_ID_MAC	2
> +#define HISI_PCIE_SUB_MODULE_ID_DL	3
> +#define HISI_PCIE_SUB_MODULE_ID_SDI	4
> +
> +#define HISI_PCIE_LOCAL_VALID_VERSION		BIT(0)
> +#define HISI_PCIE_LOCAL_VALID_SOC_ID		BIT(1)
> +#define HISI_PCIE_LOCAL_VALID_SOCKET_ID		BIT(2)
> +#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID		BIT(3)
> +#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID	BIT(4)
> +#define HISI_PCIE_LOCAL_VALID_CORE_ID		BIT(5)
> +#define HISI_PCIE_LOCAL_VALID_PORT_ID		BIT(6)
> +#define HISI_PCIE_LOCAL_VALID_ERR_TYPE		BIT(7)
> +#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY	BIT(8)
> +#define HISI_PCIE_LOCAL_VALID_ERR_MISC		9
> +
> +#define HISI_ERR_SEV_RECOVERABLE	0
> +#define HISI_ERR_SEV_FATAL		1
> +#define HISI_ERR_SEV_CORRECTED		2
> +#define HISI_ERR_SEV_NONE		3
> +
> +guid_t hip08_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
> +				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
> +
> +#define HISI_PCIE_CORE_ID(v)             ((v) >> 3)
> +#define HISI_PCIE_PORT_ID(core, v)       ((v >> 1) + (core << 3))
> +#define HISI_PCIE_CORE_PORT_ID(v)        ((v % 8) << 1)
> +#define HISI_PCIE_ROOT_BUSNR(v)          ((v) ? 0x80 : 0)

Is the root bus number really hard-wired in the chip?  You're saying
the only possible root bus numbers are 0x80 and 0x00?  Typically this
bus number is programmable.

Why parens around "v" (sometimes) but not others and "core"?

> +struct hisi_pcie_local_err_data {
> +	uint64_t   val_bits;
> +	uint8_t    version;
> +	uint8_t    soc_id;
> +	uint8_t    socket_id;
> +	uint8_t    nimbus_id;
> +	uint8_t    sub_module_id;
> +	uint8_t    core_id;
> +	uint8_t    port_id;
> +	uint8_t    err_severity;
> +	uint16_t   err_type;
> +	uint8_t    reserv[2];
> +	uint32_t   err_misc[HISI_PCIE_ERR_MISC_REGS];

Use u64, u8, u32 throughout instead.

> +};
> +
> +struct pcie_err_info {
> +	struct hisi_pcie_local_err_data err_data;
> +	struct platform_device *pdev;
> +};
> +
> +static char *pcie_local_sub_module_name(uint8_t id)
> +{
> +	switch (id) {
> +	case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer";
> +	}
> +
> +	return "unknown";
> +}
> +
> +static char *err_severity(uint8_t err_sev)
> +{
> +	switch (err_sev) {
> +	case HISI_ERR_SEV_RECOVERABLE: return "recoverable";
> +	case HISI_ERR_SEV_FATAL: return "fatal";
> +	case HISI_ERR_SEV_CORRECTED: return "corrected";
> +	case HISI_ERR_SEV_NONE: return "none";
> +	}
> +
> +	return "unknown";
> +}
> +
> +static struct pci_dev *hisi_hip08_pcie_get_rp(u32 chip_id, u32 port_id)
> +{
> +	u32 devfn = PCI_DEVFN(port_id, 0);
> +	u32 busnr = HISI_PCIE_ROOT_BUSNR(chip_id);
> +
> +	return pci_get_domain_bus_and_slot(0, busnr, devfn);
> +}
> +
> +static int hisi_hip08_pcie_port_acpi_reset(struct platform_device *pdev,
> +					u32 chip_id, u32 port_id)
> +{
> +	struct device *dev = &(pdev->dev);

Unnecessary parens.  More occurrences below.

> +	struct acpi_object_list arg_list;
> +	union acpi_object arg[3];
> +
> +	arg[0].type = ACPI_TYPE_INTEGER;
> +	arg[0].integer.value = chip_id;
> +	arg[1].type = ACPI_TYPE_INTEGER;
> +	arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
> +	arg[2].type = ACPI_TYPE_INTEGER;
> +	arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
> +
> +	arg_list.count = 3;
> +	arg_list.pointer = arg;
> +	/* Call the ACPI handle to reset root port  */

s/root port  /root port /

> +	if (ACPI_HANDLE(dev)) {

Restructure this to return early for error and unindent the following,
e.g.,

  acpi_handle handle = ACPI_HANDLE(dev);

  if (!handle) {
    dev_err(...);
    return -EINVAL;
  }

  arg[0].type = ACPI_TYPE_INTEGER;
  ...
  s = acpi_evaluate_integer(handle, ...);

> +		unsigned long long data = 0;
> +		acpi_status s;
> +
> +		s = acpi_evaluate_integer(ACPI_HANDLE(dev),
> +				"RST", &arg_list, &data);
> +
> +		if (ACPI_FAILURE(s)) {
> +			dev_err(dev, "No Reset method\n");
> +			return -EIO;
> +		}
> +
> +		if (data) {
> +			dev_err(dev, "Failed to Reset\n");
> +			return -EIO;
> +		}
> +
> +	} else {
> +		dev_err(dev, "No Reset method\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hisi_hip08_pcie_port_reset(struct platform_device *dev,
> +				      u32 chip_id, u32 port_id)
> +{
> +	struct pci_dev *pdev;
> +	struct pci_bus *root_bus;
> +
> +	pdev = hisi_hip08_pcie_get_rp(chip_id, port_id);
> +	if (!pdev) {
> +		dev_info(&(dev->dev), "Fail to get root port device\n");
> +		return -ENODEV;
> +	}
> +	root_bus = pdev->bus;
> +
> +	pci_stop_and_remove_bus_device_locked(pdev);
> +
> +	if (hisi_hip08_pcie_port_acpi_reset(dev, chip_id, port_id))
> +		return -EIO;
> +	ssleep(1UL);

Please include a comment that cites the spec section that requires
this sleep.

> +
> +	/* add root port and downstream devices */
> +	pci_lock_rescan_remove();
> +	pci_rescan_bus(root_bus);
> +	pci_unlock_rescan_remove();
> +
> +	return 0;
> +}
> +
> +static void pcie_local_error_handle(const struct hisi_pcie_local_err_data *err,
> +				    struct platform_device *pdev)
> +{
> +	char buf[HISI_PCIE_ERR_INFO_SIZE];
> +	char *p = buf, *end = buf + sizeof(buf);
> +	struct device *dev = &(pdev->dev);
> +	uint32_t i;
> +	int rc;
> +
> +	if (err->val_bits == 0) {
> +		dev_warn(dev, "%s: no valid error information\n", __func__);
> +		return;
> +	}
> +
> +	/* Logging */
> +	p += snprintf(p, end - p, "[ Table version=%d ", err->version);
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOC_ID)
> +		p += snprintf(p, end - p, "SOC ID=%d ", err->soc_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOCKET_ID)
> +		p += snprintf(p, end - p, "socket ID=%d ", err->socket_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_NIMBUS_ID)
> +		p += snprintf(p, end - p, "nimbus ID=%d ", err->nimbus_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID)
> +		p += snprintf(p, end - p, "sub module=%s ",
> +			      pcie_local_sub_module_name(err->sub_module_id));
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_CORE_ID)
> +		p += snprintf(p, end - p, "core ID=core%d ", err->core_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_PORT_ID)
> +		p += snprintf(p, end - p, "port ID=port%d ", err->port_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_SEVERITY)
> +		p += snprintf(p, end - p, "error severity=%s ",
> +			      err_severity(err->err_severity));
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_TYPE)
> +		p += snprintf(p, end - p, "error type=0x%x ", err->err_type);
> +
> +	p += snprintf(p, end - p, "]\n");
> +	dev_info(dev, "\nHISI HIP08: PCIe local error\n");
> +	dev_info(dev, "%s\n", buf);
> +
> +	dev_info(dev, "Reg Dump:\n");
> +	for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
> +		if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
> +			dev_info(dev,
> +				 "ERR_MISC_%d=0x%x\n", i, err->err_misc[i]);
> +	}
> +
> +	/* Recovery for the PCIe local errors */
> +	if (err->err_severity == HISI_ERR_SEV_RECOVERABLE) {
> +		/* try reset PCI port for the error recovery */
> +		rc = hisi_hip08_pcie_port_reset(pdev, err->socket_id,
> +				HISI_PCIE_PORT_ID(err->core_id, err->port_id));
> +		if (rc) {
> +			dev_info(dev, "fail to do hip08 pcie port reset\n");
> +			return;
> +		}
> +	}
> +}
> +
> +static DEFINE_KFIFO(pcie_err_recover_ring, struct pcie_err_info,
> +		    HISI_PCIE_ERR_RECOVER_RING_SIZE);
> +static DEFINE_SPINLOCK(pcie_err_recover_ring_lock);
> +
> +static void pcie_local_err_recover_work_func(struct work_struct *work)
> +{
> +	struct pcie_err_info pcie_err_entry;
> +
> +	while (kfifo_get(&pcie_err_recover_ring, &pcie_err_entry)) {
> +		pcie_local_error_handle(&pcie_err_entry.err_data,
> +					pcie_err_entry.pdev);
> +	}
> +}
> +
> +static DECLARE_WORK(pcie_err_recover_work, pcie_local_err_recover_work_func);
> +
> +
> +static void hip08_pcie_local_error_handle(struct acpi_hest_generic_data *gdata,
> +					  int sev, void *data)
> +{
> +	const struct hisi_pcie_local_err_data *err_data =
> +					acpi_hest_get_payload(gdata);
> +	struct pcie_err_info err_info;
> +	struct platform_device *pdev = data;
> +	struct device *dev = &(pdev->dev);
> +
> +	memcpy(&err_info.err_data, err_data, sizeof(*err_data));
> +	err_info.pdev = pdev;
> +
> +	if (kfifo_in_spinlocked(&pcie_err_recover_ring, &err_info, 1,
> +				&pcie_err_recover_ring_lock))
> +		schedule_work(&pcie_err_recover_work);
> +	else
> +		dev_warn(dev, "Buffer overflow when recovering PCIe local error\n");

I'd call this a "queue full" warning or similar.  "Buffer overflow"
suggests that we wrote past the end of a buffer and corrupted some
memory, but that's not the case here.

> +}
> +
> +static int hisi_hip08_pcie_err_handler_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;

Pointless local variable; maybe you meant to return failure if
ghes_register_event_handler() fails?  Don't initialize unless it's
necessary.

> +
> +	if (ghes_register_event_handler(hip08_pcie_sec_type,
> +					hip08_pcie_local_error_handle,
> +					pdev)) {
> +		dev_err(&(pdev->dev), "%s : ghes_register_event_handler fail\n",
> +			__func__);
> +		return ret;
> +}

Indentation error.

> +
> +	return 0;
> +}
> +
> +static int hisi_hip08_pcie_err_handler_remove(struct platform_device *pdev)
> +{
> +	ghes_unregister_event_handler(hip08_pcie_sec_type);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id hip08_pcie_acpi_match[] = {
> +	{ "HISI0361", 0 },
> +	{ }
> +};
> +
> +static struct platform_driver hisi_hip08_pcie_err_handler_driver = {
> +	.driver = {
> +		.name	= "hisi-hip08-pcie-err-handler",
> +		.acpi_match_table = hip08_pcie_acpi_match,
> +	},
> +	.probe		= hisi_hip08_pcie_err_handler_probe,
> +	.remove		= hisi_hip08_pcie_err_handler_remove,
> +};
> +module_platform_driver(hisi_hip08_pcie_err_handler_driver);
> +
> +MODULE_DESCRIPTION("HiSilicon HIP08 PCIe controller error handling driver");
> +MODULE_LICENSE("GPL v2");
> +
> -- 
> 1.9.1
> 
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors
  2020-01-15 14:13     ` Bjorn Helgaas
@ 2020-01-17  9:40       ` Shiju Jose
  0 siblings, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-17  9:40 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx, Linuxarm,
	Jonathan Cameron, tanxiaofei, yangyicong

Hi Bjorn,

Thanks for reviewing the patch.
Please find reply inline.

>-----Original Message-----
>From: Bjorn Helgaas [mailto:helgaas@kernel.org]
>Sent: 15 January 2020 14:14
>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; lenb@kernel.org; bp@alien8.de;
>james.morse@arm.com; tony.luck@intel.com; gregkh@linuxfoundation.org;
>zhangliguang@linux.alibaba.com; tglx@linutronix.de; Linuxarm
><linuxarm@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>yangyicong <yangyicong@huawei.com>
>Subject: Re: [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08
>PCIe controller's errors
>
>Follow convention for subject line.

Ok. We will ix it.

>
>On Wed, Jan 15, 2020 at 11:01:40AM +0000, Shiju Jose wrote:
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> The hip08 error handle driver logs and reports PCIe controller's
>> recoverable errors.
>> Perform root port reset and restore link status for the recovery.
>>
>> Following are some of the PCIe controller's recoverable errors 1.
>> completion transmission timeout error.
>> 2. CRS retry counter over the threshold error.
>> 3. ECC 2 bit errors
>> 4. AXI bresponse/rresponse errors etc.
>>
>> RFC: The appropriate location for this driver may be discussed.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/pci/controller/Kconfig                 |   8 +
>>  drivers/pci/controller/Makefile                |   1 +
>>  drivers/pci/controller/pcie-hisi-hip08-error.c | 323
>> +++++++++++++++++++++++++
>
>Seems like this driver and its Kconfig should be near
>drivers/pci/controller/dwc/pcie-hisi.c.

pcie-hisi.c was for our old hip* devices.
Our hip08 PCIe controller doesn't use DWC ip.
Thus the driver to be in /drivers/pci/controller/ ?

>
>>  3 files changed, 332 insertions(+)
>>  create mode 100644 drivers/pci/controller/pcie-hisi-hip08-error.c
>>
>> diff --git a/drivers/pci/controller/Kconfig
>> b/drivers/pci/controller/Kconfig index c77069c..0ee99b8 100644
>> --- a/drivers/pci/controller/Kconfig
>> +++ b/drivers/pci/controller/Kconfig
>> @@ -260,6 +260,14 @@ config PCI_HYPERV_INTERFACE
>>  	  The Hyper-V PCI Interface is a helper driver allows other drivers to
>>  	  have a common interface with the Hyper-V PCI frontend driver.
>>
>> +config PCIE_HISI_HIP08_ERR_HANDLER
>
>Config symbol is too long, maybe CONFIG_PCI_HISI_ERR or similar (to be
>parallel with existing CONFIG_PCI_HISI).  Both should probably be
>"CONFIG_PCIE", not "CONFIG_PCI".  I can't remember why CONFIG_PCI_HISI is
>that way.

Ok. We will change it to CONFIG_PCI_HISI_ERR.

>
>> +	depends on ARM64 || COMPILE_TEST
>> +	depends on (ACPI && PCI_QUIRKS)
>> +	bool "HiSilicon hip08 Soc PCIe local error handling driver"
>> +	help
>> +	  Say Y here if you want PCIe error handling support
>> +	  for the PCIe local(controller) errors on HiSilicon hip08 SoC
>> +
>>  source "drivers/pci/controller/dwc/Kconfig"
>>  source "drivers/pci/controller/cadence/Kconfig"
>>  endmenu
>> diff --git a/drivers/pci/controller/Makefile
>> b/drivers/pci/controller/Makefile index 3d4f597..ac9852f 100644
>> --- a/drivers/pci/controller/Makefile
>> +++ b/drivers/pci/controller/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
>>  obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
>>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
>>  obj-$(CONFIG_VMD) += vmd.o
>> +obj-$(CONFIG_PCIE_HISI_HIP08_ERR_HANDLER) += pcie-hisi-hip08-error.o
>>  # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>>  obj-y				+= dwc/
>>
>> diff --git a/drivers/pci/controller/pcie-hisi-hip08-error.c
>> b/drivers/pci/controller/pcie-hisi-hip08-error.c
>> new file mode 100644
>> index 0000000..6f5d002
>> --- /dev/null
>> +++ b/drivers/pci/controller/pcie-hisi-hip08-error.c
>> @@ -0,0 +1,323 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCIe driver for handling PCIe local errors occurred on
>> + * HiSilicon hip08 PCIe controller.
>
>"PCIe" occurs too many times in this sentence.  Strictly speaking this is not a
>"PCIe driver"; it's a driver for an ACPI device that reports hip08-related errors.

Ok. Will remove it.

>Hopefully we don't need a separate driver for every hip* device, so maybe the
>"hip08" name is too specific.

Currently we are using this single driver for all hip08 device. so maybe this is better:
yes, a separate driver is unnecessary. We'll remove "hip08" name and
make it more generic for hip* device.

>
>> + * Copyright (c) 2018-2019 Hisilicon Limited.
>
>Capitalize "HiSilicon" consistently.  Also "hip08"; previous practice in
>drivers/pci is "Hip05", "Hip06", so use that unless HiSilicon itself does it
>differently.

Ok. We will fix.

>
>> +#include <linux/acpi.h>
>> +#include <acpi/ghes.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/list.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/resource.h>
>> +#include <linux/kfifo.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include "../pci.h"
>> +
>> +#define HISI_PCIE_ERR_RECOVER_RING_SIZE           16
>> +#define	HISI_PCIE_ERR_INFO_SIZE	1024
>> +
>> +/* HISI PCIe Local error definitions */
>> +#define HISI_PCIE_ERR_MISC_REGS	33
>> +
>> +#define HISI_PCIE_SUB_MODULE_ID_AP	0
>> +#define HISI_PCIE_SUB_MODULE_ID_TL	1
>> +#define HISI_PCIE_SUB_MODULE_ID_MAC	2
>> +#define HISI_PCIE_SUB_MODULE_ID_DL	3
>> +#define HISI_PCIE_SUB_MODULE_ID_SDI	4
>> +
>> +#define HISI_PCIE_LOCAL_VALID_VERSION		BIT(0)
>> +#define HISI_PCIE_LOCAL_VALID_SOC_ID		BIT(1)
>> +#define HISI_PCIE_LOCAL_VALID_SOCKET_ID		BIT(2)
>> +#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID		BIT(3)
>> +#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID	BIT(4)
>> +#define HISI_PCIE_LOCAL_VALID_CORE_ID		BIT(5)
>> +#define HISI_PCIE_LOCAL_VALID_PORT_ID		BIT(6)
>> +#define HISI_PCIE_LOCAL_VALID_ERR_TYPE		BIT(7)
>> +#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY	BIT(8)
>> +#define HISI_PCIE_LOCAL_VALID_ERR_MISC		9
>> +
>> +#define HISI_ERR_SEV_RECOVERABLE	0
>> +#define HISI_ERR_SEV_FATAL		1
>> +#define HISI_ERR_SEV_CORRECTED		2
>> +#define HISI_ERR_SEV_NONE		3
>> +
>> +guid_t hip08_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
>0xA8, 0x67,
>> +				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
>> +
>> +#define HISI_PCIE_CORE_ID(v)             ((v) >> 3)
>> +#define HISI_PCIE_PORT_ID(core, v)       ((v >> 1) + (core << 3))
>> +#define HISI_PCIE_CORE_PORT_ID(v)        ((v % 8) << 1)
>> +#define HISI_PCIE_ROOT_BUSNR(v)          ((v) ? 0x80 : 0)
>
>Is the root bus number really hard-wired in the chip?  You're saying the only
>possible root bus numbers are 0x80 and 0x00?  Typically this bus number is
>programmable.

We will fix it and remove the macro. We'll get the root bus number from acpi instead.

>
>Why parens around "v" (sometimes) but not others and "core"?

We will correct it.

>
>> +struct hisi_pcie_local_err_data {
>> +	uint64_t   val_bits;
>> +	uint8_t    version;
>> +	uint8_t    soc_id;
>> +	uint8_t    socket_id;
>> +	uint8_t    nimbus_id;
>> +	uint8_t    sub_module_id;
>> +	uint8_t    core_id;
>> +	uint8_t    port_id;
>> +	uint8_t    err_severity;
>> +	uint16_t   err_type;
>> +	uint8_t    reserv[2];
>> +	uint32_t   err_misc[HISI_PCIE_ERR_MISC_REGS];
>
>Use u64, u8, u32 throughout instead.

We will change it.

>
>> +};
>> +
>> +struct pcie_err_info {
>> +	struct hisi_pcie_local_err_data err_data;
>> +	struct platform_device *pdev;
>> +};
>> +
>> +static char *pcie_local_sub_module_name(uint8_t id) {
>> +	switch (id) {
>> +	case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer";
>> +	case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer";
>> +	case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer";
>> +	case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer";
>> +	case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer";
>> +	}
>> +
>> +	return "unknown";
>> +}
>> +
>> +static char *err_severity(uint8_t err_sev) {
>> +	switch (err_sev) {
>> +	case HISI_ERR_SEV_RECOVERABLE: return "recoverable";
>> +	case HISI_ERR_SEV_FATAL: return "fatal";
>> +	case HISI_ERR_SEV_CORRECTED: return "corrected";
>> +	case HISI_ERR_SEV_NONE: return "none";
>> +	}
>> +
>> +	return "unknown";
>> +}
>> +
>> +static struct pci_dev *hisi_hip08_pcie_get_rp(u32 chip_id, u32
>> +port_id) {
>> +	u32 devfn = PCI_DEVFN(port_id, 0);
>> +	u32 busnr = HISI_PCIE_ROOT_BUSNR(chip_id);
>> +
>> +	return pci_get_domain_bus_and_slot(0, busnr, devfn); }
>> +
>> +static int hisi_hip08_pcie_port_acpi_reset(struct platform_device *pdev,
>> +					u32 chip_id, u32 port_id)
>> +{
>> +	struct device *dev = &(pdev->dev);
>
>Unnecessary parens.  More occurrences below.

We will change it.

>
>> +	struct acpi_object_list arg_list;
>> +	union acpi_object arg[3];
>> +
>> +	arg[0].type = ACPI_TYPE_INTEGER;
>> +	arg[0].integer.value = chip_id;
>> +	arg[1].type = ACPI_TYPE_INTEGER;
>> +	arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
>> +	arg[2].type = ACPI_TYPE_INTEGER;
>> +	arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
>> +
>> +	arg_list.count = 3;
>> +	arg_list.pointer = arg;
>> +	/* Call the ACPI handle to reset root port  */
>
>s/root port  /root port /
>
>> +	if (ACPI_HANDLE(dev)) {
>
>Restructure this to return early for error and unindent the following, e.g.,

Ok. We will change it.

>
>  acpi_handle handle = ACPI_HANDLE(dev);
>
>  if (!handle) {
>    dev_err(...);
>    return -EINVAL;
>  }
>
>  arg[0].type = ACPI_TYPE_INTEGER;
>  ...
>  s = acpi_evaluate_integer(handle, ...);
>
>> +		unsigned long long data = 0;
>> +		acpi_status s;
>> +
>> +		s = acpi_evaluate_integer(ACPI_HANDLE(dev),
>> +				"RST", &arg_list, &data);
>> +
>> +		if (ACPI_FAILURE(s)) {
>> +			dev_err(dev, "No Reset method\n");
>> +			return -EIO;
>> +		}
>> +
>> +		if (data) {
>> +			dev_err(dev, "Failed to Reset\n");
>> +			return -EIO;
>> +		}
>> +
>> +	} else {
>> +		dev_err(dev, "No Reset method\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_hip08_pcie_port_reset(struct platform_device *dev,
>> +				      u32 chip_id, u32 port_id)
>> +{
>> +	struct pci_dev *pdev;
>> +	struct pci_bus *root_bus;
>> +
>> +	pdev = hisi_hip08_pcie_get_rp(chip_id, port_id);
>> +	if (!pdev) {
>> +		dev_info(&(dev->dev), "Fail to get root port device\n");
>> +		return -ENODEV;
>> +	}
>> +	root_bus = pdev->bus;
>> +
>> +	pci_stop_and_remove_bus_device_locked(pdev);
>> +
>> +	if (hisi_hip08_pcie_port_acpi_reset(dev, chip_id, port_id))
>> +		return -EIO;
>> +	ssleep(1UL);
>
>Please include a comment that cites the spec section that requires this sleep.

we'll add a comment. We use a 1s delay here as does in pci_reset_secondary_bus().
It's necessary for re-initialization of subordinate devices.

>
>> +
>> +	/* add root port and downstream devices */
>> +	pci_lock_rescan_remove();
>> +	pci_rescan_bus(root_bus);
>> +	pci_unlock_rescan_remove();
>> +
>> +	return 0;
>> +}
>> +
>> +static void pcie_local_error_handle(const struct hisi_pcie_local_err_data
>*err,
>> +				    struct platform_device *pdev) {
>> +	char buf[HISI_PCIE_ERR_INFO_SIZE];
>> +	char *p = buf, *end = buf + sizeof(buf);
>> +	struct device *dev = &(pdev->dev);
>> +	uint32_t i;
>> +	int rc;
>> +
>> +	if (err->val_bits == 0) {
>> +		dev_warn(dev, "%s: no valid error information\n", __func__);
>> +		return;
>> +	}
>> +
>> +	/* Logging */
>> +	p += snprintf(p, end - p, "[ Table version=%d ", err->version);
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOC_ID)
>> +		p += snprintf(p, end - p, "SOC ID=%d ", err->soc_id);
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOCKET_ID)
>> +		p += snprintf(p, end - p, "socket ID=%d ", err->socket_id);
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_NIMBUS_ID)
>> +		p += snprintf(p, end - p, "nimbus ID=%d ", err->nimbus_id);
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID)
>> +		p += snprintf(p, end - p, "sub module=%s ",
>> +			      pcie_local_sub_module_name(err-
>>sub_module_id));
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_CORE_ID)
>> +		p += snprintf(p, end - p, "core ID=core%d ", err->core_id);
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_PORT_ID)
>> +		p += snprintf(p, end - p, "port ID=port%d ", err->port_id);
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_SEVERITY)
>> +		p += snprintf(p, end - p, "error severity=%s ",
>> +			      err_severity(err->err_severity));
>> +
>> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_TYPE)
>> +		p += snprintf(p, end - p, "error type=0x%x ", err->err_type);
>> +
>> +	p += snprintf(p, end - p, "]\n");
>> +	dev_info(dev, "\nHISI HIP08: PCIe local error\n");
>> +	dev_info(dev, "%s\n", buf);
>> +
>> +	dev_info(dev, "Reg Dump:\n");
>> +	for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
>> +		if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
>> +			dev_info(dev,
>> +				 "ERR_MISC_%d=0x%x\n", i, err->err_misc[i]);
>> +	}
>> +
>> +	/* Recovery for the PCIe local errors */
>> +	if (err->err_severity == HISI_ERR_SEV_RECOVERABLE) {
>> +		/* try reset PCI port for the error recovery */
>> +		rc = hisi_hip08_pcie_port_reset(pdev, err->socket_id,
>> +				HISI_PCIE_PORT_ID(err->core_id, err-
>>port_id));
>> +		if (rc) {
>> +			dev_info(dev, "fail to do hip08 pcie port reset\n");
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>> +static DEFINE_KFIFO(pcie_err_recover_ring, struct pcie_err_info,
>> +		    HISI_PCIE_ERR_RECOVER_RING_SIZE); static
>> +DEFINE_SPINLOCK(pcie_err_recover_ring_lock);
>> +
>> +static void pcie_local_err_recover_work_func(struct work_struct
>> +*work) {
>> +	struct pcie_err_info pcie_err_entry;
>> +
>> +	while (kfifo_get(&pcie_err_recover_ring, &pcie_err_entry)) {
>> +		pcie_local_error_handle(&pcie_err_entry.err_data,
>> +					pcie_err_entry.pdev);
>> +	}
>> +}
>> +
>> +static DECLARE_WORK(pcie_err_recover_work,
>> +pcie_local_err_recover_work_func);
>> +
>> +
>> +static void hip08_pcie_local_error_handle(struct acpi_hest_generic_data
>*gdata,
>> +					  int sev, void *data)
>> +{
>> +	const struct hisi_pcie_local_err_data *err_data =
>> +					acpi_hest_get_payload(gdata);
>> +	struct pcie_err_info err_info;
>> +	struct platform_device *pdev = data;
>> +	struct device *dev = &(pdev->dev);
>> +
>> +	memcpy(&err_info.err_data, err_data, sizeof(*err_data));
>> +	err_info.pdev = pdev;
>> +
>> +	if (kfifo_in_spinlocked(&pcie_err_recover_ring, &err_info, 1,
>> +				&pcie_err_recover_ring_lock))
>> +		schedule_work(&pcie_err_recover_work);
>> +	else
>> +		dev_warn(dev, "Buffer overflow when recovering PCIe local
>> +error\n");
>
>I'd call this a "queue full" warning or similar.  "Buffer overflow"
>suggests that we wrote past the end of a buffer and corrupted some memory,
>but that's not the case here.

We will change it to "queue full".

>
>> +}
>> +
>> +static int hisi_hip08_pcie_err_handler_probe(struct platform_device
>> +*pdev) {
>> +	int ret = 0;
>
>Pointless local variable; maybe you meant to return failure if
>ghes_register_event_handler() fails?  Don't initialize unless it's necessary.

We will fix it. We  need to return error value on ghes_register_event_handler  failure.

>
>> +
>> +	if (ghes_register_event_handler(hip08_pcie_sec_type,
>> +					hip08_pcie_local_error_handle,
>> +					pdev)) {
>> +		dev_err(&(pdev->dev), "%s : ghes_register_event_handler
>fail\n",
>> +			__func__);
>> +		return ret;
>> +}
>
>Indentation error.

Ok. we will correct it.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_hip08_pcie_err_handler_remove(struct platform_device
>> +*pdev) {
>> +	ghes_unregister_event_handler(hip08_pcie_sec_type);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id hip08_pcie_acpi_match[] = {
>> +	{ "HISI0361", 0 },
>> +	{ }
>> +};
>> +
>> +static struct platform_driver hisi_hip08_pcie_err_handler_driver = {
>> +	.driver = {
>> +		.name	= "hisi-hip08-pcie-err-handler",
>> +		.acpi_match_table = hip08_pcie_acpi_match,
>> +	},
>> +	.probe		= hisi_hip08_pcie_err_handler_probe,
>> +	.remove		= hisi_hip08_pcie_err_handler_remove,
>> +};
>> +module_platform_driver(hisi_hip08_pcie_err_handler_driver);
>> +
>> +MODULE_DESCRIPTION("HiSilicon HIP08 PCIe controller error handling
>> +driver"); MODULE_LICENSE("GPL v2");
>> +
>> --
>> 1.9.1
>>
>>

Thanks,
Shiju

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors
       [not found] <Shiju Jose>
  2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2020-01-15 11:01 ` [RFC PATCH 0/2] " Shiju Jose
@ 2020-01-24 12:39 ` Shiju Jose
  2020-01-24 12:39   ` [PATCH v2 1/2] " Shiju Jose
  2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
  2 siblings, 2 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-24 12:39 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, helgaas, lenb, bp,
	james.morse, tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

Presently the vendor drivers are unable to do the recovery for the vendor specific
recoverable HW errors, reported to the APEI driver in the vendor defined sections,
because APEI driver does not support reporting the same to the vendor drivers.

This patch set
1. add an interface to the APEI driver to enable the vendor
drivers to register the event handling functions for the corresponding
vendor specific HW errors and report the error to the vendor driver.

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

Changes:

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's errors

 drivers/acpi/apei/ghes.c                 | 116 ++++++++++-
 drivers/pci/controller/Kconfig           |   8 +
 drivers/pci/controller/Makefile          |   1 +
 drivers/pci/controller/pcie-hisi-error.c | 336 +++++++++++++++++++++++++++++++
 include/acpi/ghes.h                      |  56 ++++++
 5 files changed, 512 insertions(+), 5 deletions(-)
 create mode 100644 drivers/pci/controller/pcie-hisi-error.c

-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/2] ACPI: APEI: Add support to notify the vendor specific HW errors
  2020-01-24 12:39 ` [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
@ 2020-01-24 12:39   ` " Shiju Jose
  2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
  1 sibling, 0 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-24 12:39 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, helgaas, lenb, bp,
	james.morse, tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

Presently APEI does not support reporting the vendor specific
recoverable HW errors, received in the vendor defined table entries, to the
vendor drivers for the recovery.

This patch adds the support to register and unregister the
error handling function for the vendor specific HW errors and
to notify the registered kernel driver.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/acpi/apei/ghes.c | 116 +++++++++++++++++++++++++++++++++++++++++++++--
 include/acpi/ghes.h      |  56 +++++++++++++++++++++++
 2 files changed, 167 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 103acbb..69e18d7 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -490,6 +490,109 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
 #endif
 }
 
+struct ghes_event_notify {
+	struct list_head list;
+	struct rcu_head	rcu_head;
+	guid_t sec_type; /* guid of the error record */
+	ghes_event_handler_t event_handler; /* event handler function */
+	void *data; /* handler driver's private data if any */
+};
+
+/* List to store the registered event handling functions */
+static DEFINE_MUTEX(ghes_event_notify_mutex);
+static LIST_HEAD(ghes_event_handler_list);
+
+/**
+ * ghes_register_event_handler - register an event handling
+ * function for the non-fatal HW errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @event_handler: pointer to the error handling function.
+ * @data: handler driver's private data.
+ *
+ * return 0 : SUCCESS, non-zero : FAIL
+ */
+int ghes_register_event_handler(guid_t sec_type,
+				ghes_event_handler_t event_handler,
+				void *data)
+{
+	struct ghes_event_notify *event_notify;
+
+	event_notify = kzalloc(sizeof(*event_notify), GFP_KERNEL);
+	if (!event_notify)
+		return -ENOMEM;
+
+	event_notify->event_handler = event_handler;
+	guid_copy(&event_notify->sec_type, &sec_type);
+	event_notify->data = data;
+
+	mutex_lock(&ghes_event_notify_mutex);
+	list_add_rcu(&event_notify->list, &ghes_event_handler_list);
+	mutex_unlock(&ghes_event_notify_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ghes_register_event_handler);
+
+/**
+ * ghes_unregister_event_handler - unregister the previously
+ * registered event handling function.
+ * @sec_type: sec_type of the corresponding CPER.
+ * @data: driver specific data to distinguish devices.
+ */
+void ghes_unregister_event_handler(guid_t sec_type, void *data)
+{
+	struct ghes_event_notify *event_notify;
+	bool found = false;
+
+	mutex_lock(&ghes_event_notify_mutex);
+	rcu_read_lock();
+	list_for_each_entry_rcu(event_notify,
+				&ghes_event_handler_list, list) {
+		if (guid_equal(&event_notify->sec_type, &sec_type)) {
+			if (data != event_notify->data)
+				continue;
+			list_del_rcu(&event_notify->list);
+			found = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+	mutex_unlock(&ghes_event_notify_mutex);
+
+	if (!found) {
+		pr_err("Tried to unregister a GHES event handler that has not been registered\n");
+		return;
+	}
+
+	synchronize_rcu();
+	kfree(event_notify);
+}
+EXPORT_SYMBOL_GPL(ghes_unregister_event_handler);
+
+static int ghes_handle_non_standard_event(guid_t *sec_type,
+	struct acpi_hest_generic_data *gdata, int sev)
+{
+	struct ghes_event_notify *event_notify;
+	bool found = false;
+	int ret;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(event_notify,
+				&ghes_event_handler_list, list) {
+		if (guid_equal(&event_notify->sec_type, sec_type)) {
+			ret = event_notify->event_handler(gdata, sev,
+						    event_notify->data);
+			if (!ret)
+				continue;
+			found = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return found;
+}
+
 static void ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {
@@ -525,11 +628,14 @@ 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);
+			if (!ghes_handle_non_standard_event(sec_type, gdata,
+							    sev)) {
+				void *err = acpi_hest_get_payload(gdata);
+
+				log_non_standard_event(sec_type, fru_id,
+						       fru_text, sec_sev, err,
+						       gdata->error_data_length);
+			}
 		}
 	}
 }
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index e3f1cdd..e3387cf 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -50,6 +50,62 @@ enum {
 	GHES_SEV_PANIC = 0x3,
 };
 
+enum {
+	GHES_EVENT_NONE	= 0x0,
+	GHES_EVENT_HANDLED	= 0x1,
+};
+
+/**
+ * typedef ghes_event_handler_t - event handling function
+ * for the non-fatal HW errors.
+ *
+ * @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 : GHES_EVENT_NONE - event not handled, GHES_EVENT_HANDLED - handled.
+ *
+ * The error handling function is responsible for logging error and
+ * this function would be called in the interrupt context.
+ */
+typedef int (*ghes_event_handler_t)(struct acpi_hest_generic_data *gdata,
+				    int sev, void *data);
+
+#ifdef CONFIG_ACPI_APEI_GHES
+/**
+ * ghes_register_event_handler - register an event handling
+ * function for the non-fatal HW errors.
+ * @sec_type: sec_type of the corresponding CPER to be notified.
+ * @event_handler: pointer to the event handling function.
+ * @data: handler driver's private data.
+ *
+ * Return : 0 - SUCCESS, non-zero - FAIL.
+ */
+int ghes_register_event_handler(guid_t sec_type,
+				ghes_event_handler_t event_handler,
+				void *data);
+
+/**
+ * ghes_unregister_event_handler - unregister the previously
+ * registered event handling function.
+ * @sec_type: sec_type of the corresponding CPER.
+ * @data: driver specific data to distinguish devices.
+ */
+void ghes_unregister_event_handler(guid_t sec_typei, void *data);
+#else
+static inline int ghes_register_event_handler(guid_t sec_type,
+					ghes_event_handler_t event_handler,
+					void *data)
+{
+	return -ENODEV;
+}
+
+static inline void ghes_unregister_event_handler(guid_t sec_type, void *data)
+{
+}
+#endif
+
 int ghes_estatus_pool_init(int num_ghes);
 
 /* From drivers/edac/ghes_edac.c */
-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors
  2020-01-24 12:39 ` [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
  2020-01-24 12:39   ` [PATCH v2 1/2] " Shiju Jose
@ 2020-01-24 12:39   ` Shiju Jose
  2020-01-24 14:30     ` Bjorn Helgaas
                       ` (2 more replies)
  1 sibling, 3 replies; 23+ messages in thread
From: Shiju Jose @ 2020-01-24 12:39 UTC (permalink / raw)
  To: linux-acpi, linux-pci, linux-kernel, rjw, helgaas, lenb, bp,
	james.morse, tony.luck, gregkh, zhangliguang, tglx
  Cc: linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

From: Yicong Yang <yangyicong@hisilicon.com>

The error handling driver logs and reports hip PCIe controller's
recoverable errors.
Perform root port reset and restore link status for the recovery.

Following are some of the PCIe controller's recoverable errors
1. completion transmission timeout error.
2. CRS retry counter over the threshold error.
3. ECC 2 bit errors
4. AXI bresponse/rresponse errors etc.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
--
 drivers/pci/controller/Kconfig           |   8 +
 drivers/pci/controller/Makefile          |   1 +
 drivers/pci/controller/pcie-hisi-error.c | 336 +++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-hisi-error.c
---
 drivers/pci/controller/Kconfig           |   8 +
 drivers/pci/controller/Makefile          |   1 +
 drivers/pci/controller/pcie-hisi-error.c | 336 +++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-hisi-error.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index c77069c..769fce7 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -260,6 +260,14 @@ config PCI_HYPERV_INTERFACE
 	  The Hyper-V PCI Interface is a helper driver allows other drivers to
 	  have a common interface with the Hyper-V PCI frontend driver.
 
+config PCIE_HISI_ERR
+	depends on ARM64 || COMPILE_TEST
+	depends on (ACPI && PCI_QUIRKS)
+	bool "HiSilicon hip PCIe controller error handling driver"
+	help
+	  Say Y here if you want error handling support
+	  for the PCIe controller's errors on HiSilicon hip SoCs
+
 source "drivers/pci/controller/dwc/Kconfig"
 source "drivers/pci/controller/cadence/Kconfig"
 endmenu
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 3d4f597..2d1565f 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
 obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
 obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
+obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/
 
diff --git a/drivers/pci/controller/pcie-hisi-error.c b/drivers/pci/controller/pcie-hisi-error.c
new file mode 100644
index 0000000..27520ad
--- /dev/null
+++ b/drivers/pci/controller/pcie-hisi-error.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for handling the PCIe controller's errors on
+ * HiSilicon hip SoCs.
+ *
+ * Copyright (c) 2018-2019 HiSilicon Limited.
+ */
+
+#include <linux/acpi.h>
+#include <acpi/ghes.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/list.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock.h>
+
+#include "../pci.h"
+
+#define HISI_PCIE_ERR_RECOVER_RING_SIZE           16
+#define	HISI_PCIE_ERR_INFO_SIZE	1024
+
+/* HISI PCIe controller's error definitions */
+#define HISI_PCIE_ERR_MISC_REGS	33
+
+#define HISI_PCIE_SUB_MODULE_ID_AP	0
+#define HISI_PCIE_SUB_MODULE_ID_TL	1
+#define HISI_PCIE_SUB_MODULE_ID_MAC	2
+#define HISI_PCIE_SUB_MODULE_ID_DL	3
+#define HISI_PCIE_SUB_MODULE_ID_SDI	4
+
+#define HISI_PCIE_LOCAL_VALID_VERSION		BIT(0)
+#define HISI_PCIE_LOCAL_VALID_SOC_ID		BIT(1)
+#define HISI_PCIE_LOCAL_VALID_SOCKET_ID		BIT(2)
+#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID		BIT(3)
+#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID	BIT(4)
+#define HISI_PCIE_LOCAL_VALID_CORE_ID		BIT(5)
+#define HISI_PCIE_LOCAL_VALID_PORT_ID		BIT(6)
+#define HISI_PCIE_LOCAL_VALID_ERR_TYPE		BIT(7)
+#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY	BIT(8)
+#define HISI_PCIE_LOCAL_VALID_ERR_MISC		9
+
+#define HISI_ERR_SEV_RECOVERABLE	0
+#define HISI_ERR_SEV_FATAL		1
+#define HISI_ERR_SEV_CORRECTED		2
+#define HISI_ERR_SEV_NONE		3
+
+guid_t hisi_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
+				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
+
+#define HISI_PCIE_CORE_ID(v)             ((v) >> 3)
+#define HISI_PCIE_PORT_ID(core, v)       (((v) >> 1) + ((core) << 3))
+#define HISI_PCIE_CORE_PORT_ID(v)        (((v) % 8) << 1)
+
+struct hisi_pcie_err_data {
+	u64   val_bits;
+	u8    version;
+	u8    soc_id;
+	u8    socket_id;
+	u8    nimbus_id;
+	u8    sub_module_id;
+	u8    core_id;
+	u8    port_id;
+	u8    err_severity;
+	u16   err_type;
+	u8    reserv[2];
+	u32   err_misc[HISI_PCIE_ERR_MISC_REGS];
+};
+
+struct hisi_pcie_err_info {
+	struct hisi_pcie_err_data err_data;
+	struct platform_device *pdev;
+};
+
+static char *hisi_pcie_sub_module_name(u8 id)
+{
+	switch (id) {
+	case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer";
+	case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer";
+	case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer";
+	case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer";
+	case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer";
+	}
+
+	return "unknown";
+}
+
+static char *hisi_pcie_err_severity(u8 err_sev)
+{
+	switch (err_sev) {
+	case HISI_ERR_SEV_RECOVERABLE: return "recoverable";
+	case HISI_ERR_SEV_FATAL: return "fatal";
+	case HISI_ERR_SEV_CORRECTED: return "corrected";
+	case HISI_ERR_SEV_NONE: return "none";
+	}
+
+	return "unknown";
+}
+
+static int hisi_pcie_port_reset(struct platform_device *pdev,
+					u32 chip_id, u32 port_id)
+{
+	acpi_status s;
+	union acpi_object arg[3];
+	unsigned long long data = 0;
+	struct device *dev = &pdev->dev;
+	struct acpi_object_list arg_list;
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	if (!handle) {
+		dev_err(dev, "No Reset method\n");
+		return -EINVAL;
+	}
+
+	arg[0].type = ACPI_TYPE_INTEGER;
+	arg[0].integer.value = chip_id;
+	arg[1].type = ACPI_TYPE_INTEGER;
+	arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
+	arg[2].type = ACPI_TYPE_INTEGER;
+	arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
+
+	arg_list.count = 3;
+	arg_list.pointer = arg;
+
+	/* Call the ACPI handle to reset root port  */
+	s = acpi_evaluate_integer(handle, "RST", &arg_list, &data);
+	if (ACPI_FAILURE(s)) {
+		dev_err(dev, "No Reset method\n");
+		return -EIO;
+	}
+
+	if (data) {
+		dev_err(dev, "Failed to Reset\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int hisi_pcie_port_do_recovery(struct platform_device *dev,
+				      u32 chip_id, u32 port_id)
+{
+	u32 busnr, devfn;
+	struct pci_dev *pdev;
+	struct pci_bus *root_bus;
+
+	devfn = PCI_DEVFN(port_id, 0);
+	if (device_property_read_u32(&dev->dev, "busnr", &busnr))
+		goto failed;
+
+	pdev = pci_get_domain_bus_and_slot(0, busnr, devfn);
+	if (!pdev)
+		goto failed;
+
+	root_bus = pdev->bus;
+
+	pci_stop_and_remove_bus_device_locked(pdev);
+	pci_dev_put(pdev);
+
+	if (hisi_pcie_port_reset(dev, chip_id, port_id))
+		return -EIO;
+
+	/**
+	 * In pci_reset_secondary_bus(), using 1s delay before subordinates
+	 * devices to be re-initialized. Use the same delay here to ensure
+	 * we can get all the devices after root port reset.
+	 **/
+	ssleep(1UL);
+
+	/* add root port and downstream devices */
+	pci_lock_rescan_remove();
+	pci_rescan_bus(root_bus);
+	pci_unlock_rescan_remove();
+
+	return 0;
+
+failed:
+	dev_info(&(dev->dev), "Fail to get root port device\n");
+	return -ENODEV;
+}
+
+static void hisi_pcie_handle_one_error(const struct hisi_pcie_err_data *err,
+				    struct platform_device *pdev)
+{
+	char buf[HISI_PCIE_ERR_INFO_SIZE];
+	char *p = buf, *end = buf + sizeof(buf);
+	struct device *dev = &pdev->dev;
+	u32 i;
+	int rc;
+
+	if (err->val_bits == 0) {
+		dev_warn(dev, "%s: no valid error information\n", __func__);
+		return;
+	}
+
+	/* Logging */
+	p += snprintf(p, end - p, "[ Table version=%d ", err->version);
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOC_ID)
+		p += snprintf(p, end - p, "SOC ID=%d ", err->soc_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOCKET_ID)
+		p += snprintf(p, end - p, "socket ID=%d ", err->socket_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_NIMBUS_ID)
+		p += snprintf(p, end - p, "nimbus ID=%d ", err->nimbus_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID)
+		p += snprintf(p, end - p, "sub module=%s ",
+			      hisi_pcie_sub_module_name(err->sub_module_id));
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_CORE_ID)
+		p += snprintf(p, end - p, "core ID=core%d ", err->core_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_PORT_ID)
+		p += snprintf(p, end - p, "port ID=port%d ", err->port_id);
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_SEVERITY)
+		p += snprintf(p, end - p, "error severity=%s ",
+			      hisi_pcie_err_severity(err->err_severity));
+
+	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_TYPE)
+		p += snprintf(p, end - p, "error type=0x%x ", err->err_type);
+
+	p += snprintf(p, end - p, "]\n");
+	dev_info(dev, "\nHISI : hip : PCIe controller's error\n");
+	dev_info(dev, "%s\n", buf);
+
+	dev_info(dev, "Reg Dump:\n");
+	for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
+		if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
+			dev_info(dev,
+				 "ERR_MISC_%d=0x%x\n", i, err->err_misc[i]);
+	}
+
+	/* Recovery for the PCIe controller's errors */
+	if (err->err_severity == HISI_ERR_SEV_RECOVERABLE) {
+		/* try reset PCI port for the error recovery */
+		rc = hisi_pcie_port_do_recovery(pdev, err->socket_id,
+				HISI_PCIE_PORT_ID(err->core_id, err->port_id));
+		if (rc) {
+			dev_info(dev, "fail to do hisi pcie port reset\n");
+			return;
+		}
+	}
+}
+
+static DEFINE_KFIFO(hisi_pcie_err_recover_ring, struct hisi_pcie_err_info,
+		    HISI_PCIE_ERR_RECOVER_RING_SIZE);
+static DEFINE_SPINLOCK(hisi_pcie_err_recover_ring_lock);
+
+static void hisi_pcie_err_recover_work_func(struct work_struct *work)
+{
+	struct hisi_pcie_err_info pcie_err_entry;
+
+	while (kfifo_get(&hisi_pcie_err_recover_ring, &pcie_err_entry)) {
+		hisi_pcie_handle_one_error(&pcie_err_entry.err_data,
+					pcie_err_entry.pdev);
+	}
+}
+
+static DECLARE_WORK(hisi_pcie_err_recover_work,
+		    hisi_pcie_err_recover_work_func);
+
+static int hisi_pcie_error_handle(struct acpi_hest_generic_data *gdata,
+				  int sev, void *data)
+{
+	const struct hisi_pcie_err_data *err_data =
+					acpi_hest_get_payload(gdata);
+	struct hisi_pcie_err_info err_info;
+	struct platform_device *pdev = data;
+	struct device *dev = &pdev->dev;
+	u8 socket;
+
+	if (device_property_read_u8(dev, "socket", &socket))
+		return GHES_EVENT_NONE;
+
+	if (err_data->socket_id != socket)
+		return GHES_EVENT_NONE;
+
+	memcpy(&err_info.err_data, err_data, sizeof(*err_data));
+	err_info.pdev = pdev;
+
+	if (kfifo_in_spinlocked(&hisi_pcie_err_recover_ring, &err_info, 1,
+				&hisi_pcie_err_recover_ring_lock))
+		schedule_work(&hisi_pcie_err_recover_work);
+	else
+		dev_warn(dev, "queue full when recovering PCIe controller's error\n");
+
+	return GHES_EVENT_HANDLED;
+}
+
+static int hisi_pcie_err_handler_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = ghes_register_event_handler(hisi_pcie_sec_type,
+					  hisi_pcie_error_handle, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "%s : ghes_register_event_handler fail\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int hisi_pcie_err_handler_remove(struct platform_device *pdev)
+{
+	ghes_unregister_event_handler(hisi_pcie_sec_type, pdev);
+
+	return 0;
+}
+
+static const struct acpi_device_id hisi_pcie_acpi_match[] = {
+	{ "HISI0361", 0 },
+	{ }
+};
+
+static struct platform_driver hisi_pcie_err_handler_driver = {
+	.driver = {
+		.name	= "hisi-pcie-err-handler",
+		.acpi_match_table = hisi_pcie_acpi_match,
+	},
+	.probe		= hisi_pcie_err_handler_probe,
+	.remove		= hisi_pcie_err_handler_remove,
+};
+module_platform_driver(hisi_pcie_err_handler_driver);
+
+MODULE_DESCRIPTION("HiSilicon hip PCIe controller's error handling driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.9.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors
  2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
@ 2020-01-24 14:30     ` Bjorn Helgaas
  2020-01-26 18:12     ` kbuild test robot
  2020-01-26 18:12     ` [RFC PATCH] PCI: hip: hisi_pcie_sec_type can be static kbuild test robot
  2 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2020-01-24 14:30 UTC (permalink / raw)
  To: Shiju Jose
  Cc: linux-acpi, linux-pci, linux-kernel, rjw, lenb, bp, james.morse,
	tony.luck, gregkh, zhangliguang, tglx, linuxarm,
	jonathan.cameron, tanxiaofei, yangyicong

On Fri, Jan 24, 2020 at 12:39:38PM +0000, Shiju Jose wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> The error handling driver logs and reports hip PCIe controller's
> recoverable errors.
> Perform root port reset and restore link status for the recovery.

If the preceding is two paragraphs, there should be a blank line
between them.  If it's a single paragraph, it should be rewrapped to
use the entire line width.

> Following are some of the PCIe controller's recoverable errors
> 1. completion transmission timeout error.
> 2. CRS retry counter over the threshold error.
> 3. ECC 2 bit errors
> 4. AXI bresponse/rresponse errors etc.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> --
>  drivers/pci/controller/Kconfig           |   8 +
>  drivers/pci/controller/Makefile          |   1 +
>  drivers/pci/controller/pcie-hisi-error.c | 336 +++++++++++++++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-hisi-error.c
> ---
>  drivers/pci/controller/Kconfig           |   8 +
>  drivers/pci/controller/Makefile          |   1 +
>  drivers/pci/controller/pcie-hisi-error.c | 336 +++++++++++++++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-hisi-error.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index c77069c..769fce7 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -260,6 +260,14 @@ config PCI_HYPERV_INTERFACE
>  	  The Hyper-V PCI Interface is a helper driver allows other drivers to
>  	  have a common interface with the Hyper-V PCI frontend driver.
>  
> +config PCIE_HISI_ERR
> +	depends on ARM64 || COMPILE_TEST
> +	depends on (ACPI && PCI_QUIRKS)

Why does this depend on PCI_QUIRKS?  If it's needed, please mention
the reason somewhere (maybe in the commit log, since there's not
really a good way to do it in Kconfig itself).

> +	bool "HiSilicon hip PCIe controller error handling driver"
> +	help
> +	  Say Y here if you want error handling support
> +	  for the PCIe controller's errors on HiSilicon hip SoCs

"hip" above refers to the hardware device and should be capitalized
(two occurrences above and at least two below).

>  source "drivers/pci/controller/dwc/Kconfig"
>  source "drivers/pci/controller/cadence/Kconfig"
>  endmenu
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 3d4f597..2d1565f 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
>  obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
>  obj-$(CONFIG_VMD) += vmd.o
> +obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
>  # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>  obj-y				+= dwc/
>  
> diff --git a/drivers/pci/controller/pcie-hisi-error.c b/drivers/pci/controller/pcie-hisi-error.c
> new file mode 100644
> index 0000000..27520ad
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-hisi-error.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for handling the PCIe controller's errors on
> + * HiSilicon hip SoCs.
> + *
> + * Copyright (c) 2018-2019 HiSilicon Limited.
> + */
> +
> +#include <linux/acpi.h>
> +#include <acpi/ghes.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/kfifo.h>
> +#include <linux/spinlock.h>

Why do you need bitfield.h, bitops.h, irq.h, irqdomain.h, list.h,
resource.h?  You *do* need bits.h, which is included by at least
bitops.h.  But if you don't need bitops.h itself, include bits.h
directly instead.

> +
> +#include "../pci.h"
> +
> +#define HISI_PCIE_ERR_RECOVER_RING_SIZE           16
> +#define	HISI_PCIE_ERR_INFO_SIZE	1024
> +
> +/* HISI PCIe controller's error definitions */
> +#define HISI_PCIE_ERR_MISC_REGS	33
> +
> +#define HISI_PCIE_SUB_MODULE_ID_AP	0
> +#define HISI_PCIE_SUB_MODULE_ID_TL	1
> +#define HISI_PCIE_SUB_MODULE_ID_MAC	2
> +#define HISI_PCIE_SUB_MODULE_ID_DL	3
> +#define HISI_PCIE_SUB_MODULE_ID_SDI	4
> +
> +#define HISI_PCIE_LOCAL_VALID_VERSION		BIT(0)
> +#define HISI_PCIE_LOCAL_VALID_SOC_ID		BIT(1)
> +#define HISI_PCIE_LOCAL_VALID_SOCKET_ID		BIT(2)
> +#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID		BIT(3)
> +#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID	BIT(4)
> +#define HISI_PCIE_LOCAL_VALID_CORE_ID		BIT(5)
> +#define HISI_PCIE_LOCAL_VALID_PORT_ID		BIT(6)
> +#define HISI_PCIE_LOCAL_VALID_ERR_TYPE		BIT(7)
> +#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY	BIT(8)
> +#define HISI_PCIE_LOCAL_VALID_ERR_MISC		9
> +
> +#define HISI_ERR_SEV_RECOVERABLE	0
> +#define HISI_ERR_SEV_FATAL		1
> +#define HISI_ERR_SEV_CORRECTED		2
> +#define HISI_ERR_SEV_NONE		3
> +
> +guid_t hisi_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
> +				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
> +
> +#define HISI_PCIE_CORE_ID(v)             ((v) >> 3)
> +#define HISI_PCIE_PORT_ID(core, v)       (((v) >> 1) + ((core) << 3))
> +#define HISI_PCIE_CORE_PORT_ID(v)        (((v) % 8) << 1)
> +
> +struct hisi_pcie_err_data {
> +	u64   val_bits;
> +	u8    version;
> +	u8    soc_id;
> +	u8    socket_id;
> +	u8    nimbus_id;
> +	u8    sub_module_id;
> +	u8    core_id;
> +	u8    port_id;
> +	u8    err_severity;
> +	u16   err_type;
> +	u8    reserv[2];
> +	u32   err_misc[HISI_PCIE_ERR_MISC_REGS];
> +};
> +
> +struct hisi_pcie_err_info {
> +	struct hisi_pcie_err_data err_data;
> +	struct platform_device *pdev;
> +};
> +
> +static char *hisi_pcie_sub_module_name(u8 id)
> +{
> +	switch (id) {
> +	case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer";
> +	case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer";
> +	}
> +
> +	return "unknown";
> +}
> +
> +static char *hisi_pcie_err_severity(u8 err_sev)
> +{
> +	switch (err_sev) {
> +	case HISI_ERR_SEV_RECOVERABLE: return "recoverable";
> +	case HISI_ERR_SEV_FATAL: return "fatal";
> +	case HISI_ERR_SEV_CORRECTED: return "corrected";
> +	case HISI_ERR_SEV_NONE: return "none";
> +	}
> +
> +	return "unknown";
> +}
> +
> +static int hisi_pcie_port_reset(struct platform_device *pdev,
> +					u32 chip_id, u32 port_id)
> +{
> +	acpi_status s;
> +	union acpi_object arg[3];
> +	unsigned long long data = 0;
> +	struct device *dev = &pdev->dev;
> +	struct acpi_object_list arg_list;
> +	acpi_handle handle = ACPI_HANDLE(dev);

Order these as:

  struct device *dev = &pdev->dev;
  acpi_handle handle = ACPI_HANDLE(dev);
  union acpi_object arg[3];
  ...

so they're in order of use.

> +
> +	if (!handle) {
> +		dev_err(dev, "No Reset method\n");

Technically it's not the RST *method* that's missing; it's the entire
object that would *enclose* the RST method.  I guess that would be the
HISI0361 device?  Is it even possible for that to not exist, since
this driver binds to that device?

> +		return -EINVAL;
> +	}
> +
> +	arg[0].type = ACPI_TYPE_INTEGER;
> +	arg[0].integer.value = chip_id;
> +	arg[1].type = ACPI_TYPE_INTEGER;
> +	arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
> +	arg[2].type = ACPI_TYPE_INTEGER;
> +	arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
> +
> +	arg_list.count = 3;
> +	arg_list.pointer = arg;
> +
> +	/* Call the ACPI handle to reset root port  */

s/root port  /root port / (mentioned last time as well)
(Remove the extra space after "root port")

This isn't actually "calling the ACPI handle"; the handle is a
reference to the HISI0361 device, which *contains* the RST method.
You could just drop the comment altogether since it's pretty obvious
what's going on.

> +	s = acpi_evaluate_integer(handle, "RST", &arg_list, &data);
> +	if (ACPI_FAILURE(s)) {
> +		dev_err(dev, "No Reset method\n");

I think it'd be better to use the exact method name ("RST") in the
message.

> +		return -EIO;
> +	}
> +
> +	if (data) {
> +		dev_err(dev, "Failed to Reset\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hisi_pcie_port_do_recovery(struct platform_device *dev,
> +				      u32 chip_id, u32 port_id)
> +{
> +	u32 busnr, devfn;
> +	struct pci_dev *pdev;
> +	struct pci_bus *root_bus;
> +
> +	devfn = PCI_DEVFN(port_id, 0);
> +	if (device_property_read_u32(&dev->dev, "busnr", &busnr))
> +		goto failed;
> +
> +	pdev = pci_get_domain_bus_and_slot(0, busnr, devfn);
> +	if (!pdev)
> +		goto failed;
> +
> +	root_bus = pdev->bus;
> +
> +	pci_stop_and_remove_bus_device_locked(pdev);
> +	pci_dev_put(pdev);
> +
> +	if (hisi_pcie_port_reset(dev, chip_id, port_id))
> +		return -EIO;
> +
> +	/**
> +	 * In pci_reset_secondary_bus(), using 1s delay before subordinates
> +	 * devices to be re-initialized. Use the same delay here to ensure
> +	 * we can get all the devices after root port reset.

Use normal comment style (no "**").  Include the specific spec section
reference, e.g., PCIe r5.0, sec XXX.

> +	 **/
> +	ssleep(1UL);
> +
> +	/* add root port and downstream devices */
> +	pci_lock_rescan_remove();
> +	pci_rescan_bus(root_bus);
> +	pci_unlock_rescan_remove();
> +
> +	return 0;
> +
> +failed:
> +	dev_info(&(dev->dev), "Fail to get root port device\n");

For debugging purposes, include the address of the root port you tried
to find, e.g., ssss:bb:dd.f from segment, busnr, and devfn.

Since you're using ACPI, the segment number should not have to be
hard-coded; it should come from the host bridge's _SEG method.  I
don't know how you *find* that.  Is the HISI0361 device in the scope
of the PNP0A08 device, so you could just search up the chain?  You
need some way to associate this with the host bridge device.

> +	return -ENODEV;
> +}
> +
> +static void hisi_pcie_handle_one_error(const struct hisi_pcie_err_data *err,
> +				    struct platform_device *pdev)
> +{
> +	char buf[HISI_PCIE_ERR_INFO_SIZE];
> +	char *p = buf, *end = buf + sizeof(buf);
> +	struct device *dev = &pdev->dev;
> +	u32 i;
> +	int rc;
> +
> +	if (err->val_bits == 0) {
> +		dev_warn(dev, "%s: no valid error information\n", __func__);
> +		return;
> +	}
> +
> +	/* Logging */
> +	p += snprintf(p, end - p, "[ Table version=%d ", err->version);
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOC_ID)
> +		p += snprintf(p, end - p, "SOC ID=%d ", err->soc_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SOCKET_ID)
> +		p += snprintf(p, end - p, "socket ID=%d ", err->socket_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_NIMBUS_ID)
> +		p += snprintf(p, end - p, "nimbus ID=%d ", err->nimbus_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID)
> +		p += snprintf(p, end - p, "sub module=%s ",
> +			      hisi_pcie_sub_module_name(err->sub_module_id));
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_CORE_ID)
> +		p += snprintf(p, end - p, "core ID=core%d ", err->core_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_PORT_ID)
> +		p += snprintf(p, end - p, "port ID=port%d ", err->port_id);
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_SEVERITY)
> +		p += snprintf(p, end - p, "error severity=%s ",
> +			      hisi_pcie_err_severity(err->err_severity));
> +
> +	if (err->val_bits & HISI_PCIE_LOCAL_VALID_ERR_TYPE)
> +		p += snprintf(p, end - p, "error type=0x%x ", err->err_type);
> +
> +	p += snprintf(p, end - p, "]\n");
> +	dev_info(dev, "\nHISI : hip : PCIe controller's error\n");
> +	dev_info(dev, "%s\n", buf);
> +
> +	dev_info(dev, "Reg Dump:\n");
> +	for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
> +		if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
> +			dev_info(dev,
> +				 "ERR_MISC_%d=0x%x\n", i, err->err_misc[i]);
> +	}
> +
> +	/* Recovery for the PCIe controller's errors */
> +	if (err->err_severity == HISI_ERR_SEV_RECOVERABLE) {
> +		/* try reset PCI port for the error recovery */
> +		rc = hisi_pcie_port_do_recovery(pdev, err->socket_id,
> +				HISI_PCIE_PORT_ID(err->core_id, err->port_id));
> +		if (rc) {
> +			dev_info(dev, "fail to do hisi pcie port reset\n");
> +			return;
> +		}
> +	}
> +}
> +
> +static DEFINE_KFIFO(hisi_pcie_err_recover_ring, struct hisi_pcie_err_info,
> +		    HISI_PCIE_ERR_RECOVER_RING_SIZE);
> +static DEFINE_SPINLOCK(hisi_pcie_err_recover_ring_lock);
> +
> +static void hisi_pcie_err_recover_work_func(struct work_struct *work)
> +{
> +	struct hisi_pcie_err_info pcie_err_entry;
> +
> +	while (kfifo_get(&hisi_pcie_err_recover_ring, &pcie_err_entry)) {
> +		hisi_pcie_handle_one_error(&pcie_err_entry.err_data,
> +					pcie_err_entry.pdev);
> +	}
> +}
> +
> +static DECLARE_WORK(hisi_pcie_err_recover_work,
> +		    hisi_pcie_err_recover_work_func);
> +
> +static int hisi_pcie_error_handle(struct acpi_hest_generic_data *gdata,
> +				  int sev, void *data)
> +{
> +	const struct hisi_pcie_err_data *err_data =
> +					acpi_hest_get_payload(gdata);
> +	struct hisi_pcie_err_info err_info;
> +	struct platform_device *pdev = data;
> +	struct device *dev = &pdev->dev;
> +	u8 socket;
> +
> +	if (device_property_read_u8(dev, "socket", &socket))
> +		return GHES_EVENT_NONE;
> +
> +	if (err_data->socket_id != socket)
> +		return GHES_EVENT_NONE;
> +
> +	memcpy(&err_info.err_data, err_data, sizeof(*err_data));
> +	err_info.pdev = pdev;
> +
> +	if (kfifo_in_spinlocked(&hisi_pcie_err_recover_ring, &err_info, 1,
> +				&hisi_pcie_err_recover_ring_lock))
> +		schedule_work(&hisi_pcie_err_recover_work);
> +	else
> +		dev_warn(dev, "queue full when recovering PCIe controller's error\n");
> +
> +	return GHES_EVENT_HANDLED;
> +}
> +
> +static int hisi_pcie_err_handler_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	ret = ghes_register_event_handler(hisi_pcie_sec_type,
> +					  hisi_pcie_error_handle, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s : ghes_register_event_handler fail\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hisi_pcie_err_handler_remove(struct platform_device *pdev)
> +{
> +	ghes_unregister_event_handler(hisi_pcie_sec_type, pdev);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id hisi_pcie_acpi_match[] = {
> +	{ "HISI0361", 0 },
> +	{ }
> +};
> +
> +static struct platform_driver hisi_pcie_err_handler_driver = {
> +	.driver = {
> +		.name	= "hisi-pcie-err-handler",
> +		.acpi_match_table = hisi_pcie_acpi_match,
> +	},
> +	.probe		= hisi_pcie_err_handler_probe,
> +	.remove		= hisi_pcie_err_handler_remove,
> +};
> +module_platform_driver(hisi_pcie_err_handler_driver);
> +
> +MODULE_DESCRIPTION("HiSilicon hip PCIe controller's error handling driver");
> +MODULE_LICENSE("GPL v2");
> +
> -- 
> 1.9.1
> 
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors
  2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
  2020-01-24 14:30     ` Bjorn Helgaas
@ 2020-01-26 18:12     ` kbuild test robot
  2020-01-26 18:12     ` [RFC PATCH] PCI: hip: hisi_pcie_sec_type can be static kbuild test robot
  2 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2020-01-26 18:12 UTC (permalink / raw)
  To: Shiju Jose
  Cc: kbuild-all, linux-acpi, linux-pci, linux-kernel, rjw, helgaas,
	lenb, bp, james.morse, tony.luck, gregkh, zhangliguang, tglx,
	linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose

Hi Shiju,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on linux/master linus/master v5.5-rc7]
[cannot apply to pci/next next-20200121]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Shiju-Jose/ACPI-APEI-Add-support-to-notify-the-vendor-specific-HW-errors/20200125-171952
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-153-g47b6dfef-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/pci/controller/pcie-hisi-error.c:53:8: sparse: sparse: symbol 'hisi_pcie_sec_type' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH] PCI: hip: hisi_pcie_sec_type can be static
  2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
  2020-01-24 14:30     ` Bjorn Helgaas
  2020-01-26 18:12     ` kbuild test robot
@ 2020-01-26 18:12     ` kbuild test robot
  2 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2020-01-26 18:12 UTC (permalink / raw)
  To: Shiju Jose
  Cc: kbuild-all, linux-acpi, linux-pci, linux-kernel, rjw, helgaas,
	lenb, bp, james.morse, tony.luck, gregkh, zhangliguang, tglx,
	linuxarm, jonathan.cameron, tanxiaofei, yangyicong, Shiju Jose


Fixes: 9bf6eb234cd2 ("PCI: hip: Add handling of HiSilicon hip PCIe controller's errors")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 pcie-hisi-error.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-hisi-error.c b/drivers/pci/controller/pcie-hisi-error.c
index 7c669043a4e0e..76c37abaaf968 100644
--- a/drivers/pci/controller/pcie-hisi-error.c
+++ b/drivers/pci/controller/pcie-hisi-error.c
@@ -50,7 +50,7 @@
 #define HISI_ERR_SEV_CORRECTED		2
 #define HISI_ERR_SEV_NONE		3
 
-guid_t hisi_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
+static guid_t hisi_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, 0xA8, 0x67,
 				       0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
 
 #define HISI_PCIE_CORE_ID(v)             ((v) >> 3)

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <Shiju Jose>
2019-08-12 10:11 ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
2019-08-12 10:11   ` [PATCH RFC 1/4] " Shiju Jose
2019-08-21 17:23     ` James Morse
2019-08-22 16:57       ` Shiju Jose
2019-08-12 10:11   ` [PATCH RFC 2/4] ACPI: APEI: Add ghes_handle_memory_failure to the new notification method Shiju Jose
2019-08-21 17:22     ` James Morse
2019-08-22 16:57       ` Shiju Jose
2019-08-12 10:11   ` [PATCH RFC 3/4] ACPI: APEI: Add ghes_handle_aer " Shiju Jose
2019-08-12 10:11   ` [PATCH RFC 4/4] ACPI: APEI: Add log_arm_hw_error " Shiju Jose
2019-08-21 17:22   ` [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor specific HW errors James Morse
2019-08-22 16:56     ` Shiju Jose
2019-10-03 17:21       ` James Morse
2020-01-15 11:01 ` [RFC PATCH 0/2] " Shiju Jose
2020-01-15 11:01   ` [RFC PATCH 1/2] " Shiju Jose
2020-01-15 11:01   ` [RFC PATCH 2/2] PCI:hip08:Add driver to handle HiSilicon hip08 PCIe controller's errors Shiju Jose
2020-01-15 14:13     ` Bjorn Helgaas
2020-01-17  9:40       ` Shiju Jose
2020-01-24 12:39 ` [PATCH v2 0/2] ACPI: APEI: Add support to notify the vendor specific HW errors Shiju Jose
2020-01-24 12:39   ` [PATCH v2 1/2] " Shiju Jose
2020-01-24 12:39   ` [PATCH v2 2/2] PCI: hip: Add handling of HiSilicon hip PCIe controller's errors Shiju Jose
2020-01-24 14:30     ` Bjorn Helgaas
2020-01-26 18:12     ` kbuild test robot
2020-01-26 18:12     ` [RFC PATCH] PCI: hip: hisi_pcie_sec_type can be static kbuild test robot

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git