在 2024/5/9 6:24, Bjorn Helgaas 写道:
[这封邮件来自外部发件人 谨防风险] On Mon, Dec 18, 2023 at 11:04:30AM +0800, LeoLiu-oc wrote:From: LeoLiuoc <LeoLiu-oc@xxxxxxxxxxx> Call the func pci_acpi_program_hest_aer_params() for every PCIe device, the purpose of this function is to extract register value from HEST PCIe AER structures and program them into AER Capabilities. Signed-off-by: LeoLiuoc <LeoLiu-oc@xxxxxxxxxxx> --- drivers/pci/pci-acpi.c | 98 ++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 9 ++++ drivers/pci/probe.c | 1 + 3 files changed, 108 insertions(+) diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 004575091596..3a183d5e20cb 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -18,6 +18,7 @@ #include <linux/pm_runtime.h> #include <linux/pm_qos.h> #include <linux/rwsem.h> +#include <acpi/apei.h> #include "pci.h" /* @@ -783,6 +784,103 @@ int pci_acpi_program_hp_params(struct pci_dev *dev) return -ENODEV; } +#ifdef CONFIG_ACPI_APEI +static void program_hest_aer_endpoint(struct acpi_hest_aer_common aer_endpoint, + struct pci_dev *dev, int pos) +{ + u32 uncor_mask; + u32 uncor_severity; + u32 cor_mask; + u32 adv_cap; + + uncor_mask = aer_endpoint.uncorrectable_mask; + uncor_severity = aer_endpoint.uncorrectable_severity; + cor_mask = aer_endpoint.correctable_mask; + adv_cap = aer_endpoint.advanced_capabilities; + + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncor_mask); + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, uncor_severity); + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, cor_mask); + pci_write_config_dword(dev, pos + PCI_ERR_CAP, adv_cap);This is named for "endpoint", but it is used for Root Ports, Endpoints, and PCIe to PCI/PCI-X bridges. It relies on these four fields being in the same place for all those HEST structures.
Change the function name " program_hest_aer_endpoint " to "program_hest_aer_common" and the parameters of the function "aer_endpoint" to "aer_common". Do you think this is appropriate?
That makes good sense, but maybe should have a one-line hint about this and maybe even a compiletime_assert().
I intend to add the following comment to the function in next version："/* Configure AER common registers for Root Ports, Endpoints, and PCIe to PCI/PCI-X bridges */", Is this description appropriate?
+} + +static void program_hest_aer_root(struct acpi_hest_aer_root *aer_root, struct pci_dev *dev, int pos) +{ + u32 root_err_cmd; + + root_err_cmd = aer_root->root_error_command; + + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, root_err_cmd); +} + +static void program_hest_aer_bridge(struct acpi_hest_aer_bridge *hest_aer_bridge, + struct pci_dev *dev, int pos) +{ + u32 uncor_mask2; + u32 uncor_severity2; + u32 adv_cap2; + + uncor_mask2 = hest_aer_bridge->uncorrectable_mask2; + uncor_severity2 = hest_aer_bridge->uncorrectable_severity2; + adv_cap2 = hest_aer_bridge->advanced_capabilities2; + + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK2, uncor_mask2); + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER2, uncor_severity2); + pci_write_config_dword(dev, pos + PCI_ERR_CAP2, adv_cap2); +} + +static void program_hest_aer_params(struct hest_parse_aer_info info) +{ + struct pci_dev *dev; + int port_type; + int pos; + struct acpi_hest_aer_root *hest_aer_root; + struct acpi_hest_aer *hest_aer_endpoint; + struct acpi_hest_aer_bridge *hest_aer_bridge; + + dev = info.pci_dev; + port_type = pci_pcie_type(dev);I'd put these two initializations up at the declarations, e.g., struct pci_dev *dev = info.pci_dev; int port_type = pci_pcie_type(dev);
Okay, this will be modified in the next version.
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); + if (!pos) + return; + + switch (port_type) { + case PCI_EXP_TYPE_ROOT_PORT: + hest_aer_root = info.hest_aer_root_port; + program_hest_aer_endpoint(hest_aer_root->aer, dev, pos); + program_hest_aer_root(hest_aer_root, dev, pos); + break; + case PCI_EXP_TYPE_ENDPOINT: + hest_aer_endpoint = info.hest_aer_endpoint; + program_hest_aer_endpoint(hest_aer_endpoint->aer, dev, pos); + break; + case PCI_EXP_TYPE_PCI_BRIDGE: + case PCI_EXP_TYPE_PCIE_BRIDGE:PCI_EXP_TYPE_PCIE_BRIDGE is a PCI/PCI-X to PCIe Bridge, also known as a "reverse bridge". This has a conventional PCI or PCI-X primary interface and a PCIe secondary interface. It's not clear to me that these bridges can support AER. For one thing, the AER Capability must be in extended config space, which would only be available for PCI-X Mode 2 reverse bridges. The acpi_hest_aer_bridge certainly makes sense for PCI_EXP_TYPE_PCI_BRIDGE (a PCIe to PCI/PCI-X bridge), but the ACPI spec (r6.5, sec 18.3.2.6) is not very clear about whether it also applies to PCI_EXP_TYPE_PCIE_BRIDGE (a reverse bridge). Do you actually need this PCI_EXP_TYPE_PCIE_BRIDGE case?
Yes, you are right. I will fix this in the next version. Yours sincerely Leoliu-oc
+ hest_aer_bridge = info.hest_aer_bridge; + program_hest_aer_endpoint(hest_aer_bridge->aer, dev, pos); + program_hest_aer_bridge(hest_aer_bridge, dev, pos); + break; + default: + return; + } +}
