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 <gws...@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)


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to