| 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: Wed, 26 Jun 2024 08:51:35 -0500
From: Terry Bowman <Terry.Bowman@....com>
To: "Li, Ming4" <ming4.li@...el.com>,
"Williams, Dan J" <dan.j.williams@...el.com>,
"Weiny, Ira" <ira.weiny@...el.com>, "dave@...olabs.net" <dave@...olabs.net>,
"Jiang, Dave" <dave.jiang@...el.com>,
"Schofield, Alison" <Alison.Schofield@...el.com>,
"Verma, Vishal L" <vishal.l.verma@...el.com>,
"jim.harris@...sung.com" <jim.harris@...sung.com>,
"ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
"ardb@...nel.org" <ardb@...nel.org>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Yazen.Ghannam@....com" <Yazen.Ghannam@....com>,
"Robert.Richter@....com" <Robert.Richter@....com>
Subject: Re: [RFC PATCH 7/9] cxl/pci: Add atomic notifier callback for CXL
PCIe port AER internal errors
On 6/26/24 01:22, Li, Ming4 wrote:
> On 6/18/2024 4:04 AM, Terry Bowman wrote:
>> CXL root ports, CXL downstream switch ports, and CXL upstream switch
>> ports are bound to the PCIe port bus driver, portdrv. portdrv provides
>> an atomic notifier chain for reporting PCIe port device AER
>> correctable internal errors (CIE) and AER uncorrectable internal
>> errors (UIE).
>>
>> CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]
>>
>> Add a cxl_pci atomic notification callback for handling the portdrv's
>> AER UIE/CIE notifications.
>>
>> Register the atomic notification callback in the cxl_pci module's
>> load. Unregister the callback in the cxl_pci driver's unload.
>>
>> Implement the callback to check if the device parameter is a valid
>> CXL PCIe port. If it is valid then make the notification callback call
>> __cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
>> type.
>>
>> [1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>> Upstream Switch Ports
>>
>> Signed-off-by: Terry Bowman <terry.bowman@....com>
>> ---
>> drivers/cxl/core/core.h | 4 ++
>> drivers/cxl/core/pci.c | 97 ++++++++++++++++++++++++++++++++++++++---
>> drivers/cxl/core/port.c | 6 +--
>> drivers/cxl/cxl.h | 5 +++
>> drivers/cxl/cxlpci.h | 2 +
>> drivers/cxl/pci.c | 19 +++++++-
>> 6 files changed, 123 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index bc5a95665aa0..69bef1db6ee0 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -94,4 +94,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>> enum access_coordinate_class access);
>> bool cxl_need_node_perf_attrs_update(int nid);
>>
>> +struct cxl_dport *find_dport(struct cxl_port *port, int id);
>> +struct cxl_port *find_cxl_port(struct device *dport_dev,
>> + struct cxl_dport **dport);
>> +
>> #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 59a317ab84bb..e630eccb733d 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -689,7 +689,6 @@ EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>> static void __cxl_handle_cor_ras(struct device *dev,
>> void __iomem *ras_base)
>> {
>> - struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> void __iomem *addr;
>> u32 status;
>>
>> @@ -698,10 +697,17 @@ static void __cxl_handle_cor_ras(struct device *dev,
>>
>> addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>> status = readl(addr);
>> - if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
>> - writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> +
>> + if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
>> + return;
>> +
>> + writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> + if (is_cxl_memdev(dev)) {
>> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +
>> trace_cxl_aer_correctable_error(cxlmd, status);
>> - }
>> + } else if (dev_is_pci(dev))
>> + trace_cxl_port_aer_correctable_error(dev, status);
>> }
>>
>> static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
>> @@ -733,7 +739,6 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>> static bool __cxl_handle_ras(struct device *dev,
>> void __iomem *ras_base)
>> {
>> - struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> u32 hl[CXL_HEADERLOG_SIZE_U32];
>> void __iomem *addr;
>> u32 status;
>> @@ -759,7 +764,13 @@ static bool __cxl_handle_ras(struct device *dev,
>> }
>>
>> header_log_copy(ras_base, hl);
>> - trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> + if (is_cxl_memdev(dev)) {
>> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +
>> + trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> + } else if (dev_is_pci(dev))
>> + trace_cxl_port_aer_uncorrectable_error(dev, status);
>> +
>> writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>>
>> return true;
>> @@ -882,6 +893,80 @@ static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
>> return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
>> }
>>
>> +static int match_uport(struct device *dev, void *data)
>> +{
>> + struct device *uport_dev = (struct device *)data;
>> + struct cxl_port *port;
>> +
>> + if (!is_cxl_port(dev))
>> + return 0;
>> +
>> + port = to_cxl_port(dev);
>> +
>> + return (port->uport_dev == uport_dev);
>> +}
>> +
>> +static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
>> +{
>> + struct cxl_dport *dport;
>> + struct device *port_dev;
>> + struct cxl_port *port;
>> +
>> + port = find_cxl_port(pdev->dev.parent, &dport);
>> + if (!port)
>> + return NULL;
>> + put_device(&port->dev);
>> +
>> + port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
>> + if (!port_dev)
>> + return NULL;
>
> seems like just a bus_find_device(&cxl_bus_type, NULL, &pdev->dev, match_uport) can replace these find_cxl_port() and device_find_child().
>
>
That would be a good improvement/optimization. I'll look into making that change.
>> + put_device(port_dev);
>> +
>> + port = to_cxl_port(port_dev);
>> +
>> + return port;
>> +}
>> +
>> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
>> +{
>> + void __iomem *ras_base = NULL;
>> +
>> + if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
>> + (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
>> + struct cxl_dport *dport;
>> +
>> + find_cxl_port(&pdev->dev, &dport);
>> + ras_base = dport ? dport->regs.ras : NULL;
>
> Need put_device(&port->dev) after find_cxl_port(), use scope-based resource management __free() here should be better.
>
>
Thanks.
Regards,
Terry
Powered by blists - more mailing lists