Re: [PATCH v3 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()

[Yazen Ghannam]

On Thu, Jul 18, 2024 at 02:24:05PM +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. This function
> applies to all hardware platforms that has a PCI Express AER structure
> in HEST.
> 
> Signed-off-by: LeoLiuoc <LeoLiu-oc@xxxxxxxxxxx>
> ---
>  drivers/pci/pci-acpi.c | 101 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h      |   9 ++++
>  drivers/pci/probe.c    |   1 +
>  3 files changed, 111 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 004575091596..b522e8b226b8 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,106 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>  	return -ENODEV;
>  }
>  
> +#ifdef CONFIG_ACPI_APEI
> +/*
> + * program_hest_aer_common() - configure AER common registers for Root Ports,
> + * Endpoints and PCIe to PCI/PCI-X bridges
> + */
> +static void program_hest_aer_common(struct acpi_hest_aer_common aer_common, struct pci_dev *dev,
> +				    int pos)
> +{
> +	u32 uncor_mask;
> +	u32 uncor_severity;
> +	u32 cor_mask;
> +	u32 adv_cap;
> +
> +	uncor_mask = aer_common.uncorrectable_mask;
> +	uncor_severity = aer_common.uncorrectable_severity;
> +	cor_mask = aer_common.correctable_mask;
> +	adv_cap = aer_common.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);
> +}
> +
> +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);
> +	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_common(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_common(hest_aer_endpoint->aer, dev, pos);
> +		break;
> +	case PCI_EXP_TYPE_PCI_BRIDGE:
> +		hest_aer_bridge = info.hest_aer_bridge;
> +		program_hest_aer_common(hest_aer_bridge->aer, dev, pos);
> +		program_hest_aer_bridge(hest_aer_bridge, dev, pos);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
> +{
> +	struct hest_parse_aer_info info = {
> +		.pci_dev = dev
> +	};
> +
> +	if (!pci_is_pcie(dev))
> +		return -ENODEV;
> +
> +	if (apei_hest_parse(hest_parse_pcie_aer, &info) == 1)
> +		program_hest_aer_params(info);
> +
> +	return 0;
> +}
> +#endif
> +
>  /**
>   * 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 fd44565c4756..03b5339f399f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -731,6 +731,15 @@ static inline void pci_save_aer_state(struct pci_dev *dev) { }
>  static inline void pci_restore_aer_state(struct pci_dev *dev) { }
>  #endif
>  
> +#ifdef CONFIG_ACPI_APEI
> +int pci_acpi_program_hest_aer_params(struct pci_dev *dev);
> +#else
> +static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_ACPI
>  int pci_acpi_program_hp_params(struct pci_dev *dev);
>  extern const struct attribute_group pci_dev_acpi_attr_group;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4c367f13acdc..f7d80b2a9d1d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2267,6 +2267,7 @@ static void pci_configure_device(struct pci_dev *dev)
>  	pci_configure_serr(dev);
>  
>  	pci_acpi_program_hp_params(dev);
> +	pci_acpi_program_hest_aer_params(dev);
>  }
>  
>  static void pci_release_capabilities(struct pci_dev *dev)
> -- 

I don't see where this accounts for _OSC negotiation. The OS must be
granted control of AER before accessing AER registers in general.

Thanks,
Yazen