On Mon, Jun 15, 2020 at 11:15:52AM +0100, Shiju Jose wrote:
> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>
> The HiSilicon HIP PCIe controller is capable of handling errors
> on root port and perform port reset separately at each root port.
>
> Add error handling driver for HIP PCIe controller to log
> and report recoverable errors. Perform root port reset and restore
> link status after 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.
>
> The driver placed in the drivers/pci/controller/ because the
> HIP PCIe controller does not use DWC ip.
...
> +#include <linux/acpi.h>
> +#include <acpi/ghes.h>
bits.h ?
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/kfifo.h>
> +#include <linux/spinlock.h>
...
> +static guid_t hisi_pcie_sec_type = GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
> + 0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
Can we have it in more common pattern, i.e.
static guid_t hisi_pcie_sec_type =
GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
?
...
> +#define HISI_PCIE_CORE_PORT_ID(v) (((v) % 8) << 1)
% -> & ?
...
> +struct hisi_pcie_error_private {
> + struct notifier_block nb;
> + struct platform_device *pdev;
Do you really need platform device? Isn't struct device * enough?
> +};
...
> +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";
> + }
match_string() ?
> + return "unknown";
> +}
> +
> +static char *hisi_pcie_error_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";
> + }
Ditto?
> + return "unknown";
> +}
...
> + pdev = pci_get_domain_bus_and_slot(domain, busnr, devfn);
> + if (!pdev) {
> + dev_info(device, "Fail to get root port %04x:%02x:%02x.%d device\n",
> + domain, busnr, PCI_SLOT(devfn), PCI_FUNC(devfn));
pci_info() ?
> + return -ENODEV;
> + }
...
> + /*
> + * The initialization time of subordinate devices after
> + * hot reset is no more than 1s, which is required by
> + * the PCI spec v5.0 sec 6.6.1. The time will shorten
> + * if Readiness Notifications mechanisms are used. But
> + * wait 1s here to adapt any conditions.
> + */
> + ssleep(1UL);
It's a huge time out... Can we reduce it somehow?
...
> + for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) {
> + if (edata->val_bits &
> + BIT_ULL(HISI_PCIE_LOCAL_VALID_ERR_MISC + i))
for_each_set_bit() ?
> + dev_info(dev,
> + "ERR_MISC_%d = 0x%x\n", i, edata->err_misc[i]);
> + }
> +
> + /* Recovery for the PCIe controller errors */
> + if (edata->err_severity == HISI_ERR_SEV_RECOVERABLE) {
Perhaps negative conditional?
> + /* try reset PCI port for the error recovery */
> + rc = hisi_pcie_port_do_recovery(pdev, edata->socket_id,
> + HISI_PCIE_PORT_ID(edata->core_id, edata->port_id));
> + if (rc) {
> + dev_info(dev, "fail to do hisi pcie port reset\n");
> + return;
redundant.
> + }
> + }
...
> + const struct hisi_pcie_error_data *error_data =
> + acpi_hest_get_payload(gdata);
One line is better to read.
> + struct platform_device *pdev = priv->pdev;
> + hisi_pcie_handle_error(pdev, error_data);
And how exactly _platform_ device pointer is being used?
...
> + dev_err(&pdev->dev, "%s : ghes_register_event_notifier fail\n",
> + __func__);
Make error message more descriptive that __func__ will not be needed.
...
> + kfree(priv);
Double free?
...
> +static const struct acpi_device_id hisi_pcie_acpi_match[] = {
> + { "HISI0361", 0 },
', 0' part is not necessary to have.
> + { }
> +};
--
With Best Regards,
Andy Shevchenko
