The following warning was seen on 3.9 when a corrected PCIe error was being
handled by the AER subsystem.
WARNING: at .../drivers/pci/search.c:214 pci_get_dev_by_id+0x8a/0x90()
This occurred because a call to pci_get_domain_bus_and_slot() was added to
cper_print_pcie() to setup for the call to cper_print_aer(). The warning
showed up because cper_print_pcie() is called in an interrupt context and
pci_get* functions are not supposed to be called in that context.
The solution is to move the cper_print_aer() call out of the interrupt
context and into aer_recover_work_func() to avoid any warnings when calling
pci_get* functions.
v2 - Re-worded header text. Removed prefix arg from cper_print_aer().
Added TODO message in cper_print_aer().
v3 - Changed type of u8* to struct aer_capability_regs* in the code
to avoid too much casting based on comment from Bjorn Helgaas.
Signed-off-by: Lance Ortiz <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
---
drivers/acpi/apei/cper.c | 18 ------------------
drivers/acpi/apei/ghes.c | 4 +++-
drivers/pci/pcie/aer/aerdrv_core.c | 5 ++++-
drivers/pci/pcie/aer/aerdrv_errprint.c | 10 ++++++++--
include/linux/aer.h | 5 +++--
5 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 1e5d8a4..8713229 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -250,10 +250,6 @@ static const char *cper_pcie_port_type_strs[] = {
static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
const struct acpi_hest_generic_data *gdata)
{
-#ifdef CONFIG_ACPI_APEI_PCIEAER
- struct pci_dev *dev;
-#endif
-
if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
printk("%s""port_type: %d, %s\n", pfx, pcie->port_type,
pcie->port_type < ARRAY_SIZE(cper_pcie_port_type_strs) ?
@@ -285,20 +281,6 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
printk(
"%s""bridge: secondary_status: 0x%04x, control: 0x%04x\n",
pfx, pcie->bridge.secondary_status, pcie->bridge.control);
-#ifdef CONFIG_ACPI_APEI_PCIEAER
- dev = pci_get_domain_bus_and_slot(pcie->device_id.segment,
- pcie->device_id.bus, pcie->device_id.function);
- if (!dev) {
- pr_err("PCI AER Cannot get PCI device %04x:%02x:%02x.%d\n",
- pcie->device_id.segment, pcie->device_id.bus,
- pcie->device_id.slot, pcie->device_id.function);
- return;
- }
- if (pcie->validation_bits & CPER_PCIE_VALID_AER_INFO)
- cper_print_aer(pfx, dev, gdata->error_severity,
- (struct aer_capability_regs *) pcie->aer_info);
- pci_dev_put(dev);
-#endif
}
static const char *apei_estatus_section_flag_strs[] = {
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d668a8a..403baf4 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -454,7 +454,9 @@ static void ghes_do_proc(struct ghes *ghes,
aer_severity = cper_severity_to_aer(sev);
aer_recover_queue(pcie_err->device_id.segment,
pcie_err->device_id.bus,
- devfn, aer_severity);
+ devfn, aer_severity,
+ (struct aer_capability_regs *)
+ pcie_err->aer_info);
}
}
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 564d97f..120549a 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -582,6 +582,7 @@ struct aer_recover_entry
u8 devfn;
u16 domain;
int severity;
+ struct aer_capability_regs *regs;
};
static DEFINE_KFIFO(aer_recover_ring, struct aer_recover_entry,
@@ -595,7 +596,7 @@ static DEFINE_SPINLOCK(aer_recover_ring_lock);
static DECLARE_WORK(aer_recover_work, aer_recover_work_func);
void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
- int severity)
+ int severity, struct aer_capability_regs *aer_regs)
{
unsigned long flags;
struct aer_recover_entry entry = {
@@ -603,6 +604,7 @@ void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
.devfn = devfn,
.domain = domain,
.severity = severity,
+ .regs = aer_regs,
};
spin_lock_irqsave(&aer_recover_ring_lock, flags);
@@ -629,6 +631,7 @@ static void aer_recover_work_func(struct work_struct *work)
PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));
continue;
}
+ cper_print_aer(pdev, entry.severity, entry.regs);
do_recovery(pdev, entry.severity);
pci_dev_put(pdev);
}
diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index 5ab1425..f277dc3 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -220,13 +220,19 @@ int cper_severity_to_aer(int cper_severity)
}
EXPORT_SYMBOL_GPL(cper_severity_to_aer);
-void cper_print_aer(const char *prefix, struct pci_dev *dev, int cper_severity,
+void cper_print_aer(struct pci_dev *dev, int cper_severity,
struct aer_capability_regs *aer)
{
int aer_severity, layer, agent, status_strs_size, tlp_header_valid = 0;
u32 status, mask;
const char **status_strs;
+ /*
+ * TODO: This function needs to be re-written so that it's output
+ * matches the output of aer_print_error(). Right now, the output
+ * is formatted very differently.
+ */
+
aer_severity = cper_severity_to_aer(cper_severity);
if (aer_severity == AER_CORRECTABLE) {
status = aer->cor_status;
@@ -244,7 +250,7 @@ void cper_print_aer(const char *prefix, struct pci_dev *dev, int cper_severity,
agent = AER_GET_AGENT(aer_severity, status);
dev_err(&dev->dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n",
status, mask);
- cper_print_bits(prefix, status, status_strs, status_strs_size);
+ cper_print_bits("", status, status_strs, status_strs_size);
dev_err(&dev->dev, "aer_layer=%s, aer_agent=%s\n",
aer_error_layer[layer], aer_agent_string[agent]);
if (aer_severity != AER_CORRECTABLE)
diff --git a/include/linux/aer.h b/include/linux/aer.h
index ec10e1b..737f90a 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -49,10 +49,11 @@ static inline int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
}
#endif
-extern void cper_print_aer(const char *prefix, struct pci_dev *dev,
+extern void cper_print_aer(struct pci_dev *dev,
int cper_severity, struct aer_capability_regs *aer);
extern int cper_severity_to_aer(int cper_severity);
extern void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
- int severity);
+ int severity,
+ struct aer_capability_regs *aer_regs);
#endif //_AER_H_
On Wednesday, May 29, 2013 01:03:26 PM Lance Ortiz wrote:
> The following warning was seen on 3.9 when a corrected PCIe error was being
> handled by the AER subsystem.
>
> WARNING: at .../drivers/pci/search.c:214 pci_get_dev_by_id+0x8a/0x90()
>
> This occurred because a call to pci_get_domain_bus_and_slot() was added to
> cper_print_pcie() to setup for the call to cper_print_aer(). The warning
> showed up because cper_print_pcie() is called in an interrupt context and
> pci_get* functions are not supposed to be called in that context.
>
> The solution is to move the cper_print_aer() call out of the interrupt
> context and into aer_recover_work_func() to avoid any warnings when calling
> pci_get* functions.
>
> v2 - Re-worded header text. Removed prefix arg from cper_print_aer().
> Added TODO message in cper_print_aer().
> v3 - Changed type of u8* to struct aer_capability_regs* in the code
> to avoid too much casting based on comment from Bjorn Helgaas.
>
> Signed-off-by: Lance Ortiz <[email protected]>
> Acked-by: Borislav Petkov <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> ---
>
> drivers/acpi/apei/cper.c | 18 ------------------
> drivers/acpi/apei/ghes.c | 4 +++-
> drivers/pci/pcie/aer/aerdrv_core.c | 5 ++++-
> drivers/pci/pcie/aer/aerdrv_errprint.c | 10 ++++++++--
> include/linux/aer.h | 5 +++--
> 5 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index 1e5d8a4..8713229 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -250,10 +250,6 @@ static const char *cper_pcie_port_type_strs[] = {
> static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
> const struct acpi_hest_generic_data *gdata)
> {
> -#ifdef CONFIG_ACPI_APEI_PCIEAER
> - struct pci_dev *dev;
> -#endif
> -
> if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
> printk("%s""port_type: %d, %s\n", pfx, pcie->port_type,
> pcie->port_type < ARRAY_SIZE(cper_pcie_port_type_strs) ?
> @@ -285,20 +281,6 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
> printk(
> "%s""bridge: secondary_status: 0x%04x, control: 0x%04x\n",
> pfx, pcie->bridge.secondary_status, pcie->bridge.control);
> -#ifdef CONFIG_ACPI_APEI_PCIEAER
> - dev = pci_get_domain_bus_and_slot(pcie->device_id.segment,
> - pcie->device_id.bus, pcie->device_id.function);
> - if (!dev) {
> - pr_err("PCI AER Cannot get PCI device %04x:%02x:%02x.%d\n",
> - pcie->device_id.segment, pcie->device_id.bus,
> - pcie->device_id.slot, pcie->device_id.function);
> - return;
> - }
> - if (pcie->validation_bits & CPER_PCIE_VALID_AER_INFO)
> - cper_print_aer(pfx, dev, gdata->error_severity,
> - (struct aer_capability_regs *) pcie->aer_info);
> - pci_dev_put(dev);
> -#endif
> }
>
> static const char *apei_estatus_section_flag_strs[] = {
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d668a8a..403baf4 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -454,7 +454,9 @@ static void ghes_do_proc(struct ghes *ghes,
> aer_severity = cper_severity_to_aer(sev);
> aer_recover_queue(pcie_err->device_id.segment,
> pcie_err->device_id.bus,
> - devfn, aer_severity);
> + devfn, aer_severity,
> + (struct aer_capability_regs *)
> + pcie_err->aer_info);
> }
>
> }
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 564d97f..120549a 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -582,6 +582,7 @@ struct aer_recover_entry
> u8 devfn;
> u16 domain;
> int severity;
> + struct aer_capability_regs *regs;
> };
>
> static DEFINE_KFIFO(aer_recover_ring, struct aer_recover_entry,
> @@ -595,7 +596,7 @@ static DEFINE_SPINLOCK(aer_recover_ring_lock);
> static DECLARE_WORK(aer_recover_work, aer_recover_work_func);
>
> void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
> - int severity)
> + int severity, struct aer_capability_regs *aer_regs)
> {
> unsigned long flags;
> struct aer_recover_entry entry = {
> @@ -603,6 +604,7 @@ void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
> .devfn = devfn,
> .domain = domain,
> .severity = severity,
> + .regs = aer_regs,
> };
>
> spin_lock_irqsave(&aer_recover_ring_lock, flags);
> @@ -629,6 +631,7 @@ static void aer_recover_work_func(struct work_struct *work)
> PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));
> continue;
> }
> + cper_print_aer(pdev, entry.severity, entry.regs);
> do_recovery(pdev, entry.severity);
> pci_dev_put(pdev);
> }
> diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
> index 5ab1425..f277dc3 100644
> --- a/drivers/pci/pcie/aer/aerdrv_errprint.c
> +++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
> @@ -220,13 +220,19 @@ int cper_severity_to_aer(int cper_severity)
> }
> EXPORT_SYMBOL_GPL(cper_severity_to_aer);
>
> -void cper_print_aer(const char *prefix, struct pci_dev *dev, int cper_severity,
> +void cper_print_aer(struct pci_dev *dev, int cper_severity,
> struct aer_capability_regs *aer)
> {
> int aer_severity, layer, agent, status_strs_size, tlp_header_valid = 0;
> u32 status, mask;
> const char **status_strs;
>
> + /*
> + * TODO: This function needs to be re-written so that it's output
> + * matches the output of aer_print_error(). Right now, the output
> + * is formatted very differently.
> + */
> +
> aer_severity = cper_severity_to_aer(cper_severity);
> if (aer_severity == AER_CORRECTABLE) {
> status = aer->cor_status;
> @@ -244,7 +250,7 @@ void cper_print_aer(const char *prefix, struct pci_dev *dev, int cper_severity,
> agent = AER_GET_AGENT(aer_severity, status);
> dev_err(&dev->dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n",
> status, mask);
> - cper_print_bits(prefix, status, status_strs, status_strs_size);
> + cper_print_bits("", status, status_strs, status_strs_size);
> dev_err(&dev->dev, "aer_layer=%s, aer_agent=%s\n",
> aer_error_layer[layer], aer_agent_string[agent]);
> if (aer_severity != AER_CORRECTABLE)
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index ec10e1b..737f90a 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -49,10 +49,11 @@ static inline int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> }
> #endif
>
> -extern void cper_print_aer(const char *prefix, struct pci_dev *dev,
> +extern void cper_print_aer(struct pci_dev *dev,
> int cper_severity, struct aer_capability_regs *aer);
> extern int cper_severity_to_aer(int cper_severity);
> extern void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
> - int severity);
> + int severity,
> + struct aer_capability_regs *aer_regs);
> #endif //_AER_H_
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
> + /*
> + * TODO: This function needs to be re-written so that it's output
> + * matches the output of aer_print_error(). Right now, the output
> + * is formatted very differently.
> + */
So we have this big "TODO" comment sitting there very prominently ... which Linus
is bound to ask about if I ask him to pull this into 3.10-rcX ... what's the impact of
this? What should I say when he asks why should he pull this fix into 3.10 when
there is still some work to do? Is matching the output no big deal and can wait for
some future, while moving the pci bits to the work function needs to go in now?
-Tony
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > + /*
> > + * TODO: This function needs to be re-written so that it's output
> > + * matches the output of aer_print_error(). Right now, the
> output
> > + * is formatted very differently.
> > + */
>
> So we have this big "TODO" comment sitting there very prominently ...
> which Linus
> is bound to ask about if I ask him to pull this into 3.10-rcX ...
> what's the impact of
> this? What should I say when he asks why should he pull this fix into
> 3.10 when
> there is still some work to do? Is matching the output no big deal and
> can wait for
> some future, while moving the pci bits to the work function needs to go
> in now?
Tony,
You have a good point. Ideally the console output should be the same in both the aer and the cper case. The output in cper_print_error() does give us a reasonable amount of information, just not as detailed as I the aer case. Also now what we have the trace event for aer, the console output might be less important. This TODO is a note for future clean-up and is not directly related to the bug being fixed with this patch. Which lends to the argument of why put the TODO in this patch? Opportunistic. I don’t think we want to create a separate patch just for a TODO note.
So, why pull this patch in even though there is work to do? The patch fixes a warning that might cause customers un-due concern and removes a call in interrupt context that should not be there.
Lance
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, May 30, 2013 at 04:55:20AM +0000, Ortiz, Lance E wrote:
> This TODO is a note for future clean-up and is not directly related to
>the bug being fixed with this patch. Which lends to the argument of why
>put the TODO in this patch? Opportunistic. I don’t think we want to
>create a separate patch just for a TODO note.
Sounds to me, this TODO item should be on your TODO list - not in kernel
sources :-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 2013-05-30 at 09:28 +0200, Borislav Petkov wrote:
> On Thu, May 30, 2013 at 04:55:20AM +0000, Ortiz, Lance E wrote:
> > This TODO is a note for future clean-up and is not directly related to
> >the bug being fixed with this patch. Which lends to the argument of why
> >put the TODO in this patch? Opportunistic. I don’t think we want to
> >create a separate patch just for a TODO note.
>
> Sounds to me, this TODO item should be on your TODO list - not in kernel
> sources :-)
>
Also, that TODO sounds like there's output to userspace that can be
parsed by a userspace tool. If a tool expects the current format, it may
not be acceptable to change it later.
If the contents of this patch has nothing to do with the TODO, then
leave it out. It just confuses things.
-- Steve
> On Thu, May 30, 2013 at 04:55:20AM +0000, Ortiz, Lance E wrote:
> > This TODO is a note for future clean-up and is not directly related
> to
> >the bug being fixed with this patch. Which lends to the argument of
> why
> >put the TODO in this patch? Opportunistic. I don’t think we want to
> >create a separate patch just for a TODO note.
>
> Sounds to me, this TODO item should be on your TODO list - not in
> kernel
> sources :-)
> --
Makes sense. I will yank the TODO and resubmit the patch.
Lance
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> >
> > Sounds to me, this TODO item should be on your TODO list - not in
> kernel
> > sources :-)
> >
>
> Also, that TODO sounds like there's output to userspace that can be
> parsed by a userspace tool. If a tool expects the current format, it
> may
> not be acceptable to change it later.
>
> If the contents of this patch has nothing to do with the TODO, then
> leave it out. It just confuses things.
Steve, you do have a good point here. I am wondering if that is why we should consider changing the output to match aer_print_error(). The code path to aer_print_error() is the more common path where not as many platforms support the cper_print_error() path (firmware first AER). So it is more likely that any tools written would know how to parse the output from aer_print_error(). It would be good for those tools to support firmware first AER when it becomes more common. Of course this is purely conjecture. I have no idea if there are any tools that parse this text output.
Lance
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, May 30, 2013 at 02:04:42PM +0000, Ortiz, Lance E wrote:
> Steve, you do have a good point here. I am wondering if that is why
> we should consider changing the output to match aer_print_error().
> The code path to aer_print_error() is the more common path where not
> as many platforms support the cper_print_error() path (firmware first
> AER). So it is more likely that any tools written would know how to
> parse the output from aer_print_error(). It would be good for those
> tools to support firmware first AER when it becomes more common. Of
> course this is purely conjecture. I have no idea if there are any
> tools that parse this text output.
Whatever you end up doing, just make sure you've hammered out the
information going out to userspace to be clear, succinct and complete
(as far as possible, of course). Because once you cast it in stone and
tools start using it, changing its format is a huge PITA, if at all
possible.
And if the error formats are compatible, then sharing the format is
obviously advantageous.