Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 09:52:13 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Terry Bowman <terry.bowman@amd.com>
Cc:     alison.schofield@intel.com, vishal.l.verma@intel.com,
        dave.jiang@intel.com, ira.weiny@intel.com, bwidawsk@kernel.org,
        dan.j.williams@intel.com, linux-cxl@vger.kernel.org,
        linux-kernel@vger.kernel.org, bhelgaas@google.com,
        rafael@kernel.org, lenb@kernel.org, Jonathan.Cameron@huawei.com,
        dave@stgolabs.net, rrichter@amd.com
Subject: Re: [PATCH 2/5] cxl/pci: Discover and cache pointer to RCD dport's
 PCIe AER capability
Message-ID: <20221027145213.GA828055@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221021185615.605233-3-terry.bowman@amd.com>
Precedence: bulk

On Fri, Oct 21, 2022 at 01:56:12PM -0500, Terry Bowman wrote:
> CXL downport PCIe AER information needs to be logged during error handling.
> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
> result the CXL PCIe driver is not aware of the AER in 'PCI Express'
> capability located in the RCRB downport/upport. Logic must be introduced to
> use the downport/upport AER information.

I assume "downport" is the same as "dport" in "cxl_dport" and means
"Downstream Port".  Might be nice to reduce the number of variations
if feasible.

> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id)
> +{
> +	resource_size_t offset, rcrb;
> +	void *rcrb_mapped;
> +	u32 cap_hdr;
> +
> +	rcrb = cxl_get_rcrb(cxlmd);
> +	if (!rcrb)
> +		return 0;
> +
> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
> +	if (!rcrb_mapped)
> +		return 0;
> +
> +	offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST);
> +	cap_hdr = readl(rcrb_mapped + offset);
> +
> +	while (PCI_CAP_ID(cap_hdr)) {
> +		if (PCI_CAP_ID(cap_hdr) == cap_id)
> +			break;
> +
> +		offset = PCI_CAP_NEXT(cap_hdr);
> +		if (offset == 0)
> +			break;
> +
> +		cap_hdr = readl(rcrb_mapped + offset);
> +	}
> +	iounmap((void *)rcrb_mapped);
> +
> +	if (PCI_CAP_ID(cap_hdr) != cap_id)
> +		return 0;
> +
> +	pr_debug("Found capability %X @ %llX (%X)\n",
> +		 cap_id, rcrb + offset, cap_hdr);

Would be nice to use dev_dbg() if possible here.

Is "%X" (upper-case hex) the convention in CXL?  Most places in Linux
seem to use "%x".  Also consider "%#x" (or "%#X") so it's obvious
these are hex.

> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd)
> +{
> +	resource_size_t cap_base;
> +
> +	/* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */
> +	if (!is_rcd(cxlmd))
> +		return;
> +
> +	/*
> +	 * Read base address of the PCI express cap. Cache the cap's
> +	 * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status.
> +	 */
> +	cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP);
> +	cxl_setup_dport_aer(cxlmd, cap_base);

I don't see anything about PCI_EXP_DEVCTL and PCI_EXP_DEVSTA in
cxl_get_dport_cap() or cxl_setup_dport_aer().  And I don't see any
caching, except for setting map->base in cxl_setup_dport_aer().

Caching those registers, especially PCI_EXP_DEVSTA, doesn't seem like
it would make much sense anyway since bits there are set by hardware
when things happen.