On 05/22/2018 11:37 AM, Michael Bringmann wrote: > This patch applies a common parse function for the ibm,drc-info > property that can be modified by a callback function to the > hot-add CPU code. Candidate code is replaced by a call to the > parser including a pointer to a local context-specific functions, > and local data. > > In addition, a bug in the release of the previous patch set may > break things in some of the CPU DLPAR operations. For instance, > when attempting to hot-add a new CPU or set of CPUs, the original > patch failed to always properly calculate the available resources, > and aborted the operation. > > Signed-off-by: Michael Bringmann <m...@linux.vnet.ibm.com> > Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info > firmwar > e feature" -- end of patch series applied to powerpc next) > --- > Changes in V4: > -- Update code to account for latest kernel checkins. > -- Rebased to 4.17-rc5 kernel > -- Compress some more code > --- > arch/powerpc/platforms/pseries/hotplug-cpu.c | 118 > +++++++++++++++++------ > arch/powerpc/platforms/pseries/pseries_energy.c | 107 +++++++++++---------- > 2 files changed, 141 insertions(+), 84 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c > b/arch/powerpc/platforms/pseries/hotplug-cpu.c > index 6ef77ca..ceacad9 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c > @@ -411,27 +411,63 @@ static bool dlpar_cpu_exists(struct device_node > *parent, u32 drc_index) > return found; > } > > -static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index) > +static bool check_cpu_drc_index(struct device_node *parent, > + int (*cb)(struct of_drc_info *drc, > + void *data, void *not_used, > + int *ret_code), > + void *cdata) > { > bool found = false; > - int rc, index; > > - index = 0; > - while (!found) { > - u32 drc; > + if (firmware_has_feature(FW_FEATURE_DRC_INFO)) { > + if (drc_info_parser(parent, cb, "CPU", cdata)) > + found = true; > + } else { > + int index = 0; > > - rc = of_property_read_u32_index(parent, "ibm,drc-indexes", > - index++, &drc); > - if (rc) > - break; > + while (!found) { > + u32 drc; > > - if (drc == drc_index) > - found = true; > + if (of_property_read_u32_index(parent, > + "ibm,drc-indexes", > + index++, &drc)) > + break; > + if (cb(NULL, cdata, &drc, NULL)) > + found = true; > + } > } > > return found; > } > > +struct valid_drc_index_struct { > + u32 targ_drc_index; > +};
Can you help me understand the need to encapsulate the drc_index as a struct. > + > +static int valid_drc_index_cb(struct of_drc_info *drc, void *idata, > + void *drc_index, int *ret_code) > +{ > + struct valid_drc_index_struct *cdata = idata; > + > + if (drc) { > + if (!((drc->drc_index_start <= cdata->targ_drc_index) && > + (cdata->targ_drc_index <= drc->last_drc_index))) > + return 0; > + } else { > + if (*((u32 *)drc_index) != cdata->targ_drc_index) > + return 0; > + } > + (*ret_code) = 1; > + return 1; > +} > + > +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index) > +{ > + struct valid_drc_index_struct cdata = { drc_index }; > + > + return check_cpu_drc_index(parent, valid_drc_index_cb, &cdata); > +} > + > static ssize_t dlpar_cpu_add(u32 drc_index) > { > struct device_node *dn, *parent; > @@ -721,11 +757,43 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove) > return rc; > } > > +struct cpus_to_add_struct { > + struct device_node *parent; > + u32 *cpu_drcs; > + u32 cpus_to_add; > + u32 cpus_found; > +}; > + > +static int cpus_to_add_cb(struct of_drc_info *drc, void *idata, > + void *drc_index, int *ret_code) > +{ > + struct cpus_to_add_struct *cdata = idata; > + > + if (drc) { > + int k; > + > + for (k = 0; (k < drc->num_sequential_elems) && > + (cdata->cpus_found < cdata->cpus_to_add); k++) { > + u32 idrc = drc->drc_index_start + > + (k * drc->sequential_inc); > + > + if (dlpar_cpu_exists(cdata->parent, idrc)) > + continue; > + cdata->cpu_drcs[cdata->cpus_found++] = idrc; > + } > + } else { > + if (!dlpar_cpu_exists(cdata->parent, *((u32 *)drc_index))) > + cdata->cpu_drcs[cdata->cpus_found++] = > + *((u32 *)drc_index); > + } > + return 0; > +} > + > static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add) > { > struct device_node *parent; > - int cpus_found = 0; > - int index, rc; > + struct cpus_to_add_struct cdata = { > + NULL, cpu_drcs, cpus_to_add, 0 }; > > parent = of_find_node_by_path("/cpus"); > if (!parent) { > @@ -734,28 +802,14 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 > cpus_to_add) > return -1; > } > > - /* Search the ibm,drc-indexes array for possible CPU drcs to > - * add. Note that the format of the ibm,drc-indexes array is > - * the number of entries in the array followed by the array > - * of drc values so we start looking at index = 1. > + /* Search the appropriate property for possible CPU drcs to > + * add. > */ > - index = 1; > - while (cpus_found < cpus_to_add) { > - u32 drc; > - > - rc = of_property_read_u32_index(parent, "ibm,drc-indexes", > - index++, &drc); > - if (rc) > - break; > - > - if (dlpar_cpu_exists(parent, drc)) > - continue; > - > - cpu_drcs[cpus_found++] = drc; > - } > + cdata.parent = parent; > + check_cpu_drc_index(parent, cpus_to_add_cb, &cdata); > > of_node_put(parent); > - return cpus_found; > + return cdata.cpus_found; > } > > static int dlpar_cpu_add_by_count(u32 cpus_to_add) > diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c > b/arch/powerpc/platforms/pseries/pseries_energy.c > index 5261975..d8d7750 100644 > --- a/arch/powerpc/platforms/pseries/pseries_energy.c > +++ b/arch/powerpc/platforms/pseries/pseries_energy.c > @@ -36,6 +36,26 @@ > > /* Helper Routines to convert between drc_index to cpu numbers */ > > +struct cpu_to_drc_index_struct { > + u32 thread_index; > + u32 ret; > +}; > + > +static int cpu_to_drc_index_cb(struct of_drc_info *drc, void *idata, > + void *not_used, int *ret_code) > +{ > + struct cpu_to_drc_index_struct *cdata = idata; > + int ret = 0; > + > + if (cdata->thread_index < drc->last_drc_index) { Is this correct? You're comparing a thread index to a drc index. > + cdata->ret = drc->drc_index_start + > + (cdata->thread_index * drc->sequential_inc); > + ret = 1; > + } > + (*ret_code) = ret; > + return ret; > +} > + > static u32 cpu_to_drc_index(int cpu) > { > struct device_node *dn = NULL; > @@ -51,30 +71,14 @@ static u32 cpu_to_drc_index(int cpu) > thread_index = cpu_core_index_of_thread(cpu); > > if (firmware_has_feature(FW_FEATURE_DRC_INFO)) { > - struct property *info = NULL; > - struct of_drc_info drc; > - int j; > - u32 num_set_entries; > - const __be32 *value; > - > - info = of_find_property(dn, "ibm,drc-info", NULL); > - if (info == NULL) > - goto err_of_node_put; > + struct cpu_to_drc_index_struct cdata = { > + thread_index, 0 }; > > - value = info->value; > - num_set_entries = of_read_number(value++, 1); > - > - for (j = 0; j < num_set_entries; j++) { > - > - of_read_drc_info_cell(&info, &value, &drc); > - if (strncmp(drc.drc_type, "CPU", 3)) > - goto err; > - > - if (thread_index < drc.last_drc_index) > - break; > - } > - > - ret = drc.drc_index_start + (thread_index * drc.sequential_inc); > + rc = drc_info_parser(dn, &cpu_to_drc_index_cb, > + "CPU", &cdata); > + if (rc < 0) > + goto err_of_node_put; > + ret = cdata.ret; > } else { > const __be32 *indexes; > > @@ -100,11 +104,33 @@ static u32 cpu_to_drc_index(int cpu) > return ret; > } > > +struct drc_index_to_cpu_struct { > + u32 drc_index; > + u32 thread_index; > + u32 cpu; > +}; > + > +static int drc_index_to_cpu_cb(struct of_drc_info *drc, > + void *idata, void *not_used, int *ret_code) > +{ > + struct drc_index_to_cpu_struct *cdata = idata; > + > + if (cdata->drc_index > drc->last_drc_index) { > + cdata->cpu += drc->num_sequential_elems; > + } else { > + cdata->cpu += ((cdata->drc_index - drc->drc_index_start) / > + drc->sequential_inc); > + cdata->thread_index = cpu_first_thread_of_core(cdata->cpu); Should this return 1 here to avoid continuing to walk the drc_info entries? -Nathan > + } > + (*ret_code) = 0; > + return 0; > +} > + > static int drc_index_to_cpu(u32 drc_index) > { > struct device_node *dn = NULL; > const int *indexes; > - int thread_index = 0, cpu = 0; > + int thread_index = 0; > int rc = 1; > > dn = of_find_node_by_path("/cpus"); > @@ -112,36 +138,13 @@ static int drc_index_to_cpu(u32 drc_index) > goto err; > > if (firmware_has_feature(FW_FEATURE_DRC_INFO)) { > - struct property *info = NULL; > - struct of_drc_info drc; > - int j; > - u32 num_set_entries; > - const __be32 *value; > - > - info = of_find_property(dn, "ibm,drc-info", NULL); > - if (info == NULL) > - goto err_of_node_put; > - > - value = info->value; > - num_set_entries = of_read_number(value++, 1); > - > - for (j = 0; j < num_set_entries; j++) { > + struct drc_index_to_cpu_struct cdata = { > + drc_index, 0, 0 }; > > - of_read_drc_info_cell(&info, &value, &drc); > - if (strncmp(drc.drc_type, "CPU", 3)) > - goto err; > + rc = drc_info_parser(dn, &drc_index_to_cpu_cb, > + "CPU", &cdata); > + thread_index = cdata.thread_index; > > - if (drc_index > drc.last_drc_index) { > - cpu += drc.num_sequential_elems; > - continue; > - } > - cpu += ((drc_index - drc.drc_index_start) / > - drc.sequential_inc); > - > - thread_index = cpu_first_thread_of_core(cpu); > - rc = 0; > - break; > - } > } else { > unsigned long int i; >