Re: [PATCH v5 24/26] cxl/pci: Add RCH downstream port error logging

[Terry Bowman <Terry.Bowman@amd.com>]

Hi Dan,

On 6/16/23 11:17, Terry Bowman wrote:
> Hi Dan,
> 
> I added responses below.
> 
> On 6/12/23 16:38, Dan Williams wrote:
>> Terry Bowman wrote:
>>> RCH downstream port error logging is missing in the current CXL driver. The
>>> missing AER and RAS error logging is needed for communicating driver error
>>> details to userspace. Update the driver to include PCIe AER and CXL RAS
>>> error logging.
>>>
>>> Add RCH downstream port error handling into the existing RCiEP handler.
>>> The downstream port error handler is added to the RCiEP error handler
>>> because the downstream port is implemented in a RCRB, is not PCI
>>> enumerable, and as a result is not directly accessible to the PCI AER
>>> root port driver. The AER root port driver calls the RCiEP handler for
>>> handling RCD errors and RCH downstream port protocol errors.
>>>
>>> Update existing RCiEP correctable and uncorrectable handlers to also call
>>> the RCH handler. The RCH handler will read the RCH AER registers, check for
>>> error severity, and if an error exists will log using an existing kernel
>>> AER trace routine. The RCH handler will also log downstream port RAS errors
>>> if they exist.
>>>
>>> Co-developed-by: Robert Richter <rrichter@amd.com>
>>> Signed-off-by: Robert Richter <rrichter@amd.com>
>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> ---
>>>  drivers/cxl/core/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 98 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index def6ee5ab4f5..97886aacc64a 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>>> @@ -5,6 +5,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/pci-doe.h>
>>> +#include <linux/aer.h>
>>>  #include <cxlpci.h>
>>>  #include <cxlmem.h>
>>>  #include <cxl.h>
>>> @@ -747,10 +748,105 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds)
>>>  	return __cxl_report_and_clear(cxlds, cxlds->regs.ras);
>>>  }
>>>  
>>> +#ifdef CONFIG_PCIEAER_CXL
>>
>> A general reaction to the "ifdef in a .c file" style recommendation.
>> Maybe this section could move to a drivers/cxl/core/aer.c file, and be
>> optionally compiled by config in the Makefile? I.e. similar to:
>>
>> cxl_core-$(CONFIG_TRACING) += trace.o
>> cxl_core-$(CONFIG_CXL_REGION) += region.o
>>
>> ...it is borderline just big enough, but I'll leave it up to you.
>>
> 
> 
> I'll take a look at this. We have most of the patchset requests implplemented
> and will give me time to look at this.
> 
>>> +
>>> +static void cxl_log_correctable_ras_dport(struct cxl_dev_state *cxlds,
>>> +					  struct cxl_dport *dport)
>>> +{
>>> +	return __cxl_log_correctable_ras(cxlds, dport->regs.ras);
>>> +}
>>> +
>>> +static bool cxl_report_and_clear_dport(struct cxl_dev_state *cxlds,
>>> +				       struct cxl_dport *dport)
>>> +{
>>> +	return __cxl_report_and_clear(cxlds, dport->regs.ras);
>>> +}
>>> +
>>> +/*
>>> + * Copy the AER capability registers using 32 bit read accesses.
>>> + * This is necessary because RCRB AER capability is MMIO mapped. Clear the
>>> + * status after copying.
>>> + *
>>> + * @aer_base: base address of AER capability block in RCRB
>>> + * @aer_regs: destination for copying AER capability
>>> + */
>>> +static bool cxl_rch_get_aer_info(void __iomem *aer_base,
>>> +				 struct aer_capability_regs *aer_regs)
>>> +{
>>> +	int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
>>> +	u32 *aer_regs_buf = (u32 *)aer_regs;
>>> +	int n;
>>> +
>>> +	if (!aer_base)
>>> +		return false;
>>> +
>>> +	/* Use readl() to guarantee 32-bit accesses */
>>> +	for (n = 0; n < read_cnt; n++)
>>> +		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
>>> +
>>> +	writel(aer_regs->uncor_status, aer_base + PCI_ERR_UNCOR_STATUS);
>>> +	writel(aer_regs->cor_status, aer_base + PCI_ERR_COR_STATUS);
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +/* Get AER severity. Return false if there is no error. */
>>> +static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
>>> +				     int *severity)
>>> +{
>>> +	if (aer_regs->uncor_status & ~aer_regs->uncor_mask) {
>>> +		if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
>>> +			*severity = AER_FATAL;
>>> +		else
>>> +			*severity = AER_NONFATAL;
>>> +		return true;
>>> +	}
>>> +
>>> +	if (aer_regs->cor_status & ~aer_regs->cor_mask) {
>>> +		*severity = AER_CORRECTABLE;
>>> +		return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +static void cxl_handle_rch_dport_errors(struct cxl_dev_state *cxlds)
>>> +{
>>> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>>> +	struct aer_capability_regs aer_regs;
>>> +	struct cxl_dport *dport;
>>> +	int severity;
>>> +
>>> +	if (!cxlds->rcd)
>>> +		return;
>>
>> Small quibble, but I think this check belongs in the caller.
>>
> 
> Ok.
> 
>>> +
>>> +	if (!cxl_pci_find_port(pdev, &dport) || !dport->rch)
>>> +		return;
>>
>> The reference for the @port return from cxl_pci_find_port() is leaked
>> here.
>>


I will address this as well. Thanks for pointing this out.

Regards,
Terry