From: leoliu-oc <[email protected]>
The extracted register values from HEST PCI Express AER structures are
written to AER Capabilities.
Signed-off-by: leoliu-oc <[email protected]>
---
drivers/pci/pci-acpi.c | 92 ++++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 5 +++
2 files changed, 97 insertions(+)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a05350a4e49cb..cff54410e2427 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,97 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
return -ENODEV;
}
+/*
+ * program_aer_structure_to_aer_registers - Write the AER structure to
+ * the corresponding dev's AER registers.
+ *
+ * @info - the AER structure information
+ *
+ */
+static void program_aer_structure_to_aer_registers(struct acpi_hest_parse_aer_info info)
+{
+ u32 uncorrectable_mask;
+ u32 uncorrectable_severity;
+ u32 correctable_mask;
+ u32 advanced_capabilities;
+ u32 root_error_command;
+ u32 uncorrectable_mask2;
+ u32 uncorrectable_severity2;
+ u32 advanced_capabilities2;
+ int port_type;
+ int pos;
+ struct pci_dev *dev;
+
+ dev = info.pci_dev;
+ port_type = pci_pcie_type(dev);
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+ if (!pos)
+ return;
+
+ if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
+ uncorrectable_mask = info.acpi_hest_aer_root_port->uncorrectable_mask;
+ uncorrectable_severity = info.acpi_hest_aer_root_port->uncorrectable_severity;
+ correctable_mask = info.acpi_hest_aer_root_port->correctable_mask;
+ advanced_capabilities = info.acpi_hest_aer_root_port->advanced_capabilities;
+ root_error_command = info.acpi_hest_aer_root_port->root_error_command;
+
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncorrectable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, uncorrectable_severity);
+ pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, correctable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_CAP, advanced_capabilities);
+ pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, root_error_command);
+ } else if (port_type == PCI_EXP_TYPE_ENDPOINT) {
+ uncorrectable_mask = info.acpi_hest_aer_endpoint->uncorrectable_mask;
+ uncorrectable_severity = info.acpi_hest_aer_endpoint->uncorrectable_severity;
+ correctable_mask = info.acpi_hest_aer_endpoint->correctable_mask;
+ advanced_capabilities = info.acpi_hest_aer_endpoint->advanced_capabilities;
+
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncorrectable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, uncorrectable_severity);
+ pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, correctable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_CAP, advanced_capabilities);
+ } else if ((pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) ||
+ (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE)) {
+ uncorrectable_mask = info.acpi_hest_aer_for_bridge->uncorrectable_mask;
+ uncorrectable_severity = info.acpi_hest_aer_for_bridge->uncorrectable_severity;
+ correctable_mask = info.acpi_hest_aer_for_bridge->correctable_mask;
+ advanced_capabilities = info.acpi_hest_aer_for_bridge->advanced_capabilities;
+ uncorrectable_mask2 = info.acpi_hest_aer_for_bridge->uncorrectable_mask2;
+ uncorrectable_severity2 = info.acpi_hest_aer_for_bridge->uncorrectable_severity2;
+ advanced_capabilities2 = info.acpi_hest_aer_for_bridge->advanced_capabilities2;
+
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncorrectable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, uncorrectable_severity);
+ pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, correctable_mask);
+ pci_write_config_dword(dev, pos + PCI_ERR_CAP, advanced_capabilities);
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK2, uncorrectable_mask2);
+ pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER2, uncorrectable_severity2);
+ pci_write_config_dword(dev, pos + PCI_ERR_CAP2, advanced_capabilities2);
+ }
+}
+
+int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
+{
+ struct acpi_hest_parse_aer_info info = {
+ .pci_dev = dev,
+ .hest_matched_with_dev = 0,
+ .acpi_hest_aer_endpoint = NULL,
+ .acpi_hest_aer_root_port = NULL,
+ .acpi_hest_aer_for_bridge = NULL,
+ };
+
+ if (!pci_is_pcie(dev))
+ return -ENODEV;
+
+ apei_hest_parse(apei_hest_parse_aer, &info);
+ if (info.hest_matched_with_dev == 1)
+ program_aer_structure_to_aer_registers(info);
+ else
+ return -ENODEV;
+ return 0;
+}
+
/**
* pciehp_is_native - Check whether a hotplug port is handled by the OS
* @bridge: Hotplug port to check
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a4c3974340576..37aa4a33eeed2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -713,6 +713,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
bool acpi_pci_need_resume(struct pci_dev *dev);
pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
+int pci_acpi_program_hest_aer_params(struct pci_dev *dev);
#else
static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
{
@@ -752,6 +753,10 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
{
return PCI_POWER_ERROR;
}
+static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
+{
+ return -ENODEV;
+}
#endif
#ifdef CONFIG_PCIEASPM
--
2.34.1
On Tue, Jul 04, 2023 at 08:05:44PM +0800, LeoLiu-oc wrote:
> From: leoliu-oc <[email protected]>
>
> The extracted register values from HEST PCI Express AER structures are
> written to AER Capabilities.
In the subject, the prevailing style for this file is
(see "git log --oneline drivers/pci/pci-acpi.c"):
PCI/ACPI: ...
And I'd like the subject to tell users why they might want this patch.
It's obvious from the patch that this adds a function. What's *not*
obvious is *why* we want this new function. So the commit log should
tell us what the benefit is, and the subject line should be one-line
summary of that benefit.
This patch adds a function but no caller. The next patch is one-liner
that adds the caller. I think these two should be squashed so it's
easier to review (and easier to explain the benefit of *this* patch :))
> Signed-off-by: leoliu-oc <[email protected]>
> ---
> drivers/pci/pci-acpi.c | 92 ++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 5 +++
> 2 files changed, 97 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a05350a4e49cb..cff54410e2427 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,97 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
> return -ENODEV;
> }
>
> +/*
> + * program_aer_structure_to_aer_registers - Write the AER structure to
> + * the corresponding dev's AER registers.
> + *
> + * @info - the AER structure information
> + *
Remove the spurious blank comment line.
> + */
> +static void program_aer_structure_to_aer_registers(struct acpi_hest_parse_aer_info info)
> +{
> + u32 uncorrectable_mask;
> + u32 uncorrectable_severity;
> + u32 correctable_mask;
> + u32 advanced_capabilities;
> + u32 root_error_command;
> + u32 uncorrectable_mask2;
> + u32 uncorrectable_severity2;
> + u32 advanced_capabilities2;
> + int port_type;
> + int pos;
> + struct pci_dev *dev;
Order these declarations in order of use.
> + dev = info.pci_dev;
> + port_type = pci_pcie_type(dev);
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> + if (!pos)
> + return;
> +
> + if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> + uncorrectable_mask = info.acpi_hest_aer_root_port->uncorrectable_mask;
> + uncorrectable_severity = info.acpi_hest_aer_root_port->uncorrectable_severity;
> + correctable_mask = info.acpi_hest_aer_root_port->correctable_mask;
> + advanced_capabilities = info.acpi_hest_aer_root_port->advanced_capabilities;
> + root_error_command = info.acpi_hest_aer_root_port->root_error_command;
Except for this new code, this file fits in 80 columns, so I'd like
the new code to match.
> +
> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncorrectable_mask);
I'm not sure we need to copy everything into local variables. Maybe
this could be split into three helper functions, which would save a
level of indent and a level of struct traversal (e.g., "rp->" instead
of "info.acpi_hest_aer_root_port->".
pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, rp->uncorrectable_mask);
or
pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK,
rp->uncorrectable_mask);
If you have to define a new struct acpi_hest_aer_root_port, you could
make the member names shorter. But hopefully you *don't* have to do
that, so maybe we're stuck with the long existing member names in
acpi_hest_aer_common.
> +int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
> +{
> + struct acpi_hest_parse_aer_info info = {
> + .pci_dev = dev,
> + .hest_matched_with_dev = 0,
> + .acpi_hest_aer_endpoint = NULL,
> + .acpi_hest_aer_root_port = NULL,
> + .acpi_hest_aer_for_bridge = NULL,
Drop the tab from the .pci_dev initialization since the other members
aren't lined up anyway. I think you can drop the other
initializations completely since they will be initialized to 0 or NULL
pointers by default.
> + };
> +
> + if (!pci_is_pcie(dev))
> + return -ENODEV;
> +
> + apei_hest_parse(apei_hest_parse_aer, &info);
> + if (info.hest_matched_with_dev == 1)
> + program_aer_structure_to_aer_registers(info);
> + else
> + return -ENODEV;
> + return 0;
> +}
> +
> /**
> * pciehp_is_native - Check whether a hotplug port is handled by the OS
> * @bridge: Hotplug port to check
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index a4c3974340576..37aa4a33eeed2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -713,6 +713,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
> int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
> bool acpi_pci_need_resume(struct pci_dev *dev);
> pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
> +int pci_acpi_program_hest_aer_params(struct pci_dev *dev);
> #else
> static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
> {
> @@ -752,6 +753,10 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> {
> return PCI_POWER_ERROR;
> }
> +static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
> +{
> + return -ENODEV;
> +}
> #endif
>
> #ifdef CONFIG_PCIEASPM
> --
> 2.34.1
>