Re: [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver

["Zhang, Yanmin" <yanmin_zhang@linux.intel.com>]

On Thu, 2006-07-13 at 00:26, Andi Kleen wrote:
> "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> writes:
> 
> > With Arjan's comments, I changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL.
> > Sorry for flooding your emailbox again. :)
> 
> This means that non GPL drivers will reimplement these functions 
> on their own (which is possible, just ugly) The fallout of them getting that wrong
> might be significant.
Yeah, it looks like a hard decision. 

> 
> I would change it back. _GPL should be only for core services, not
> for generic driver interfaces.
> 
> > --- linux-2.6.17/drivers/pci/pcie/aer/aerdrv_core.c	1970-01-01 08:00:00.000000000 +0800
> > +++ linux-2.6.17_aer/drivers/pci/pcie/aer/aerdrv_core.c	2006-07-12 15:47:38.000000000 +0800
> > @@ -0,0 +1,737 @@
> > +/*
> > + * Copyright (C) 2006 Intel
> > + *	Tom Long Nguyen (tom.l.nguyen@intel.com)
> > + *	Zhang Yanmin (yanmin.zhang@intel.com)
> 
> Comment describing what the file does missing. At least one paragraph
> of design rationale would be good 
Ok. I will add it.

> 
> > +
> > +config PCIEAER
> > +	tristate "Root Port Advanced Error Reporting support"
> > +	depends on PCIEPORTBUS 
> > +	default y
> > +	help
> > +	  This enables Root Port Advanced Error Reporting (AER) driver
> > +	  support. Error reporting messages sent to Root Port will be
> > +	  handled by PCI Express AER driver.
> 
> I hope it's clear from the context this is PCI-E specific?
The dependence on PCIEPORTBUS means it's of PCI-E specific. I will add
more description.


> 
> > --- linux-2.6.17/drivers/pci/pcie/aer/Makefile	1970-01-01 08:00:00.000000000 +0800
> > +++ linux-2.6.17_aer/drivers/pci/pcie/aer/Makefile	2006-06-22 16:46:29.000000000 +0800
> > @@ -0,0 +1,10 @@
> > +#
> > +# Makefile for PCI-Express Root Port Advanced Error Reporting Driver
> > +#
> > +
> > +obj-$(CONFIG_PCIEAER)		+= aerdriver.o
> > +aerdrv_acpi-$(CONFIG_ACPI)	+= aerdrv_acpi.o
> > +
> > +aerdriver-objs		:= aerdrv_errprint.o aerdrv_core.o aerdrv.o
> > +aerdriver-objs		+= $(aerdrv_acpi-y)
> > +
> > --- linux-2.6.17/drivers/pci/pcie/Kconfig	2006-06-22 16:26:43.000000000 +0800
> > +++ linux-2.6.17_aer/drivers/pci/pcie/Kconfig	2006-06-22 16:46:29.000000000 +0800
> > @@ -34,3 +34,4 @@ config HOTPLUG_PCI_PCIE_POLL_EVENT_MODE
> >  	   
> >  	  When in doubt, say N.
> >  
> > +source "drivers/pci/pcie/aer/Kconfig"
> > --- linux-2.6.17/drivers/pci/pcie/Makefile	2006-06-22 16:26:43.000000000 +0800
> > +++ linux-2.6.17_aer/drivers/pci/pcie/Makefile	2006-06-22 16:46:29.000000000 +0800
> > @@ -5,3 +5,6 @@
> >  pcieportdrv-y			:= portdrv_core.o portdrv_pci.o portdrv_bus.o
> >  
> >  obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
> > +
> > +# Build PCI Express AER if needed
> > +obj-$(CONFIG_PCIEAER)		+= aer/
> > --- linux-2.6.17/drivers/pci/pcie/aer/aerdrv_errprint.c	1970-01-01 08:00:00.000000000 +0800
> > +++ linux-2.6.17_aer/drivers/pci/pcie/aer/aerdrv_errprint.c	2006-06-22 16:46:29.000000000 +0800
> > @@ -0,0 +1,216 @@
> > +/*
> > + * Copyright (C) 2006 Intel
> > + *	Tom Long Nguyen (tom.l.nguyen@intel.com)
> > + *	Zhang Yanmin (yanmin.zhang@intel.com)
> > + *
> 
> Comment what the code does missing.
Ok.

> 
> At least one paragraph of design rationale would be good.
> 
> > +	"Unknown Error Bit 22  ",	/* Bit Position 22	*/
> > +	"Unknown Error Bit 23  ", 	/* Bit Position 23	*/
> > +	"Unknown Error Bit 24  ",	/* Bit Position 24	*/
> > +	"Unknown Error Bit 25  ", 	/* Bit Position 25	*/
> > +	"Unknown Error Bit 26  ", 	/* Bit Position 26 	*/
> > +	"Unknown Error Bit 27  ",	/* Bit Position 27	*/
> > +	"Unknown Error Bit 28  ",	/* Bit Position 28	*/
> > +	"Unknown Error Bit 29  ", 	/* Bit Position 29	*/
> > +	"Unknown Error Bit 30  ", 	/* Bit Position 30 	*/
> > +	"Unknown Error Bit 31  "	/* Bit Position 31	*/
> 
> Make all the unknown error bits a NULL and use a sprintf in the 
> decoder instead.
I will try.

> 
> Similar for the following arrays.
> > +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
> > +{
> > +	char * errmsg;
> > +	int err_layer, agent;
> > +
> > +	printk(KERN_ERR "+------ PCI-Express Device Error ------+\n");
> > +	printk(KERN_ERR "Error Severity\t\t: %s\n",
> > +		aer_error_severity_string[info->severity]);
> > +
> > +	if ( info->status == 0) {
> > +                printk(KERN_ERR "PCIE Bus Error type\t: (Unaccessible)\n");
> 
> KERN_ERR? THis means it will appear on consoles, won't it?
> And surely not all these errors are fatal enough to need user attention
> immediately and I bet there will be some devices who report these
> errors unnecessarily. I would use a lower log level.
It should be more elaborated. I will change it.

> 
> Also I would suggest you add something in the documentation
> on what the messages mean exactly and how to decode them. I'm sure that will be a FAQ.
I will add more description, but we couldn't hope it has detailed info like
the pci-e specs has. 

I really appreciate your comments.

Yanmin