Re: [PATCH v4 0/5] Remove AER HEST table parser

From: Kuppuswamy, Sathyanarayanan
Date: Fri May 29 2020 - 19:16:49 EST




On 5/29/20 4:09 PM, Bjorn Helgaas wrote:
On Tue, May 26, 2020 at 04:18:24PM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

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@xxxxxxxxxx>
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@xxxxxxxxxx>
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

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, &reg32);
+ pci_read_config_dword(dev, aer + PCI_ERR_CAP, &reg32);
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, &reg32);
+ pci_read_config_dword(dev, aer + PCI_ERR_CAP, &reg32);
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, &reg32);
- pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, reg32);
- pci_read_config_dword(pdev, aer_pos + PCI_ERR_COR_STATUS, &reg32);
- pci_write_config_dword(pdev, aer_pos + PCI_ERR_COR_STATUS, reg32);
- pci_read_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, &reg32);
- pci_write_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
+ pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, &reg32);
+ pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
+ 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, &reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
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, &reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
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, &reg32);
- pci_write_config_dword(pdev, pos + PCI_ERR_ROOT_STATUS, reg32);
+ pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
+ 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, &reg32);
+ pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
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, &reg32);
- pci_write_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, reg32);
+ pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
+ 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, &reg32);
+ pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
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