| 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
| ||
|
Message-ID: <8bf0d1c9-632e-458b-9b78-0faeea0472f8@amd.com>
Date: Wed, 10 Jul 2024 16:48:09 -0500
From: Terry Bowman <Terry.Bowman@....com>
To: nifan.cxl@...il.com
Cc: Dan Williams <dan.j.williams@...el.com>, ira.weiny@...el.com,
dave@...olabs.net, dave.jiang@...el.com, alison.schofield@...el.com,
ming4.li@...el.com, vishal.l.verma@...el.com, jim.harris@...sung.com,
ilpo.jarvinen@...ux.intel.com, ardb@...nel.org,
sathyanarayanan.kuppuswamy@...ux.intel.com, linux-cxl@...r.kernel.org,
linux-kernel@...r.kernel.org, Yazen.Ghannam@....com, Robert.Richter@....com,
Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 1/9] PCI/AER: Update AER driver to call root port and
downstream port UCE handlers
Hi Fan,
On 7/10/24 15:48, nifan.cxl@...il.com wrote:
> On Mon, Jun 24, 2024 at 12:56:29PM -0500, Terry Bowman wrote:
>> Hi Dan,
>>
>> I added a response below.
>>
>> On 6/21/24 14:17, Dan Williams wrote:
>>> Terry Bowman wrote:
>>>> The AER service driver does not currently call a handler for AER
>>>> uncorrectable errors (UCE) detected in root ports or downstream
>>>> ports. This is not needed in most cases because common PCIe port
>>>> functionality is handled by portdrv service drivers.
>>>>
>>>> CXL root ports include CXL specific RAS registers that need logging
>>>> before starting do_recovery() in the UCE case.
>>>>
>>>> Update the AER service driver to call the UCE handler for root ports
>>>> and downstream ports. These PCIe port devices are bound to the portdrv
>>>> driver that includes a CE and UCE handler to be called.
>>>>
>>>> Signed-off-by: Terry Bowman <terry.bowman@....com>
>>>> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
>>>> Cc: linux-pci@...r.kernel.org
>>>> ---
>>>> drivers/pci/pcie/err.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index 705893b5f7b0..a4db474b2be5 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -203,6 +203,26 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>>>> struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>>>
>>>> + /*
>>>> + * PCIe ports may include functionality beyond the standard
>>>> + * extended port capabilities. This may present a need to log and
>>>> + * handle errors not addressed in this driver. Examples are CXL
>>>> + * root ports and CXL downstream switch ports using AER UIE to
>>>> + * indicate CXL UCE RAS protocol errors.
>>>> + */
>>>> + if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>> + type == PCI_EXP_TYPE_DOWNSTREAM) {
>>>> + struct pci_driver *pdrv = dev->driver;
>>>> +
>>>> + if (pdrv && pdrv->err_handler &&
>>>> + pdrv->err_handler->error_detected) {
>>>> + const struct pci_error_handlers *err_handler;
>>>> +
>>>> + err_handler = pdrv->err_handler;
>>>> + status = err_handler->error_detected(dev, state);
>>>> + }
>>>> + }
>>>> +
>>>
>>> Would not a more appropriate place for this be pci_walk_bridge() where
>>> the ->subordinate == NULL and these type-check cases are unified?
>>
>> It does. I can take a look at moving that.
>
> Has that already been handled in pci_walk_bridge?
>
> The function pci_walk_bridge() will call report_error_detected, where
> the err handler will be called.
> https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/pci/pcie/err.c#L80
>
> Fan
>
You would think so but the UCE handler was not called in my testing for the PCIe
ports (RP,USP,DSP). The pci_walk_bridge() function has 2 cases:
- If there is a subordinate/secondary bus then the callback is called for
those downstream devices but not the port itself.
- If there is no subordinate/secondary bus then the callback is invoked for the
port itself.
The function header comment may explain it better:
/**
* pci_walk_bridge - walk bridges potentially AER affected
* @bridge: bridge which may be a Port, an RCEC, or an RCiEP
* @cb: callback to be called for each device found
* @userdata: arbitrary pointer to be passed to callback
*
* If the device provided is a bridge, walk the subordinate bus, including
* any bridged devices on buses under this bus. Call the provided callback
* on each device found.
*
* If the device provided has no subordinate bus, e.g., an RCEC or RCiEP,
* call the callback on the device itself.
*/
Regards,
Terry
>>
>> Regards,
>> Terry
Powered by blists - more mailing lists