Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, May 30, 2013 at 02:04:42PM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, May 30, 2013 at 02:04:42PM +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
> >
> > 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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
> On Thu, May 30, 2013 at 04:55:20AM +, 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
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, 2013-05-30 at 09:28 +0200, Borislav Petkov wrote: > On Thu, May 30, 2013 at 04:55:20AM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, May 30, 2013 at 04:55:20AM +, 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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, May 30, 2013 at 04:55:20AM +, 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. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, 2013-05-30 at 09:28 +0200, Borislav Petkov wrote: On Thu, May 30, 2013 at 04:55:20AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
On Thu, May 30, 2013 at 04:55:20AM +, 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
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
> > + /*
> > +* 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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
> + /*
> + * 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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
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
> Acked-by: Borislav Petkov
Acked-by: Rafael J. Wysocki
> ---
>
> 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,
>
Re: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
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 lance.or...@hp.com
Acked-by: Borislav Petkov b...@suse.de
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
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(%sport_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(
%sbridge: 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,
};
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
+ /*
+ * 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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3] aerdrv: Move cper_print_aer() call out of interrupt context
+ /*
+* 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�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
