Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
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@huawei.com,
	jonathan.cameron@huawei.com, tanxiaofei@huawei.com,
	yangyicong@hisilicon.com,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v5 2/2] PCI: HIP: Add handling of HiSilicon HIP PCIe controller errors
Date: Wed, 25 Mar 2020 12:36:39 -0500
Message-ID: <20200325173639.GA484@google.com> (raw)
In-Reply-To: <24330bd8-afaa-d7ac-594c-f9fda4242400@huawei.com>

[+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.

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.

> +	/* Call the ACPI handle to reset root port */

Superfluous 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 ...".

Bjorn

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 13:55 Shiju Jose
2020-03-25 17:36 ` Bjorn Helgaas [this message]
2020-03-26  9:47   ` Shiju Jose
2020-04-03 10:23   ` Dan Carpenter
2020-04-03 13:33     ` Bjorn Helgaas
2020-04-03 14:41       ` Dan Carpenter
2020-03-25 14:11 Shiju Jose

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200325173639.GA484@google.com \
    --to=helgaas@kernel.org \
    --cc=bp@alien8.de \
    --cc=dan.carpenter@oracle.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=james.morse@arm.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=rjw@rjwysocki.net \
    --cc=shiju.jose@huawei.com \
    --cc=tanxiaofei@huawei.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=yangyicong@hisilicon.com \
    --cc=zhangliguang@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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