| Message ID | 20230704120544.1322315-1-LeoLiu-oc@zhaoxin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Parse the PCIe AER and set to relevant registers | expand |
On Tue, Jul 04, 2023 at 08:05:44PM +0800, LeoLiu-oc wrote: > From: leoliu-oc <leoliu-oc@zhaoxin.com> > > 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 <leoliu-oc@zhaoxin.com> > --- > 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 >
在 2023/8/11 7:30, Bjorn Helgaas 写道: > On Tue, Jul 04, 2023 at 08:05:44PM +0800, LeoLiu-oc wrote: >> From: leoliu-oc <leoliu-oc@zhaoxin.com> >> >> 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 :)) > Ok, I will merge this patch with 5/5 in the next version. >> Signed-off-by: leoliu-oc <leoliu-oc@zhaoxin.com> >> --- >> 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. > OK. >> + */ >> +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. > OK. >> + 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. > OK. >> + >> + 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. > Thanks for your guidance, I will make changes to the code where it fits and does not conform to the specification. Best Regards. LeoLiu-oc >> + }; >> + >> + 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 >>
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