| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 16 Jun 2023 11:28:01 -0500
From: Terry Bowman <Terry.Bowman@....com>
To: Dan Williams <dan.j.williams@...el.com>,
alison.schofield@...el.com, vishal.l.verma@...el.com,
ira.weiny@...el.com, bwidawsk@...nel.org, dave.jiang@...el.com,
Jonathan.Cameron@...wei.com, linux-cxl@...r.kernel.org
Cc: rrichter@....com, linux-kernel@...r.kernel.org, bhelgaas@...gle.com
Subject: Re: [PATCH v5 24/26] cxl/pci: Add RCH downstream port error logging
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@....com>
>>> Signed-off-by: Robert Richter <rrichter@....com>
>>> Signed-off-by: Terry Bowman <terry.bowman@....com>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.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
Powered by blists - more mailing lists