From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 41Ry6Q0J1TzDr55 for ; Sat, 14 Jul 2018 01:54:20 +1000 (AEST) Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w6DFsA0a076086 for ; Fri, 13 Jul 2018 11:54:18 -0400 Received: from e17.ny.us.ibm.com (e17.ny.us.ibm.com [129.33.205.207]) by mx0a-001b2d01.pphosted.com with ESMTP id 2k6y4q807h-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 13 Jul 2018 11:54:18 -0400 Received: from localhost by e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 13 Jul 2018 11:54:17 -0400 Subject: Re: [PATCH v06 1/9] hotplug/cpu: Conditionally acquire/release DRC index To: "Naveen N. Rao" , linuxppc-dev@lists.ozlabs.org Cc: John Allen , Thomas Falcon , Tyrel Datwyler , Nathan Fontenot References: <1531324062.imrh0yfelc.naveen@linux.ibm.com> From: Michael Bringmann Date: Fri, 13 Jul 2018 10:54:13 -0500 MIME-Version: 1.0 In-Reply-To: <1531324062.imrh0yfelc.naveen@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Message-Id: <3009a3a1-0d67-a9f0-0673-8827c632c46c@linux.vnet.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , See below. Code cleanups accepted for next revision. On 07/11/2018 11:00 AM, Naveen N. Rao wrote: > Michael Bringmann wrote: >> powerpc/cpu: Modify dlpar_cpu_add and dlpar_cpu_remove to allow the >> skipping of DRC index acquire or release operations during the CPU >> add or remove operations.  This is intended to support subsequent >> changes to provide a 'CPU readd' operation. >> >> Signed-off-by: Michael Bringmann >> --- >> Changes in patch: >>   -- Move new validity check added to pseries_smp_notifier >>      to another patch >> --- >>  arch/powerpc/platforms/pseries/hotplug-cpu.c |   68 +++++++++++++++----------- >>  1 file changed, 39 insertions(+), 29 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c >> index 6ef77ca..3632db2 100644 >> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c >> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c >> @@ -432,7 +432,7 @@ static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index) >>      return found; >>  } >> >> -static ssize_t dlpar_cpu_add(u32 drc_index) >> +static ssize_t dlpar_cpu_add(u32 drc_index, bool acquire_drc) >>  { >>      struct device_node *dn, *parent; >>      int rc, saved_rc; >> @@ -457,19 +457,22 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >>          return -EINVAL; >>      } >> >> -    rc = dlpar_acquire_drc(drc_index); >> -    if (rc) { >> -        pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n", >> -            rc, drc_index); >> -        of_node_put(parent); >> -        return -EINVAL; >> +    if (acquire_drc) { >> +        rc = dlpar_acquire_drc(drc_index); >> +        if (rc) { >> +            pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n", >> +                rc, drc_index); >> +            of_node_put(parent); >> +            return -EINVAL; >> +        } >>      } >> >>      dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent); >>      if (!dn) { >>          pr_warn("Failed call to configure-connector, drc index: %x\n", >>              drc_index); >> -        dlpar_release_drc(drc_index); >> +        if (acquire_drc) >> +            dlpar_release_drc(drc_index); >>          of_node_put(parent); >>          return -EINVAL; >>      } >> @@ -484,8 +487,9 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >>          pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n", >>              dn->name, rc, drc_index); >> >> -        rc = dlpar_release_drc(drc_index); >> -        if (!rc) >> +        if (acquire_drc) >> +            rc = dlpar_release_drc(drc_index); >> +        if (!rc || acquire_drc) >>              dlpar_free_cc_nodes(dn); > > (!rc) can only be true if acquire_drc is true, so we seem to be only invoking dlpar_free_cc_nodes() if acquire_drc is true. Would it be better to frame that condition in this manner: > >     if (acquire_drc) { >         rc = dlpar_release_drc(drc_index): >         if (!rc) >             dlpar_free_cc_nodes(dn); >     } > Okay. >> >>          return saved_rc; >> @@ -498,7 +502,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >>              dn->name, rc, drc_index); >> >>          rc = dlpar_detach_node(dn); >> -        if (!rc) >> +        if (!rc && acquire_drc) >>              dlpar_release_drc(drc_index); >> >>          return saved_rc; >> @@ -566,7 +570,8 @@ static int dlpar_offline_cpu(struct device_node *dn) >> >>  } >> >> -static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) >> +static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index, >> +                bool release_drc) >>  { >>      int rc; >> >> @@ -579,12 +584,14 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) >>          return -EINVAL; >>      } >> >> -    rc = dlpar_release_drc(drc_index); >> -    if (rc) { >> -        pr_warn("Failed to release drc (%x) for CPU %s, rc: %d\n", >> -            drc_index, dn->name, rc); >> -        dlpar_online_cpu(dn); >> -        return rc; >> +    if (release_drc) { >> +        rc = dlpar_release_drc(drc_index); >> +        if (rc) { >> +            pr_warn("Failed to release drc (%x) for CPU %s, rc: %d\n", >> +                drc_index, dn->name, rc); >> +            dlpar_online_cpu(dn); >> +            return rc; >> +        } >>      } >> >>      rc = dlpar_detach_node(dn); >> @@ -593,7 +600,10 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) >> >>          pr_warn("Failed to detach CPU %s, rc: %d", dn->name, rc); >> >> -        rc = dlpar_acquire_drc(drc_index); >> +        if (release_drc) >> +            rc = dlpar_acquire_drc(drc_index); >> +        else >> +            rc = 0; >>          if (!rc) >>              dlpar_online_cpu(dn); > > Here, we seem to want to invoke dlpar_online_cpu() unconditionally if release_drc is false. So, to make that explicit, would it be better to frame that as: >     if (release_drc) >         rc = dlpar_acquire_drc(drc_index); >     if (!release_drc || !rc) >         dlpar_online_cpu(dn); > > - Naveen Okay. > >> >> @@ -622,7 +632,7 @@ static struct device_node *cpu_drc_index_to_dn(u32 drc_index) >>      return dn; >>  } >> >> -static int dlpar_cpu_remove_by_index(u32 drc_index) >> +static int dlpar_cpu_remove_by_index(u32 drc_index, bool release_drc) >>  { >>      struct device_node *dn; >>      int rc; >> @@ -634,7 +644,7 @@ static int dlpar_cpu_remove_by_index(u32 drc_index) >>          return -ENODEV; >>      } >> >> -    rc = dlpar_cpu_remove(dn, drc_index); >> +    rc = dlpar_cpu_remove(dn, drc_index, release_drc); >>      of_node_put(dn); >>      return rc; >>  } >> @@ -699,7 +709,7 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove) >>      } >> >>      for (i = 0; i < cpus_to_remove; i++) { >> -        rc = dlpar_cpu_remove_by_index(cpu_drcs[i]); >> +        rc = dlpar_cpu_remove_by_index(cpu_drcs[i], true); >>          if (rc) >>              break; >> >> @@ -710,7 +720,7 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove) >>          pr_warn("CPU hot-remove failed, adding back removed CPUs\n"); >> >>          for (i = 0; i < cpus_removed; i++) >> -            dlpar_cpu_add(cpu_drcs[i]); >> +            dlpar_cpu_add(cpu_drcs[i], true); >> >>          rc = -EINVAL; >>      } else { >> @@ -780,7 +790,7 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add) >>      } >> >>      for (i = 0; i < cpus_to_add; i++) { >> -        rc = dlpar_cpu_add(cpu_drcs[i]); >> +        rc = dlpar_cpu_add(cpu_drcs[i], true); >>          if (rc) >>              break; >> >> @@ -791,7 +801,7 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add) >>          pr_warn("CPU hot-add failed, removing any added CPUs\n"); >> >>          for (i = 0; i < cpus_added; i++) >> -            dlpar_cpu_remove_by_index(cpu_drcs[i]); >> +            dlpar_cpu_remove_by_index(cpu_drcs[i], true); >> >>          rc = -EINVAL; >>      } else { >> @@ -817,7 +827,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog) >>          if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) >>              rc = dlpar_cpu_remove_by_count(count); >>          else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) >> -            rc = dlpar_cpu_remove_by_index(drc_index); >> +            rc = dlpar_cpu_remove_by_index(drc_index, true); >>          else >>              rc = -EINVAL; >>          break; >> @@ -825,7 +835,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog) >>          if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) >>              rc = dlpar_cpu_add_by_count(count); >>          else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) >> -            rc = dlpar_cpu_add(drc_index); >> +            rc = dlpar_cpu_add(drc_index, true); >>          else >>              rc = -EINVAL; >>          break; >> @@ -850,7 +860,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count) >>      if (rc) >>          return -EINVAL; >> >> -    rc = dlpar_cpu_add(drc_index); >> +    rc = dlpar_cpu_add(drc_index, true); >> >>      return rc ? rc : count; >>  } >> @@ -871,7 +881,7 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count) >>          return -EINVAL; >>      } >> >> -    rc = dlpar_cpu_remove(dn, drc_index); >> +    rc = dlpar_cpu_remove(dn, drc_index, true); >>      of_node_put(dn); >> >>      return rc ? rc : count; >> >> > > -- Michael W. Bringmann Linux Technology Center IBM Corporation Tie-Line 363-5196 External: (512) 286-5196 Cell: (512) 466-0650 mwb@linux.vnet.ibm.com