On Wed, Nov 26, 2014 at 10:02:31AM +1100, Benjamin Herrenschmidt wrote:

On Tue, 2014-11-25 at 09:49 +1100, Gavin Shan wrote:

quoted

The patch applies code cleanup to RPA modules to address following
issues and it shouldn't affect the logic:

   * Coding style issue: removed unnecessary "break" for default case
     in switch statement and added default case; removed unnecessary
     braces or parenthese for if or return statements; removed
     unecessary return statements
   * Refactor rpaphp_get_sensor_state() and find_php_slot_pci_node()
     to avoid nested if statements
   * Drop is_registered(), is_php_type(), is_dlpar_capable()

Why dropping those helpers ? Make them inline if you want to avoid
polluting the namespace but I don't see what you gain in code
readability by making the functions bigger...

Yeah, it's pointless to drop those functions and I'll keep them in
next revision.

Thanks,
Gavin

Ben.

quoted

Signed-off-by: Gavin Shan <redacted>
---
 drivers/pci/hotplug/rpadlpar_core.c | 90 +++++++++++++++++++------------------
 drivers/pci/hotplug/rpaphp_core.c   | 57 ++++++++++-------------
 drivers/pci/hotplug/rpaphp_pci.c    | 43 +++++++++---------
 drivers/pci/hotplug/rpaphp_slot.c   | 25 ++++-------
 4 files changed, 101 insertions(+), 114 deletions(-)

diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
index 7660232..35da3b3 100644
--- a/drivers/pci/hotplug/rpadlpar_core.c
+++ b/drivers/pci/hotplug/rpadlpar_core.c

@@ -52,30 +52,35 @@ static struct device_node *find_vio_slot_node(char *drc_name)
 
 	while ((dn = of_get_next_child(parent, dn))) {
 		rc = rpaphp_get_drc_props(dn, NULL, &name, NULL, NULL);
-		if ((rc == 0) && (!strcmp(drc_name, name)))
-			break;
+		if (rc)
+			continue;
+
+		if (!strcmp(drc_name, name))
+			return dn;
 	}
 
-	return dn;
+	return NULL;
 }
 
 /* Find dlpar-capable pci node that contains the specified name and type */
 static struct device_node *find_php_slot_pci_node(char *drc_name,
 						  char *drc_type)
 {
-	struct device_node *np = NULL;
+	struct device_node *dn = NULL;
 	char *name;
 	char *type;
 	int rc;
 
-	while ((np = of_find_node_by_name(np, "pci"))) {
-		rc = rpaphp_get_drc_props(np, NULL, &name, &type, NULL);
-		if (rc == 0)
-			if (!strcmp(drc_name, name) && !strcmp(drc_type, type))
-				break;
+	while ((dn = of_find_node_by_name(dn, "pci"))) {
+		rc = rpaphp_get_drc_props(dn, NULL, &name, &type, NULL);
+		if (rc)
+			continue;
+
+		if (!strcmp(drc_name, name) && !strcmp(drc_type, type))
+			return dn;
 	}
 
-	return np;
+	return NULL;
 }
 
 static struct device_node *find_dlpar_node(char *drc_name, int *node_type)

@@ -239,10 +244,9 @@ static int dlpar_add_phb(char *drc_name, struct device_node *dn)
 {
 	struct pci_controller *phb;
 
-	if (PCI_DN(dn) && PCI_DN(dn)->phb) {
-		/* PHB already exists */
+	/* PHB already exists */
+	if (PCI_DN(dn) && PCI_DN(dn)->phb)
 		return -EINVAL;
-	}
 
 	phb = init_phb_dynamic(dn);
 	if (!phb)

@@ -299,15 +303,18 @@ int dlpar_add_slot(char *drc_name)
 	}
 
 	switch (node_type) {
-		case NODE_TYPE_VIO:
-			rc = dlpar_add_vio_slot(drc_name, dn);
-			break;
-		case NODE_TYPE_SLOT:
-			rc = dlpar_add_pci_slot(drc_name, dn);
-			break;
-		case NODE_TYPE_PHB:
-			rc = dlpar_add_phb(drc_name, dn);
-			break;
+	case NODE_TYPE_VIO:
+		rc = dlpar_add_vio_slot(drc_name, dn);
+		break;
+	case NODE_TYPE_SLOT:
+		rc = dlpar_add_pci_slot(drc_name, dn);
+		break;
+	case NODE_TYPE_PHB:
+		rc = dlpar_add_phb(drc_name, dn);
+		break;
+	default:
+		rc = -EINVAL;
+		goto exit;
 	}
 
 	printk(KERN_INFO "%s: slot %s added\n", DLPAR_MODULE_NAME, drc_name);

@@ -429,15 +436,18 @@ int dlpar_remove_slot(char *drc_name)
 	}
 
 	switch (node_type) {
-		case NODE_TYPE_VIO:
-			rc = dlpar_remove_vio_slot(drc_name, dn);
-			break;
-		case NODE_TYPE_PHB:
-			rc = dlpar_remove_phb(drc_name, dn);
-			break;
-		case NODE_TYPE_SLOT:
-			rc = dlpar_remove_pci_slot(drc_name, dn);
-			break;
+	case NODE_TYPE_VIO:
+		rc = dlpar_remove_vio_slot(drc_name, dn);
+		break;
+	case NODE_TYPE_PHB:
+		rc = dlpar_remove_phb(drc_name, dn);
+		break;
+	case NODE_TYPE_SLOT:
+		rc = dlpar_remove_pci_slot(drc_name, dn);
+		break;
+	default:
+		rc = -EINVAL;
+		goto exit;
 	}
 	vm_unmap_aliases();
 

@@ -447,20 +457,15 @@ exit:
 	return rc;
 }
 
-static inline int is_dlpar_capable(void)
-{
-	int rc = rtas_token("ibm,configure-connector");
-
-	return (int) (rc != RTAS_UNKNOWN_SERVICE);
-}
-
-int __init rpadlpar_io_init(void)
+static int __init rpadlpar_io_init(void)
 {
 	int rc = 0;
 
-	if (!is_dlpar_capable()) {
+	/* Check if we have DLPAR capability */
+	rc = rtas_token("ibm,configure-connector");
+	if (rc == RTAS_UNKNOWN_SERVICE) {
 		printk(KERN_WARNING "%s: partition not DLPAR capable\n",
-			__func__);
+		       __func__);
 		return -EPERM;
 	}
 

@@ -468,10 +473,9 @@ int __init rpadlpar_io_init(void)
 	return rc;
 }
 
-void rpadlpar_io_exit(void)
+static void __exit rpadlpar_io_exit(void)
 {
 	dlpar_sysfs_exit();
-	return;
 }
 
 module_init(rpadlpar_io_init);

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index f2945fa..ff800df 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c

@@ -74,7 +74,6 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 value)
 		break;
 	default:
 		value = 1;
-		break;
 	}
 
 	rc = rtas_set_indicator(DR_INDICATOR, slot->index, value);

@@ -94,7 +93,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	int retval, level;
 	struct slot *slot = (struct slot *)hotplug_slot->private;
 
-	retval = rtas_get_power_level (slot->power_domain, &level);
+	retval = rtas_get_power_level(slot->power_domain, &level);
 	if (!retval)
 		*value = level;
 	return retval;

@@ -161,7 +160,6 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
 		break;
 	default:
 		speed = PCI_SPEED_UNKNOWN;
-		break;
 	}
 
 	return speed;

@@ -178,17 +176,17 @@ static int get_children_props(struct device_node *dn, const int **drc_indexes,
 	types = of_get_property(dn, "ibm,drc-types", NULL);
 	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
 
-	if (!indexes || !names || !types || !domains) {
-		/* Slot does not have dynamically-removable children */
+	/* Slot does not have dynamically-removable children */
+	if (!indexes || !names || !types || !domains)
 		return -EINVAL;
-	}
+
 	if (drc_indexes)
 		*drc_indexes = indexes;
+	/* &drc_names[1] contains NULL terminated slot names */
 	if (drc_names)
-		/* &drc_names[1] contains NULL terminated slot names */
 		*drc_names = names;
+	/* &drc_types[1] contains NULL terminated slot types */
 	if (drc_types)
-		/* &drc_types[1] contains NULL terminated slot types */
 		*drc_types = types;
 	if (drc_power_domains)
 		*drc_power_domains = domains;

@@ -210,15 +208,13 @@ int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
 	int i, rc;
 
 	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
-	if (!my_index) {
-		/* Node isn't DLPAR/hotplug capable */
+	/* Node isn't DLPAR/hotplug capable */
+	if (!my_index)
 		return -EINVAL;
-	}
 
 	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
-	if (rc < 0) {
+	if (rc < 0)
 		return -EINVAL;
-	}
 
 	name_tmp = (char *) &names[1];
 	type_tmp = (char *) &types[1];

@@ -244,21 +240,8 @@ int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
 }
 EXPORT_SYMBOL_GPL(rpaphp_get_drc_props);
 
-static int is_php_type(char *drc_type)
-{
-	unsigned long value;
-	char *endptr;
-
-	/* PCI Hotplug nodes have an integer for drc_type */
-	value = simple_strtoul(drc_type, &endptr, 10);
-	if (endptr == drc_type)
-		return 0;
-
-	return 1;
-}
-
 /**
- * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
+ * is_php_dn() - return true if this is a hotpluggable pci slot, else false
  * @dn: target &device_node
  * @indexes: passed to get_children_props()
  * @names: passed to get_children_props()

@@ -270,21 +253,28 @@ static int is_php_type(char *drc_type)
  * for built-in pci slots (even when the built-in slots are
  * dlparable.)
  */
-static int is_php_dn(struct device_node *dn, const int **indexes,
-		const int **names, const int **types, const int **power_domains)
+static bool is_php_dn(struct device_node *dn,
+		      const int **indexes, const int **names,
+		      const int **types, const int **power_domains)
 {
 	const int *drc_types;
+	const char *drc_type_str;
+	char *endptr;
+	unsigned long val;
 	int rc;
 
 	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
 	if (rc < 0)
-		return 0;
+		return false;
 
-	if (!is_php_type((char *) &drc_types[1]))
-		return 0;
+	/* PCI Hotplug nodes have an integer for drc_type */
+	drc_type_str = (char *)&drc_types[1];
+	val = simple_strtoul(drc_type_str, &endptr, 10);
+	if (endptr == drc_type_str)
+		return false;
 
 	*types = drc_types;
-	return 1;
+	return true;
 }
 
 /**

@@ -370,7 +360,6 @@ static void __exit cleanup_slots(void)
 		list_del(&slot->rpaphp_slot_list);
 		pci_hp_deregister(slot->hotplug_slot);
 	}
-	return;
 }
 
 static int __init rpaphp_init(void)

diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
index 9243f3e7..a4aa65c 100644
--- a/drivers/pci/hotplug/rpaphp_pci.c
+++ b/drivers/pci/hotplug/rpaphp_pci.c

@@ -38,30 +38,31 @@ int rpaphp_get_sensor_state(struct slot *slot, int *state)
 	int setlevel;
 
 	rc = rtas_get_sensor(DR_ENTITY_SENSE, slot->index, state);
+	if (rc >= 0)
+		return rc;
+	if (rc != -EFAULT && rc != -EEXIST) {
+		err("%s: Failure %d getting sensor state on slot[%s]\n",
+		    __func__, rc, slot->name);
+		return rc;
+	}
 
+
+	/*
+	 * Some slots have to be powered up before
+	 * get-sensor will succeed
+	 */
+	dbg("%s: Slot[%s] must be power up to get sensor-state\n",
+	    __func__, slot->name);
+	rc = rtas_set_power_level(slot->power_domain, POWER_ON,
+				  &setlevel);
 	if (rc < 0) {
-		if (rc == -EFAULT || rc == -EEXIST) {
-			dbg("%s: slot must be power up to get sensor-state\n",
-			    __func__);
-
-			/* some slots have to be powered up
-			 * before get-sensor will succeed.
-			 */
-			rc = rtas_set_power_level(slot->power_domain, POWER_ON,
-						  &setlevel);
-			if (rc < 0) {
-				dbg("%s: power on slot[%s] failed rc=%d.\n",
-				    __func__, slot->name, rc);
-			} else {
-				rc = rtas_get_sensor(DR_ENTITY_SENSE,
-						     slot->index, state);
-			}
-		} else if (rc == -ENODEV)
-			info("%s: slot is unusable\n", __func__);
-		else
-			err("%s failed to get sensor state\n", __func__);
+		dbg("%s: Failure %d powerng on slot[%s]\n",
+		    __func__, rc, slot->name);
+		return rc;
 	}
-	return rc;
+
+	return rtas_get_sensor(DR_ENTITY_SENSE,
+			       slot->index, state);
 }
 
 /**

diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index a6082cc..be48e69 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c

@@ -72,7 +72,7 @@ struct slot *alloc_slot_struct(struct device_node *dn,
 	slot->hotplug_slot->ops = &rpaphp_hotplug_slot_ops;
 	slot->hotplug_slot->release = &rpaphp_release_slot;
 
-	return (slot);
+	return slot;
 
 error_info:
 	kfree(slot->hotplug_slot->info);

@@ -84,17 +84,6 @@ error_nomem:
 	return NULL;
 }
 
-static int is_registered(struct slot *slot)
-{
-	struct slot *tmp_slot;
-
-	list_for_each_entry(tmp_slot, &rpaphp_slot_head, rpaphp_slot_list) {
-		if (!strcmp(tmp_slot->name, slot->name))
-			return 1;
-	}
-	return 0;
-}
-
 int rpaphp_deregister_slot(struct slot *slot)
 {
 	int retval = 0;

@@ -117,6 +106,7 @@ EXPORT_SYMBOL_GPL(rpaphp_deregister_slot);
 int rpaphp_register_slot(struct slot *slot)
 {
 	struct hotplug_slot *php_slot = slot->hotplug_slot;
+	struct slot *tmp;
 	int retval;
 	int slotno;
 

@@ -124,10 +114,13 @@ int rpaphp_register_slot(struct slot *slot)
 		__func__, slot->dn->full_name, slot->index, slot->name,
 		slot->power_domain, slot->type);
 
-	/* should not try to register the same slot twice */
-	if (is_registered(slot)) {
-		err("rpaphp_register_slot: slot[%s] is already registered\n", slot->name);
-		return -EAGAIN;
+	/* Should not try to register the same slot twice */
+	list_for_each_entry(tmp, &rpaphp_slot_head, rpaphp_slot_list) {
+		if (!strcmp(tmp->name, slot->name)) {
+			err("%s: Slot[%s] is already registered\n",
+			    __func__, slot->name);
+			return -EAGAIN;
+		}
 	}
 
 	if (slot->dn->child)