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
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 <[email protected]>
---
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
From: Yicong Yang <[email protected]>
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 <[email protected]>
Signed-off-by: Shiju Jose <[email protected]>
---
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
Follow convention for subject line.
On Wed, Jan 15, 2020 at 11:01:40AM +0000, Shiju Jose wrote:
> From: Yicong Yang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Shiju Jose <[email protected]>
> ---
> 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
>
>
Hi Bjorn,
Thanks for reviewing the patch.
Please find reply inline.
>-----Original Message-----
>From: Bjorn Helgaas [mailto:[email protected]]
>Sent: 15 January 2020 14:14
>To: Shiju Jose <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; Linuxarm
><[email protected]>; Jonathan Cameron
><[email protected]>; tanxiaofei <[email protected]>;
>yangyicong <[email protected]>
>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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Shiju Jose <[email protected]>
>> ---
>> 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