From: LeoLiu-oc <leoliu-oc@zhaoxin.com>
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>
Subject: Re: [PATCH v2 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()
Date: Thu, 9 May 2024 17:06:52 +0800 [thread overview]
Message-ID: <b99685d9-9f3a-4c21-8d33-2eaa5de8be54@zhaoxin.com> (raw)
In-Reply-To: <20240508222459.GA1791619@bhelgaas>
在 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;
>> + }
>> +}
next prev parent reply other threads:[~2024-05-09 9:06 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-15 9:16 [PATCH 0/3] Parse the HEST PCIe AER and set to relevant registers LeoLiu-oc
2023-11-15 9:16 ` [PATCH 1/3] ACPI/APEI: Add hest_parse_pcie_aer() LeoLiu-oc
2023-12-06 16:35 ` Rafael J. Wysocki
2023-12-14 2:57 ` LeoLiu-oc
2023-11-15 9:16 ` [PATCH 2/3] PCI: Add AER bits #defines for PCIe to PCI/PCI-X Bridge LeoLiu-oc
2023-11-15 9:16 ` [PATCH 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params() LeoLiu-oc
2023-12-06 23:08 ` Bjorn Helgaas
2023-12-14 2:54 ` LeoLiu-oc
2023-12-18 3:04 ` [PATCH v2 0/3] Parse the HEST PCIe AER and set to relevant registers LeoLiu-oc
2023-12-18 3:04 ` [PATCH v2 1/3] ACPI/APEI: Add hest_parse_pcie_aer() LeoLiu-oc
2023-12-18 3:04 ` [PATCH v2 2/3] PCI: Add AER bits #defines for PCIe to PCI/PCI-X Bridge LeoLiu-oc
2024-05-08 22:10 ` Bjorn Helgaas
2024-05-09 8:42 ` LeoLiu-oc
2023-12-18 3:04 ` [PATCH v2 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params() LeoLiu-oc
2024-05-08 22:24 ` Bjorn Helgaas
2024-05-09 9:06 ` LeoLiu-oc [this message]
2024-05-08 22:04 ` [PATCH v2 0/3] Parse the HEST PCIe AER and set to relevant registers Bjorn Helgaas
2024-05-09 8:39 ` LeoLiu-oc
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b99685d9-9f3a-4c21-8d33-2eaa5de8be54@zhaoxin.com \
--to=leoliu-oc@zhaoxin.com \
--cc=CobeChen@zhaoxin.com \
--cc=ErosZhang@zhaoxin.com \
--cc=LeoLiu@zhaoxin.com \
--cc=TonyWWang@zhaoxin.com \
--cc=acpica-devel@lists.linux.dev \
--cc=bhelgaas@google.com \
--cc=bp@alien8.de \
--cc=helgaas@kernel.org \
--cc=james.morse@arm.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=robert.moore@intel.com \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).