Messages in this thread | | | Subject | Re: [PATCH v4 0/5] Remove AER HEST table parser | From | "Kuppuswamy, Sathyanarayanan" <> | Date | Fri, 29 May 2020 16:15:43 -0700 |
| |
On 5/29/20 4:09 PM, Bjorn Helgaas wrote: > On Tue, May 26, 2020 at 04:18:24PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> Commit c100beb9ccfb ("PCI/AER: Use only _OSC to determine AER ownership") >> removed HEST dependency in determining the AER ownership status. The >> following patch set cleansup rest of the HEST table related code from >> AER driver. >> >> This patchset also includes some minor AER driver fixes. >> >> Changes since v3: >> * Fixed compilation issues reported by kbuild test robot. >> >> Changes since v2: >> * Fixed commit sha id for patch "PCI/AER: Use only _OSC to determine AER ownership". >> >> Kuppuswamy Sathyanarayanan (5): >> PCI/AER: Remove redundant pci_is_pcie() checks. >> PCI/AER: Remove redundant dev->aer_cap checks. >> ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set. >> ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native >> is set. >> PCI/AER: Replace pcie_aer_get_firmware_first() with >> pcie_aer_is_native() > > I reordered these a bit and applied them as follows for v5.8: > > ("PCI/AER: Remove HEST/FIRMWARE_FIRST parsing for AER ownership") > ("PCI/AER: Remove redundant dev->aer_cap checks") > ("PCI/AER: Remove redundant pci_is_pcie() checks") > > I added the trivial patch below on top. > >> drivers/acpi/pci_root.c | 28 ++++---- >> drivers/pci/pcie/aer.c | 139 ++++--------------------------------- >> drivers/pci/pcie/dpc.c | 2 +- >> drivers/pci/pcie/portdrv.h | 15 +--- >> include/linux/pci.h | 2 + >> 5 files changed, 34 insertions(+), 152 deletions(-) > > commit 643a9f8854f9 ("PCI/AER: Use "aer" variable for capability offset") > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Fri May 29 17:56:09 2020 -0500 > > PCI/AER: Use "aer" variable for capability offset > > Previously we used "pos" or "aer_pos" for the offset of the AER Capability. > Use "aer" consistently and initialize it the same way everywhere. No > functional change intended. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 61e8cb23e98b..3acf56683915 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -136,19 +136,18 @@ static const char * const ecrc_policy_str[] = { > */ > static int enable_ecrc_checking(struct pci_dev *dev) > { > - int pos; > + int aer = dev->aer_cap; > u32 reg32; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return -ENODEV; > > - pci_read_config_dword(dev, pos + PCI_ERR_CAP, ®32); > + pci_read_config_dword(dev, aer + PCI_ERR_CAP, ®32); > if (reg32 & PCI_ERR_CAP_ECRC_GENC) > reg32 |= PCI_ERR_CAP_ECRC_GENE; > if (reg32 & PCI_ERR_CAP_ECRC_CHKC) > reg32 |= PCI_ERR_CAP_ECRC_CHKE; > - pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32); > + pci_write_config_dword(dev, aer + PCI_ERR_CAP, reg32); > > return 0; > } > @@ -161,16 +160,15 @@ static int enable_ecrc_checking(struct pci_dev *dev) > */ > static int disable_ecrc_checking(struct pci_dev *dev) > { > - int pos; > + int aer = dev->aer_cap; > u32 reg32; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return -ENODEV; > > - pci_read_config_dword(dev, pos + PCI_ERR_CAP, ®32); > + pci_read_config_dword(dev, aer + PCI_ERR_CAP, ®32); > reg32 &= ~(PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE); > - pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32); > + pci_write_config_dword(dev, aer + PCI_ERR_CAP, reg32); > > return 0; > } > @@ -253,18 +251,18 @@ void pci_aer_clear_device_status(struct pci_dev *dev) > > int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > { > - int pos = dev->aer_cap; > + int aer = dev->aer_cap; > u32 status, sev; > > if (!pcie_aer_is_native(dev)) > return -EIO; > > /* Clear status bits for ERR_NONFATAL errors only */ > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status); > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_SEVER, &sev); > status &= ~sev; > if (status) > - pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status); > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > > return 0; > } > @@ -272,18 +270,18 @@ EXPORT_SYMBOL_GPL(pci_aer_clear_nonfatal_status); > > void pci_aer_clear_fatal_status(struct pci_dev *dev) > { > - int pos = dev->aer_cap; > + int aer = dev->aer_cap; > u32 status, sev; > > if (!pcie_aer_is_native(dev)) > return; > > /* Clear status bits for ERR_FATAL errors only */ > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status); > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_SEVER, &sev); > status &= sev; > if (status) > - pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status); > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > } > > /** > @@ -297,25 +295,24 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev) > */ > int pci_aer_raw_clear_status(struct pci_dev *dev) > { > - int pos; > + int aer = dev->aer_cap; > u32 status; > int port_type; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return -EIO; > > port_type = pci_pcie_type(dev); > if (port_type == PCI_EXP_TYPE_ROOT_PORT) { > - pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status); > - pci_write_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, status); > + pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status); > + pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status); > } > > - pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS, &status); > - pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS, status); > + pci_read_config_dword(dev, aer + PCI_ERR_COR_STATUS, &status); > + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, status); > > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status); > - pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > > return 0; > } > @@ -330,12 +327,11 @@ int pci_aer_clear_status(struct pci_dev *dev) > > void pci_save_aer_state(struct pci_dev *dev) > { > + int aer = dev->aer_cap; > struct pci_cap_saved_state *save_state; > u32 *cap; > - int pos; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return; > > save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > @@ -343,22 +339,21 @@ void pci_save_aer_state(struct pci_dev *dev) > return; > > cap = &save_state->cap.data[0]; > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > - pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > - pci_read_config_dword(dev, pos + PCI_ERR_CAP, cap++); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, cap++); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_SEVER, cap++); > + pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, cap++); > + pci_read_config_dword(dev, aer + PCI_ERR_CAP, cap++); > if (pcie_cap_has_rtctl(dev)) > - pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); > + pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, cap++); > } > > void pci_restore_aer_state(struct pci_dev *dev) > { > + int aer = dev->aer_cap; > struct pci_cap_saved_state *save_state; > u32 *cap; > - int pos; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return; > > save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > @@ -366,12 +361,12 @@ void pci_restore_aer_state(struct pci_dev *dev) > return; > > cap = &save_state->cap.data[0]; > - pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > - pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > - pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > - pci_write_config_dword(dev, pos + PCI_ERR_CAP, *cap++); > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, *cap++); > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_SEVER, *cap++); > + pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, *cap++); > + pci_write_config_dword(dev, aer + PCI_ERR_CAP, *cap++); > if (pcie_cap_has_rtctl(dev)) > - pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > + pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, *cap++); > } > > void pci_aer_init(struct pci_dev *dev) > @@ -802,7 +797,7 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) > */ > static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info) > { > - int pos; > + int aer = dev->aer_cap; > u32 status, mask; > u16 reg16; > > @@ -837,17 +832,16 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info) > if (!(reg16 & PCI_EXP_AER_FLAGS)) > return false; > > - pos = dev->aer_cap; > - if (!pos) > + if (!aer) > return false; > > /* Check if error is recorded */ > if (e_info->severity == AER_CORRECTABLE) { > - pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS, &status); > - pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, &mask); > + pci_read_config_dword(dev, aer + PCI_ERR_COR_STATUS, &status); > + pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask); > } else { > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status); > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, &mask); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask); > } > if (status & ~mask) > return true; > @@ -918,16 +912,15 @@ static bool find_source_device(struct pci_dev *parent, > */ > static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > { > - int pos; > + int aer = dev->aer_cap; > > if (info->severity == AER_CORRECTABLE) { > /* > * Correctable error does not need software intervention. > * No need to go through error recovery process. > */ > - pos = dev->aer_cap; > - if (pos) > - pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS, > + if (aer) > + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, > info->status); > pci_aer_clear_device_status(dev); > } else if (info->severity == AER_NONFATAL) > @@ -1018,22 +1011,21 @@ EXPORT_SYMBOL_GPL(aer_recover_queue); > */ > int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info) > { > - int pos, temp; > + int aer = dev->aer_cap; > + int temp; > > /* Must reset in this function */ > info->status = 0; > info->tlp_header_valid = 0; > > - pos = dev->aer_cap; > - > /* The device might not support AER */ > - if (!pos) > + if (!aer) > return 0; > > if (info->severity == AER_CORRECTABLE) { > - pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS, > + pci_read_config_dword(dev, aer + PCI_ERR_COR_STATUS, > &info->status); > - pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, > + pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, > &info->mask); > if (!(info->status & ~info->mask)) > return 0; > @@ -1042,27 +1034,27 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info) > info->severity == AER_NONFATAL) { > > /* Link is still healthy for IO reads */ > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, > &info->status); > - pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, > &info->mask); > if (!(info->status & ~info->mask)) > return 0; > > /* Get First Error Pointer */ > - pci_read_config_dword(dev, pos + PCI_ERR_CAP, &temp); > + pci_read_config_dword(dev, aer + PCI_ERR_CAP, &temp); > info->first_error = PCI_ERR_CAP_FEP(temp); > > if (info->status & AER_LOG_TLP_MASKS) { > info->tlp_header_valid = 1; > pci_read_config_dword(dev, > - pos + PCI_ERR_HEADER_LOG, &info->tlp.dw0); > + aer + PCI_ERR_HEADER_LOG, &info->tlp.dw0); > pci_read_config_dword(dev, > - pos + PCI_ERR_HEADER_LOG + 4, &info->tlp.dw1); > + aer + PCI_ERR_HEADER_LOG + 4, &info->tlp.dw1); > pci_read_config_dword(dev, > - pos + PCI_ERR_HEADER_LOG + 8, &info->tlp.dw2); > + aer + PCI_ERR_HEADER_LOG + 8, &info->tlp.dw2); > pci_read_config_dword(dev, > - pos + PCI_ERR_HEADER_LOG + 12, &info->tlp.dw3); > + aer + PCI_ERR_HEADER_LOG + 12, &info->tlp.dw3); > } > } > > @@ -1168,15 +1160,15 @@ static irqreturn_t aer_irq(int irq, void *context) > struct pcie_device *pdev = (struct pcie_device *)context; > struct aer_rpc *rpc = get_service_data(pdev); > struct pci_dev *rp = rpc->rpd; > + int aer = rp->aer_cap; > struct aer_err_source e_src = {}; > - int pos = rp->aer_cap; > > - pci_read_config_dword(rp, pos + PCI_ERR_ROOT_STATUS, &e_src.status); > + pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status); > if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV))) > return IRQ_NONE; > > - pci_read_config_dword(rp, pos + PCI_ERR_ROOT_ERR_SRC, &e_src.id); > - pci_write_config_dword(rp, pos + PCI_ERR_ROOT_STATUS, e_src.status); > + pci_read_config_dword(rp, aer + PCI_ERR_ROOT_ERR_SRC, &e_src.id); > + pci_write_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, e_src.status); > > if (!kfifo_put(&rpc->aer_fifo, e_src)) > return IRQ_HANDLED; > @@ -1228,7 +1220,7 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev, > static void aer_enable_rootport(struct aer_rpc *rpc) > { > struct pci_dev *pdev = rpc->rpd; > - int aer_pos; > + int aer = pdev->aer_cap; > u16 reg16; > u32 reg32; > > @@ -1240,14 +1232,13 @@ static void aer_enable_rootport(struct aer_rpc *rpc) > pcie_capability_clear_word(pdev, PCI_EXP_RTCTL, > SYSTEM_ERROR_INTR_ON_MESG_MASK); > > - aer_pos = pdev->aer_cap; > /* Clear error status */ > - pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, ®32); > - pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, reg32); > - pci_read_config_dword(pdev, aer_pos + PCI_ERR_COR_STATUS, ®32); > - pci_write_config_dword(pdev, aer_pos + PCI_ERR_COR_STATUS, reg32); > - pci_read_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, ®32); > - pci_write_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, reg32); > + pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); > + pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); > + pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, ®32); > + pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32); > + pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, ®32); > + pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32); > > /* > * Enable error reporting for the root port device and downstream port > @@ -1256,9 +1247,9 @@ static void aer_enable_rootport(struct aer_rpc *rpc) > set_downstream_devices_error_reporting(pdev, true); > > /* Enable Root Port's interrupt in response to error messages */ > - pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_COMMAND, ®32); > + pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32); > reg32 |= ROOT_PORT_INTR_ON_MESG_MASK; > - pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_COMMAND, reg32); > + pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32); > } > > /** > @@ -1270,8 +1261,8 @@ static void aer_enable_rootport(struct aer_rpc *rpc) > static void aer_disable_rootport(struct aer_rpc *rpc) > { > struct pci_dev *pdev = rpc->rpd; > + int aer = pdev->aer_cap; > u32 reg32; > - int pos; > > /* > * Disable error reporting for the root port device and downstream port > @@ -1279,15 +1270,14 @@ static void aer_disable_rootport(struct aer_rpc *rpc) > */ > set_downstream_devices_error_reporting(pdev, false); > > - pos = pdev->aer_cap; > /* Disable Root's interrupt in response to error messages */ > - pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, ®32); > + pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32); > reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK; > - pci_write_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, reg32); > + pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32); > > /* Clear Root's error status reg */ > - pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_STATUS, ®32); > - pci_write_config_dword(pdev, pos + PCI_ERR_ROOT_STATUS, reg32); > + pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); > + pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); > } > > /** > @@ -1344,28 +1334,27 @@ static int aer_probe(struct pcie_device *dev) > */ > static pci_ers_result_t aer_root_reset(struct pci_dev *dev) > { > + int aer = dev->aer_cap; > u32 reg32; > - int pos; > int rc; > > - pos = dev->aer_cap; > > /* Disable Root's interrupt in response to error messages */ > - pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, ®32); > + pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, ®32); > reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK; > - pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32); > + pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32); > > rc = pci_bus_error_reset(dev); > pci_info(dev, "Root Port link has been reset\n"); > > /* Clear Root Error Status */ > - pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); > - pci_write_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, reg32); > + pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, ®32); > + pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32); > > /* Enable Root Port's interrupt in response to error messages */ > - pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, ®32); > + pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, ®32); > reg32 |= ROOT_PORT_INTR_ON_MESG_MASK; > - pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32); > + pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32); > > return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED; > } >
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |