Subject: Re: [PATCH 4/8] PCI/hotplug/rpa: Code cleanup

From: Benjamin Herrenschmidt <[email-masked]>
Date: Wed, 26 Nov 2014 10:02:31 +1100

On Tue, 2014-11-25 at 09:49 +1100, Gavin Shan wrote:
> 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...

Ben.

> 
> Signed-off-by: Gavin Shan <[email-masked]>
> ---
>  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)