Re: [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Sean V Kelley <sean.v.kelley@intel.com>
Cc: <bhelgaas@google.com>, <rjw@rjwysocki.net>,
	<ashok.raj@kernel.org>, <tony.luck@intel.com>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	<linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Subject: Re: [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC
Date: Mon, 27 Jul 2020 13:30:10 +0100	[thread overview]
Message-ID: <20200727133010.00003de8@Huawei.com> (raw)
In-Reply-To: <20200724172223.145608-3-sean.v.kelley@intel.com>

On Fri, 24 Jul 2020 10:22:16 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
> optionally be sent to a corresponding Root Complex Event Collector (RCEC).
> Each RCiEP must be associated with no more than one RCEC. Interface errors
> are reported to the OS by RCECs.
> 
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
> 
> Given the commonality with Root Ports and the need to also support AER
> and PME services for RCECs, extend the Root Port driver to support RCEC
> devices through the addition of the RCEC Class ID to the driver
> structure.
> 
Hi.

I'm surprised it ended up this simple :) Seems we are a bit lucky that
the existing code is rather flexible on what is there and what isn't
and that there is never any reason to directly touch the various
type1 specific registers (given as I read the spec, an RCEC uses a
type0 config space header unlike the ports).

Given you mention PME, it's probably worth noting (I think) you aren't
actually enabling the service yet as there is a check in that path on whether we
have a root port or not.
https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/pci/pcie/portdrv_core.c#L241

Thanks,

Jonathan

> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 3acf151ae015..d5b109499b10 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	if (!pci_is_pcie(dev) ||
>  	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
>  	status = pcie_port_device_register(dev);
> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>  	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> +	/* handle any Root Complex Event Collector */
> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>  	{ },
>  };
>  