Messages in this thread | | | From | Shiju Jose <> | Subject | RE: [PATCH v5 2/2] PCI: HIP: Add handling of HiSilicon HIP PCIe controller errors | Date | Thu, 26 Mar 2020 09:47:52 +0000 |
| |
Hi Bjorn,
Thanks for the feedbacks.
>-----Original Message----- >From: Bjorn Helgaas [mailto:helgaas@kernel.org] >Sent: 25 March 2020 17:37 >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>; Dan Carpenter ><dan.carpenter@oracle.com> >Subject: Re: [PATCH v5 2/2] PCI: HIP: Add handling of HiSilicon HIP PCIe >controller errors > >[+cc Dan] > >On Wed, Mar 25, 2020 at 01:55:18PM +0000, Shiju Jose wrote: >> The HiSilicon HIP PCIe controller is capable of handling errors on >> root port and perform port reset separately at each root port. >> >> This patch 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. >> >> Also fix the following Smatch warning: >> warn: should '((((1))) << (9 + i))' be a 64 bit type? >> if (err->val_bits & BIT(HISI_PCIE_LOCAL_VALID_ERR_MISC + i)) >> ^^^ This should be BIT_ULL() because it goes up to 9 + 32. >> Reported-by: kbuild test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >I'm glad you did this fix, and thanks for acknowledging Dan, but I don't think it's >necessary to mention it in the commit log here because it won't really be useful >in the future. It's only relevant when comparing the unmerged versions of this >series, e.g., v4 compared to v3. Sure. We will delete this.
> >If we were fixing something that's already been merged upstream, we should >absolutely include this, but since this hasn't been merged yet Dan's report is >basically the same as other review comments, which we normally just address >and mention in the change history in the [0/n] cover letter (as you're already >doing, thanks for that!).
> >Also, I think it's nice to CC: anybody who has commented on previous versions >of the patch series, so I added Dan to the CC: list here. >That way he can chime in if we're not addressing his report correctly. > >> 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 > >As I mentioned in the other message, I think this file should be >drivers/pci/controller/dwc/pcie-hisi-error.c so it's right next to pcie-hisi.c. If >there's some reason it needs to be here instead, please mention that in the >commit log. > >> --- >> drivers/pci/controller/Kconfig | 8 + >> drivers/pci/controller/Makefile | 1 + >> drivers/pci/controller/pcie-hisi-error.c | 357 >> +++++++++++++++++++++++ >> 3 files changed, 366 insertions(+) >> create mode 100644 drivers/pci/controller/pcie-hisi-error.c > >> +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; >> +}; >> + >> +struct hisi_pcie_err_private { >> + struct notifier_block nb; >> + struct platform_device *pdev; >> +}; > >Either align all the struct members or none of them. Currently >hisi_pcie_err_data is aligned but hisi_pcie_err_info and hisi_pcie_err_private >are not. We will align the structures.
> >> + /* Call the ACPI handle to reset root port */ > >Superfluous comment. We will remove this comment.
> >> + s = acpi_evaluate_integer(handle, "RST", &arg_list, &data); >> + if (ACPI_FAILURE(s)) { >> + dev_err(dev, "No RST method\n"); >> + return -EIO; >> + } > >> +static void hisi_pcie_handle_one_error(const struct >> +hisi_pcie_err_data >> *err, >> + struct platform_device *pdev) > >Align "struct platform_device ..." under "const struct hisi_pcie_err_data ...". sure.
> >Bjorn
Thanks, Shiju
| |