PCI/AER: Use a common function to print AER error bits
PATCH RESEND 1/1 PCI/AER: Use a common function to print AER error bits
On errors reported from CPER, cper_print_bits() was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use __aer_print_error() for both native AER and CPER to provide a more consistent log format.
--- drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/pci/pcie/aer/aerdrv_errprint.c +9 -7
@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ; } layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status); +memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =0x1f ; +pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);
Signature · 1 line
2.14.3
Quoted 6 lines · Alexandru Gagniuc
On errors reported from CPER,
cper_print_bits()was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use__aer_print_error()for both native AER and CPER to provide a more consistent log format.Signed-off-by Alexandru Gagniuc <[email protected]>
Thanks!
Quoted 46 lines
--- drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/pci/pcie/aer/aerdrv_errprint.c +9
-7@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ; } layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status); +memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =0x1f ; +pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 35 lines · Alexandru Gagniuc
On errors reported from CPER,
cper_print_bits()was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use__aer_print_error()for both native AER and CPER to provide a more consistent log format.Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/pci/pcie/aer/aerdrv_errprint.c +2
-6@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ;
drivers/pci/pcie/aer/aerdrv_errprint.c EXPORT_SYMBOL_GPL(cper_severity_to_aer); · line 239
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_errprint.c +5
-0} layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status); +memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =0x1f ;
I like this patch a lot, but where does this "first_error = 0x1f" come from?
I assume this is supposed to be the "First Error Pointer" in the Advanced Error Capabilities and Control register (PCIe r4.0, sec 7.8.4.7). There is a "cap_control" field in struct aer_capability_regs; should we be using that here?
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_errprint.c +2
-1+pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);Signature · 1 line
2.14.3
Quoted 41 lines · Bjorn Helgaas
Quoted 40 lines · Alexandru Gagniuc
On errors reported from CPER,
cper_print_bits()was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use__aer_print_error()for both native AER and CPER to provide a more consistent log format.Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/pci/pcie/aer/aerdrv_errprint.c +2
-6@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ; } layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status);
Quoted 8 lines
drivers/pci/pcie/aer/aerdrv_errprint.c EXPORT_SYMBOL_GPL(cper_severity_to_aer); · line 239
Quoted 5 lines
drivers/pci/pcie/aer/aerdrv_errprint.c +5
-0+memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =0x1f ;I like this patch a lot, but where does this "
first_error=0x1f" come from?
aer_(un)correctable_error_string don't go to [0x1f], so this guarantees us we don't print "(First)".
Quoted 4 lines
I assume this is supposed to be the "First Error Pointer" in the Advanced Error Capabilities and Control register (PCIe r4.0, sec 7.8.4.7). There is a "
cap_control" field in structaer_capability_regs; should we be using that here?
There is a way to extract it from the PCI regs, and it's quite simple. IIRC, it should be all f's when the capability is not implemented. I wanted to avoid any further parsing of PCI regs in this patch.
I can see a way to use even more common printk code, but that requires validating the PCI regs we get from firmware. That means we need to make a guarantee about CPER that is beyond the scope of this patch.
Alex
Quoted 10 lines
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_errprint.c +2
-1+pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);Signature · 1 line
2.14.3
Quoted 2 lines · Alex G.
Quoted 1 line · Bjorn Helgaas
On Tue, Apr 17, 2018 at 12:09:43PM -0500, Alexandru Gagniuc wrote:
(snip)
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
+
memset(&info, 0, sizeof(info));
+ info.severity =aer_severity;
+ info.status = status;
+ info.mask = mask;
+ info.first_error =0x1f;I like this patch a lot, but where does this "
first_error=0x1f" come from?
Quoted 8 lines
aer_(un)correctable_error_string don't go to [
0x1f], so this guarantees us we don't print "(First)".Quoted 4 lines
I assume this is supposed to be the "First Error Pointer" in the Advanced Error Capabilities and Control register (PCIe r4.0, sec 7.8.4.7). There is a "
cap_control" field in structaer_capability_regs; should we be using that here?There is a way to extract it from the PCI regs, and it's quite simple. IIRC, it should be all f's when the capability is not implemented. I wanted to avoid any further parsing of PCI regs in this patch.
I could update the offending line to say:
+ info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
Though I still have the concerns with validating CPER data:
Quoted 4 lines
I can see a way to use even more common printk code, but that requires validating the PCI regs we get from firmware. That means we need to make a guarantee about CPER that is beyond the scope of this patch.
Alex
Quoted 21 lines · Alex G.
Quoted 2 lines · Alex G.
Quoted 1 line · Bjorn Helgaas
On Tue, Apr 17, 2018 at 12:09:43PM -0500, Alexandru Gagniuc wrote:
(snip)
Quoted 17 lines
Quoted 8 lines
Quoted 5 lines
+
memset(&info, 0, sizeof(info));
+ info.severity =aer_severity;
+ info.status = status;
+ info.mask = mask;
+ info.first_error =0x1f;I like this patch a lot, but where does this "
first_error=0x1f" come from?aer_(un)correctable_error_string don't go to [
0x1f], so this guarantees us we don't print "(First)".Quoted 4 lines
I assume this is supposed to be the "First Error Pointer" in the Advanced Error Capabilities and Control register (PCIe r4.0, sec 7.8.4.7). There is a "
cap_control" field in structaer_capability_regs; should we be using that here?
Quoted 6 lines
Quoted 3 lines
There is a way to extract it from the PCI regs, and it's quite simple. IIRC, it should be all f's when the capability is not implemented. I wanted to avoid any further parsing of PCI regs in this patch.
I could update the offending line to say:
+ info.first_error =PCI_ERR_CAP_FEP(aer->cap_control);
That's what I would have expected. So I'd say either do this, or add a comment about why it's not the right thing to do.
Quoted 5 lines
Though I still have the concerns with validating CPER data:
Quoted 3 lines
I can see a way to use even more common printk code, but that requires validating the PCI regs we get from firmware. That means we need to make a guarantee about CPER that is beyond the scope of this patch.
Sounds like this is material for another patch, but if/when you do that, I'd like to understand your concern about validating the registers we get from firmware. Are you worried about getting incorrect register contents, then printing the wrong info, making the wrong decision about how to recover, something else?
Bjorn
Quoted 1 line · Bjorn Helgaas
On Sat, Apr 28, 2018 at 12:07:48PM -0500, Alex G. wrote:
(snip)
Quoted 5 lines
Quoted 2 lines
I could update the offending line to say:
+ info.first_error =PCI_ERR_CAP_FEP(aer->cap_control);That's what I would have expected. So I'd say either do this, or add a comment about why it's not the right thing to do.
Okay.
Quoted 11 lines
Quoted 5 lines
Though I still have the concerns with validating CPER data:
Quoted 3 lines
I can see a way to use even more common printk code, but that requires validating the PCI regs we get from firmware. That means we need to make a guarantee about CPER that is beyond the scope of this patch.
Sounds like this is material for another patch, but if/when you do that, I'd like to understand your concern about validating the registers we get from firmware. Are you worried about getting incorrect register contents, then printing the wrong info, making the wrong decision about how to recover, something else?
I don't trust firmware, and I have daymares about firmware leaving these fields uninitialized. In jargon, I'd like to treat it as external untrusted serialized data.
Alex
Quoted 24 lines · Alex G.
Quoted 1 line · Bjorn Helgaas
On Sat, Apr 28, 2018 at 12:07:48PM -0500, Alex G. wrote:
(snip)
Quoted 5 lines
Quoted 2 lines
I could update the offending line to say:
+ info.first_error =PCI_ERR_CAP_FEP(aer->cap_control);That's what I would have expected. So I'd say either do this, or add a comment about why it's not the right thing to do.
Okay.
Quoted 11 lines
Quoted 5 lines
Though I still have the concerns with validating CPER data:
Quoted 3 lines
I can see a way to use even more common printk code, but that requires validating the PCI regs we get from firmware. That means we need to make a guarantee about CPER that is beyond the scope of this patch.
Sounds like this is material for another patch, but if/when you do that, I'd like to understand your concern about validating the registers we get from firmware. Are you worried about getting incorrect register contents, then printing the wrong info, making the wrong decision about how to recover, something else?
Quoted 3 lines
I don't trust firmware, and I have daymares about firmware leaving these fields uninitialized. In jargon, I'd like to treat it as external untrusted serialized data.
That makes good sense to me.
In this particular case, we only test first_error for equality:
__aer_print_error(...)
{
...pci_err(dev, " [%2d] %-22s%s\n", i, errmsg, info->first_error == i ? " (First)" : "");
so I don't think there's any danger. If we were using it to index an array or something, we should certainly validate it first.
PATCH v2 1/1 PCI/AER: Use a common function to print AER error bits
In reply to Alexandru GagniucOn errors reported from CPER, cper_print_bits() was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use __aer_print_error() for both native AER and CPER to provide a more consistent log format.
---
Changes since v1:
- Parse aer regs for first error pointer instead of disabling it
On the Dell machine where I tested this, the first error pointer is reported correctly by firmware.
drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/pci/pcie/aer/aerdrv_errprint.c +9 -7
@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ; } layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status); +memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =PCI_ERR_CAP_FEP (aer->cap_control); +pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);
Signature · 1 line
2.14.3
Bjorn Helgaas
AppliedQuoted 6 lines · Alexandru Gagniuc
On errors reported from CPER,
cper_print_bits()was used to log the AER bits. This resulted in hard-to-understand messages, without a prefix. Instead use__aer_print_error()for both native AER and CPER to provide a more consistent log format.Signed-off-by Alexandru Gagniuc <[email protected]>
Applied to pci/aer for v4.18, thanks!
Quoted 55 lines
---
Changes since v1:
- Parse aer regs for first error pointer instead of disabling it
On the Dell machine where I tested this, the first error pointer is reported correctly by firmware.drivers/pci/pcie/aer/aerdrv_errprint.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/pci/pcie/aer/aerdrv_errprint.c +9
-7@@ -216,28 +216,30 @@ EXPORT_SYMBOL_GPL(cper_severity_to_aer);void cper_print_aer (struct pci_dev *dev,int aer_severity,struct aer_capability_regs *aer) { -int layer, agent, status_strs_size, tlp_header_valid =0 ; +int layer, agent, tlp_header_valid =0 ;u32 status, mask; -const char **status_strs; +struct aer_err_info info;if (aer_severity ==AER_CORRECTABLE ) { status = aer->cor_status; mask = aer->cor_mask; - status_strs = aer_correctable_error_string; - status_strs_size =ARRAY_SIZE (aer_correctable_error_string); }else { status = aer->uncor_status; mask = aer->uncor_mask; - status_strs = aer_uncorrectable_error_string; - status_strs_size =ARRAY_SIZE (aer_uncorrectable_error_string); tlp_header_valid = status &AER_LOG_TLP_MASKS ; } layer =AER_GET_LAYER_ERROR (aer_severity, status); agent =AER_GET_AGENT (aer_severity, status); +memset (&info,0 ,sizeof (info)); + info.severity = aer_severity; + info.status = status; + info.mask = mask; + info.first_error =PCI_ERR_CAP_FEP (aer->cap_control); +pci_err (dev,"aer_status: 0x%08x, aer_mask: 0x%08x\n" , status, mask); -cper_print_bits ("" , status, status_strs, status_strs_size); +__aer_print_error (dev, &info);pci_err (dev,"aer_layer=%s, aer_agent=%s\n" , aer_error_layer[layer], aer_agent_string[agent]);Signature · 1 line
2.14.3