[PATCH] PCI PHB unit id detection interface change

[Olof Johansson]

linas at austin.ibm.com wrote:

>Hi,
>
>I'm looking for sanity-check/review of the following patch.
>Turns out ath the kernel has been using a deprecated firmware
>interface for discovring the PCI PHB Unit Id's.   The PHN Unit
>ID's are used for a variety of PCI operations.  The patch
>below is a change to use the recommended API.
>
>Its been tested on one-each of power3,4,and 5 boxes; however,
>given that its an API change,  I have some concern that someone
>might get broken by this.  Please review.
>

Linas,

Your code has some CodingStyle breakage. Mostly nitpicky stuff, see below.

>------------------------------------------------------------------------
>
>Index: arch/ppc64/kernel/pSeries_pci.c
>===================================================================
>RCS file: /home/linas/cvsroot/eeh/arch/ppc64/kernel/pSeries_pci.c,v
>retrieving revision 1.1
>diff -u -p -r1.1 pSeries_pci.c
>--- a/arch/ppc64/kernel/pSeries_pci.c	8 Jun 2004 22:52:54 -0000	1.1
>+++ b/arch/ppc64/kernel/pSeries_pci.c	11 Jun 2004 21:56:41 -0000
>@@ -353,6 +353,55 @@ static void python_countermeasures(unsig
> 	iounmap(chip_regs);
> }
>
>+void init_pci_config_tokens (void)
>
Should this be __init?

>+{
>+	read_pci_config = rtas_token("read-pci-config");
>+	write_pci_config = rtas_token("write-pci-config");
>+	ibm_read_pci_config = rtas_token("ibm,read-pci-config");
>+	ibm_write_pci_config = rtas_token("ibm,write-pci-config");
>+}
>+
>+unsigned long get_phb_buid (struct device_node *phb)
>+{
>+	int addr_cells;
>+	unsigned int *buid_vals;
>+	unsigned int len;
>+	unsigned long buid;
>+
>+#define NEW_STYLE_BUIDS
>+#ifdef NEW_STYLE_BUIDS
>+	if (-1 == ibm_read_pci_config) return 0;
>+
>+	/* PHB's will always be children of the root node,
>+	 * or so it is promised by the current firmware. */
>+	if (NULL == phb->parent) return 0;
>
It's common practice in the Linux kernel to do tests in form (<variable>
== <constant>). Good compilers should warn about missing equal signs
anyway. :)

>+	if (phb->parent->parent) return 0;
>+
>+	buid_vals = (unsigned int *) get_property(phb, "reg", &len);
>+	if (NULL == buid_vals) return 0;
>
Break if statements into two lines, please, all 4 above
Also, another constant == variable check

>+
>+	addr_cells = prom_n_addr_cells(phb);
>+	if (addr_cells == 1) {
>+		buid = (unsigned long) buid_vals[0];
>+	} else {
>+		buid = (((unsigned long)buid_vals[0]) << 32UL) |
>+			(((unsigned long)buid_vals[1]) & 0xffffffff);
>+	}
>+#else
>+	// The use of ibm,fw-phb-id is deprecated.  Keep a copy of this
>+	// code for reference, in case the above code needs to be rolled back.
>
Please stick to C-style comments, i.e. /* */

Also, revision control is good enough to track old code for reference,
instead of keeping it cluttered with #ifdefs. So please take out the old
code alltogether.

>+	buid_vals = (int *) get_property(phb, "ibm,fw-phb-id", &len);
>+	if (buid_vals == NULL) return 0;
>
break in two lines

>+	if (len < 2 * sizeof(int)) {
>+			buid = (unsigned long) buid_vals[0];
>+	} else {
>+		buid = (((unsigned long)buid_vals[0]) << 32UL) |
>+			(((unsigned long)buid_vals[1]) & 0xffffffff);
>+	}
>+#endif
>+	return buid;
>+}
>+
> struct pci_controller *alloc_phb(struct device_node *dev,
> 				 unsigned int addr_size_words)
> {
>@@ -360,7 +409,6 @@ struct pci_controller *alloc_phb(struct
> 	unsigned int *ui_ptr = NULL, len;
> 	struct reg_property64 reg_struct;
> 	int *bus_range;
>-	int *buid_vals;
> 	char *model;
> 	enum phb_types phb_type;
>  	struct property *of_prop;
>@@ -431,18 +479,7 @@ struct pci_controller *alloc_phb(struct
> 	phb->arch_data   = dev;
> 	phb->ops = &rtas_pci_ops;
>
>-	buid_vals = (int *) get_property(dev, "ibm,fw-phb-id", &len);
>-
>-	if (buid_vals == NULL) {
>-		phb->buid = 0;
>-	} else {
>-		if (len < 2 * sizeof(int))
>-			// Support for new OF that only has 1 integer for buid.
>-			phb->buid = (unsigned long)buid_vals[0];
>-		else
>-			phb->buid = (((unsigned long)buid_vals[0]) << 32UL) |
>-				(((unsigned long)buid_vals[1]) & 0xffffffff);
>-	}
>+	phb->buid = get_phb_buid (dev);
>
Take out space between function name and ()

>
> 	return phb;
> }
>@@ -456,10 +493,7 @@ unsigned long __init find_and_init_phbs(
> 	unsigned int *opprop;
> 	struct device_node *root = of_find_node_by_path("/");
>
>-	read_pci_config = rtas_token("read-pci-config");
>-	write_pci_config = rtas_token("write-pci-config");
>-	ibm_read_pci_config = rtas_token("ibm,read-pci-config");
>-	ibm_write_pci_config = rtas_token("ibm,write-pci-config");
>+	init_pci_config_tokens();
>
> 	if (naca->interrupt_controller == IC_OPEN_PIC) {
> 		opprop = (unsigned int *)get_property(root,
>--- arch/ppc64/kernel/eeh.c.7.69-orig	2004-06-11 14:10:19.000000000 -0500
>+++ arch/ppc64/kernel/eeh.c	2004-06-11 16:50:55.000000000 -0500
>@@ -40,6 +40,10 @@
> #define CONFIG_ADDR(busno, devfn) \
> 		(((((busno) & 0xff) << 8) | ((devfn) & 0xf8)) << 8)
>
>+/* From pSeries_pci.h */
>+extern void init_pci_config_tokens (void);
>+extern unsigned long get_phb_buid (struct device_node *);
>+
>

> /* RTAS tokens */
> static int ibm_set_eeh_option;
> static int ibm_set_slot_reset;
>@@ -645,7 +649,7 @@ static void *early_enable_eeh(struct dev
> 			       dn->full_name);
> #endif
> 		} else {
>-			printk(KERN_WARNING "EEH: %s: rtas_call failed.\n",
>+			printk(KERN_WARNING "EEH: %s: could not enable EEH, rtas_call failed.\n",
> 			       dn->full_name);
> 		}
> 	} else {
>@@ -709,24 +713,16 @@ void __init eeh_init(void)
> 	}
>
> 	/* Enable EEH for all adapters.  Note that eeh requires buid's */
>+	init_pci_config_tokens();
>

Why do you need this here? Is EEH code ever called before
find_and_init_phbs? If so, can't you make the init_pci_config_tokens()
an early_initcall() to avoid calling it twice?

> 	for (phb = of_find_node_by_name(NULL, "pci"); phb;
> 	     phb = of_find_node_by_name(phb, "pci")) {
>-		int len;
>-		int *buid_vals;
>+		unsigned long buid;
>
>-		buid_vals = (int *)get_property(phb, "ibm,fw-phb-id", &len);
>-		if (!buid_vals)
>-			continue;
>-		if (len == sizeof(int)) {
>-			info.buid_lo = buid_vals[0];
>-			info.buid_hi = 0;
>-		} else if (len == sizeof(int)*2) {
>-			info.buid_hi = buid_vals[0];
>-			info.buid_lo = buid_vals[1];
>-		} else {
>-			printk(KERN_INFO "EEH: odd ibm,fw-phb-id len returned: %d\n", len);
>-			continue;
>-		}
>+		buid = get_phb_buid (phb);
>+		if (0 == buid) continue;
>
break in two lines

>+
>+		info.buid_lo = BUID_LO(buid);
>+		info.buid_hi = BUID_HI(buid);
> 		traverse_pci_devices(phb, early_enable_eeh, NULL, &info);
> 	}
>
>
>


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/