[PATCH 4/8] PCI/hotplug/rpa: Code cleanup
Gavin Shan
gwshan at linux.vnet.ibm.com
Wed Nov 26 11:00:19 AEDT 2014
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 <gwshan at linux.vnet.ibm.com>
>> ---
>> 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)
>
>
More information about the Linuxppc-dev
mailing list