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

From: Gavin Shan <[email-masked]>
Date: Wed, 26 Nov 2014 11:00:19 +1100

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:
>> 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.
>
>> 
>> 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)
>
>