[PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Linas Vepstas]

Use the correct CONFIG_ option to mark off the EEH bits.
Move the EEH bits to the bottom of the struct.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 include/asm-powerpc/pci-bridge.h |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h
===================================================================
--- linux-2.6.22-rc1.orig/include/asm-powerpc/pci-bridge.h	2007-05-21 18:13:00.000000000 -0500
+++ linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h	2007-05-21 18:13:36.000000000 -0500
@@ -70,19 +70,20 @@ struct pci_dn {
 	int	devfn;			/* pci device and function number */
 	int	class_code;		/* pci device class */
 
-#ifdef CONFIG_PPC_PSERIES
-	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
-	int	eeh_config_addr;
-	int	eeh_pe_config_addr; /* new-style partition endpoint address */
-	int 	eeh_check_count;	/* # times driver ignored error */
-	int 	eeh_freeze_count;	/* # times this device froze up. */
-#endif
 	int	pci_ext_config_space;	/* for pci devices */
 	struct  pci_controller *phb;	/* for pci devices */
 	struct	iommu_table *iommu_table;	/* for phb's or bridges */
 	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
 	struct	device_node *node;	/* back-pointer to the device_node */
+
+#ifdef CONFIG_EEH
+	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
+	int	eeh_config_addr;
+	int	eeh_pe_config_addr; /* new-style partition endpoint address */
+	int 	eeh_check_count;	/* # times driver ignored error */
+	int 	eeh_freeze_count;	/* # times this device froze up. */
 	u32	config_space[16];	/* saved PCI config space */
+#endif
 };
 
 /* Get the pointer to a device_node's pci_dn */

Re: [PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On Mon, 2007-05-21 at 18:18 -0500, Linas Vepstas wrote:
> Use the correct CONFIG_ option to mark off the EEH bits.
> Move the EEH bits to the bottom of the struct.
> 
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> 
> ----
>  include/asm-powerpc/pci-bridge.h |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h
> ===================================================================
> --- linux-2.6.22-rc1.orig/include/asm-powerpc/pci-bridge.h	2007-05-21 18:13:00.000000000 -0500
> +++ linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h	2007-05-21 18:13:36.000000000 -0500
> @@ -70,19 +70,20 @@ struct pci_dn {
>  	int	devfn;			/* pci device and function number */
>  	int	class_code;		/* pci device class */
>  
> -#ifdef CONFIG_PPC_PSERIES
> -	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
> -	int	eeh_config_addr;
> -	int	eeh_pe_config_addr; /* new-style partition endpoint address */
> -	int 	eeh_check_count;	/* # times driver ignored error */
> -	int 	eeh_freeze_count;	/* # times this device froze up. */
> -#endif

You're making the struct 4 bytes larger by creating a hole here :(

>  	int	pci_ext_config_space;	/* for pci devices */
>  	struct  pci_controller *phb;	/* for pci devices */
>  	struct	iommu_table *iommu_table;	/* for phb's or bridges */
>  	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
>  	struct	device_node *node;	/* back-pointer to the device_node */
> +
> +#ifdef CONFIG_EEH
> +	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
> +	int	eeh_config_addr;
> +	int	eeh_pe_config_addr; /* new-style partition endpoint address */
> +	int 	eeh_check_count;	/* # times driver ignored error */
> +	int 	eeh_freeze_count;	/* # times this device froze up. */
>  	u32	config_space[16];	/* saved PCI config space */
> +#endif

It looks correct, but I think it's worth mentioning in the changelog
that config_space was previously unconditionally defined, but is now
within CONFIG_EEH.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Linas Vepstas]

On Tue, May 22, 2007 at 10:40:59AM +1000, Michael Ellerman wrote:
> On Mon, 2007-05-21 at 18:18 -0500, Linas Vepstas wrote:
> > Use the correct CONFIG_ option to mark off the EEH bits.
> 
> You're making the struct 4 bytes larger by creating a hole here :(

Hmm. Perhaps I could shuffle one of the ints down ...  

I figured there might be some vague cache-line benefits to
getting the eeh stuff out of the way. 

> >  	u32	config_space[16];	/* saved PCI config space */
> > +#endif
> 
> It looks correct, but I think it's worth mentioning in the changelog
> that config_space was previously unconditionally defined, but is now
> within CONFIG_EEH.

I could just tell that someone would remark on this; why I didn't
mention it, I don't know.

--linas 

[PATCH] (revised) pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Linas Vepstas]

Use the correct CONFIG_ option to mark off the EEH bits.
Move the EEH bits to the bottom of the struct.
The config_space array is used by EEH only; it does not
need to be part of the struct for non-pseries machines.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
Revised patch, per commments from Michael Ellerman.

 include/asm-powerpc/pci-bridge.h |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Index: linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h
===================================================================
--- linux-2.6.22-rc1.orig/include/asm-powerpc/pci-bridge.h	2007-05-21 18:13:00.000000000 -0500
+++ linux-2.6.22-rc1/include/asm-powerpc/pci-bridge.h	2007-05-22 12:03:50.000000000 -0500
@@ -70,19 +70,21 @@ struct pci_dn {
 	int	devfn;			/* pci device and function number */
 	int	class_code;		/* pci device class */
 
-#ifdef CONFIG_PPC_PSERIES
+	struct  pci_controller *phb;	/* for pci devices */
+	struct	iommu_table *iommu_table;	/* for phb's or bridges */
+	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
+	struct	device_node *node;	/* back-pointer to the device_node */
+
+	int	pci_ext_config_space;	/* for pci devices */
+
+#ifdef CONFIG_EEH
 	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
 	int	eeh_config_addr;
 	int	eeh_pe_config_addr; /* new-style partition endpoint address */
 	int 	eeh_check_count;	/* # times driver ignored error */
 	int 	eeh_freeze_count;	/* # times this device froze up. */
-#endif
-	int	pci_ext_config_space;	/* for pci devices */
-	struct  pci_controller *phb;	/* for pci devices */
-	struct	iommu_table *iommu_table;	/* for phb's or bridges */
-	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
-	struct	device_node *node;	/* back-pointer to the device_node */
 	u32	config_space[16];	/* saved PCI config space */
+#endif
 };
 
 /* Get the pointer to a device_node's pci_dn */

Re: [PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Tue, 2007-05-22 at 12:00 -0500, Linas Vepstas wrote:
> On Tue, May 22, 2007 at 10:40:59AM +1000, Michael Ellerman wrote:
> > On Mon, 2007-05-21 at 18:18 -0500, Linas Vepstas wrote:
> > > Use the correct CONFIG_ option to mark off the EEH bits.
> > 
> > You're making the struct 4 bytes larger by creating a hole here :(
> 
> Hmm. Perhaps I could shuffle one of the ints down ...  
> 
> I figured there might be some vague cache-line benefits to
> getting the eeh stuff out of the way. 

Yeah who knows. The hole doesn't really matter unless the struct gets
bigger, having the EEH stuff at the bottom makes it read better which is
probably more important.


> > >  	u32	config_space[16];	/* saved PCI config space */
> > > +#endif
> > 
> > It looks correct, but I think it's worth mentioning in the changelog
> > that config_space was previously unconditionally defined, but is now
> > within CONFIG_EEH.
> 
> I could just tell that someone would remark on this; why I didn't
> mention it, I don't know.

Me neither :)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pseries: asm/pci-bridge.h CONFIG_ minor cleanup

[Linas Vepstas]

On Wed, May 23, 2007 at 11:11:32AM +1000, Michael Ellerman wrote:
> 
> Yeah who knows. The hole doesn't really matter unless the struct gets
> bigger, having the EEH stuff at the bottom makes it read better which is
> probably more important.

Havin become proficient at adding hex addrs in my head, I have also
come to appreciate structs which have all the important/hot stuff
at the top, instead of large filler holes that one has to skip over
during debugging.

--linas