Received-SPF: pass (google.com: domain of linux-kernel+bounces-174329-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <b99685d9-9f3a-4c21-8d33-2eaa5de8be54@zhaoxin.com>
Date: Thu, 9 May 2024 17:06:52 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()
To: Bjorn Helgaas <helgaas@kernel.org>
CC: <rafael@kernel.org>, <lenb@kernel.org>, <james.morse@arm.com>,
	<tony.luck@intel.com>, <bp@alien8.de>, <bhelgaas@google.com>,
	<robert.moore@intel.com>, <linux-acpi@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
	<acpica-devel@lists.linux.dev>, <CobeChen@zhaoxin.com>,
	<TonyWWang@zhaoxin.com>, <ErosZhang@zhaoxin.com>, <LeoLiu@zhaoxin.com>
References: <20240508222459.GA1791619@bhelgaas>
From: LeoLiu-oc <leoliu-oc@zhaoxin.com>
In-Reply-To: <20240508222459.GA1791619@bhelgaas>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


在 2024/5/9 6:24, Bjorn Helgaas 写道:
> 
> 
> [这封邮件来自外部发件人 谨防风险]
> 
> On Mon, Dec 18, 2023 at 11:04:30AM +0800, LeoLiu-oc wrote:
>> From: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
>>
>> 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@zhaoxin.com>
>> ---
>>   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;
>> +     }
>> +}