[PATCH 1/4] powerpc: Add EEH sysfs blinkenlights

[Nathan Lynch]

Hi Linas-

Linas Vepstas wrote:
> 
> Add sysfs blinkenlights for EEH statistics. Shuffle the
> eeh_add_device_tree() call so that it appears in the correct
> sequence.

( blinkenlights? :)

To me this seems a somewhat terse changelog considering that the patch
introduces a user-visible interface.  The changelog does not really
say what the code is doing or why, or who will use it.


> Index: linux-2.6.22-rc1/arch/powerpc/platforms/pseries/eeh_sysfs.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.22-rc1/arch/powerpc/platforms/pseries/eeh_sysfs.c	2007-05-23 11:57:23.000000000 -0500
> @@ -0,0 +1,84 @@
> +/*
> + * Sysfs entries for PCI Error Recovery for PAPR-compliant platform.

platforms?

> +#include <linux/pci.h>
> +#include <asm/ppc-pci.h>
> +#include <asm/pci-bridge.h>
> +#include <linux/kobject.h>

Include linux/ before asm/ please.

> +/**
> + * EEH_SHOW_ATTR -- create sysfs entry for eeh statistic
> + * @_name: name of file in sysfs directory
> + * @_memb: name of member in struct pci_dn to access
> + * @_format: printf format for display
> + *
> + * All of the attributes look very similar, so just
> + * auto-gen a cut-n-paste routine to display them.
> + */
> +#define EEH_SHOW_ATTR(_name,_memb,_format)               \
> +static ssize_t eeh_show_##_name(struct device *dev,      \
> +		struct device_attribute *attr, char *buf)          \

I have been frustrated by similar constructions more than once in the
midst of debugging.  I know this has become a common practice with
sysfs-related code, but using cpp to generate function names
completely defeats grep etc. when you're trying to track down a
problem, and I'd not like to see this sort of thing propagated.


> --- linux-2.6.22-rc1.orig/include/asm-powerpc/ppc-pci.h	2007-05-23 11:43:42.000000000 -0500
> +++ linux-2.6.22-rc1/include/asm-powerpc/ppc-pci.h	2007-05-23 11:44:43.000000000 -0500
> @@ -139,6 +139,9 @@ void eeh_clear_slot (struct device_node 
>   */
>  struct device_node * find_device_pe(struct device_node *dn);
>  
> +void eeh_sysfs_add_device(struct pci_dev *pdev);
> +void eeh_sysfs_remove_device(struct pci_dev *pdev);

Don't you need dummy static inline placeholders for CONFIG_EEH=n?