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 <gwshan@xxxxxxxxxxxxxxxxxx>
> ---
> 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
