Yup. You are right. I thought I had it enabled, I will send out the new patch
soon.
Lance
> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Thursday, November 29, 2012 3:12 PM
> To: Ortiz, Lance E
> Cc: bhelg...@google.com; lance_or...@hot
> Ok, cool.
>
> Lance, care to move the TP to a new header called
> include/trace/events/ras.h in your next iteration of the patches?
Will do.
Lance
> > +/*
> > + * PCIe Advanced Error Reporting (AER) PCIE Report Error
>
> Why do you insist on keeping this convoluted compound name? What does
> "PCIe AER PCIe Report Error" even mean?
>
> This is not unreadable technical documentation but something people
> should actually understand. And it is
> > +#define correctable_error_string \
> > + {BIT(0),"Receiver Error"}, \
> > + {BIT(6),"Bad TLP"}, \
> > + {BIT(7),"Bad DLLP"},\
> > + {BIT(8),"RELAY_NUM Rollover"}, \
> > + {
> > +#define correctable_error_string \
> > + {BIT(0),"Receiver Error"}, \
> > + {BIT(6),"Bad TLP"}, \
> > + {BIT(7),"Bad DLLP"},\
> > + {BIT(8),"RELAY_NUM Rollover"}, \
> > + {
>
> Maybe we should ask Yanmin who added those strings in
> 6c2b374d74857e892080ee726184ec1d15e7d4e4; CCed.
>
> Also, it would make a lot of sense to have those string definitions
> at one place and then reuse them instead of define them again in the
> tracepoint header and they get out of sync a
> Hi again Lance.
>
> > diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c
> b/drivers/pci/pcie/aer/aerdrv_errprint.c
> []
> > @@ -228,9 +228,14 @@ void cper_print_aer(struct pci_dev *dev, int
> cper_severity,
> > int aer_severity, layer, agent, status_strs_size,
> tlp_header_valid = 0;
> >
> > + if (!dev) {
> > + pr_info("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);
>
> Hmm... please correct if I'm wrong, but an error hap
> > + char *prefix = NULL;
>
> What are you doing here? You dropped the 'prefix' argument being passed
> down in this function and are defining a local variable of the same
> name
> which is used in the function later:
>
> printk("%s""aer_status: 0x%08x, aer_mask: 0x%08x\n",
>
>
> Here it is, you're initializing 'prefix' here although it is being
> used in the previous patch. You should concentrate the whole prefix
> initialization and passing in one patch so that there are no breakages.
Yes I probably could have broken up the patches in a cleaner way. Thanks.
>
> A
> > + /*
> > +* 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
> 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? Opportunisti
> >
> > 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.
>
> I
> > [ 69.965933] [ cut here ]
> > [ 69.965938] WARNING: at /data/kernel/linux-
> git/drivers/pci/search.c:214 pci_get_dev_by_id+0x8a/0x90()
> > [ 69.965941] Hardware name: PRIMERGY RX200 S7
> > [ 69.965946] Modules linked in:
> > [ 69.965950] Pid: 0, comm: swapper/
> Hmm, not sure.
>
> Off the top of my head, maybe add the whole code around:
>
> #ifdef CONFIG_ACPI_APEI_PCIEAER
> ...
>
> #endif
>
> in cper_print_pcie() into a separate function which is called from a
> workqueue right after the interrupt is done.. Or something to that
> effect.
>
Thanks.
> > The only reason we are calling pci_get_domain_bus_and_slot() is to
> get
> > the pci_dev* to pass into cper_print_aer() so we can have the
> device's
> > name to put into the trace event for AER. If we can find another way
> > to get the device name for the trace event we could remove this call
Rafael,
Thanks for your feedback.
> The way the changes are described here isn't particularly clear to me.
I will try to make it a little more clear.
> Also, since aer_recover_work_func() is going to be the only existing
> caller of
> cper_print_aer() after this change, as far as I can say,
Comment Below...
> -Original Message-
> From: Yasuaki Ishimatsu [mailto:isimatu.yasu...@jp.fujitsu.com]
> Sent: Tuesday, September 18, 2012 11:45 PM
> To: Ortiz, Lance E
> Cc: r...@landley.net; l...@kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.k
18 matches
Mail list logo
