[PATCH] pseries/hotplug-memory: remove dlpar_memory_{add,remove}_by_index() functions

[PATCH] pseries/hotplug-memory: remove dlpar_memory_{add,remove}_by_index() functions

[Scott Cheloha]

The dlpar_memory_{add,remove}_by_index() functions are just special
cases of their dlpar_memory_{add,remove}_by_ic() counterparts where
the LMB count is 1.  There is no need to maintain separate code.

In addition, there is a NULL dereference bug in the *_by_index()
functions if the LMB in question is not found.  This bug is not
present in their *_by_ic() counterparts.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 .../platforms/pseries/hotplug-memory.c        | 74 +------------------
 1 file changed, 2 insertions(+), 72 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index c126b94d1943..df7854c5c1f2 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -473,38 +473,6 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 	return rc;
 }
 
-static int dlpar_memory_remove_by_index(u32 drc_index)
-{
-	struct drmem_lmb *lmb;
-	int lmb_found;
-	int rc;
-
-	pr_info("Attempting to hot-remove LMB, drc index %x\n", drc_index);
-
-	lmb_found = 0;
-	for_each_drmem_lmb(lmb) {
-		if (lmb->drc_index == drc_index) {
-			lmb_found = 1;
-			rc = dlpar_remove_lmb(lmb);
-			if (!rc)
-				dlpar_release_drc(lmb->drc_index);
-
-			break;
-		}
-	}
-
-	if (!lmb_found)
-		rc = -EINVAL;
-
-	if (rc)
-		pr_info("Failed to hot-remove memory at %llx\n",
-			lmb->base_addr);
-	else
-		pr_info("Memory at %llx was hot-removed\n", lmb->base_addr);
-
-	return rc;
-}
-
 static int dlpar_memory_readd_by_index(u32 drc_index)
 {
 	struct drmem_lmb *lmb;
@@ -631,10 +599,6 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 {
 	return -EOPNOTSUPP;
 }
-static int dlpar_memory_remove_by_index(u32 drc_index)
-{
-	return -EOPNOTSUPP;
-}
 static int dlpar_memory_readd_by_index(u32 drc_index)
 {
 	return -EOPNOTSUPP;
@@ -762,40 +726,6 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
 	return rc;
 }
 
-static int dlpar_memory_add_by_index(u32 drc_index)
-{
-	struct drmem_lmb *lmb;
-	int rc, lmb_found;
-
-	pr_info("Attempting to hot-add LMB, drc index %x\n", drc_index);
-
-	lmb_found = 0;
-	for_each_drmem_lmb(lmb) {
-		if (lmb->drc_index == drc_index) {
-			lmb_found = 1;
-			rc = dlpar_acquire_drc(lmb->drc_index);
-			if (!rc) {
-				rc = dlpar_add_lmb(lmb);
-				if (rc)
-					dlpar_release_drc(lmb->drc_index);
-			}
-
-			break;
-		}
-	}
-
-	if (!lmb_found)
-		rc = -EINVAL;
-
-	if (rc)
-		pr_info("Failed to hot-add memory, drc index %x\n", drc_index);
-	else
-		pr_info("Memory at %llx (drc index %x) was hot-added\n",
-			lmb->base_addr, drc_index);
-
-	return rc;
-}
-
 static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
 {
 	struct drmem_lmb *lmb, *start_lmb, *end_lmb;
@@ -887,7 +817,7 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_INDEX:
 			drc_index = hp_elog->_drc_u.drc_index;
-			rc = dlpar_memory_add_by_index(drc_index);
+			rc = dlpar_memory_add_by_ic(1, drc_index);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_IC:
 			count = hp_elog->_drc_u.ic.count;
@@ -908,7 +838,7 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_INDEX:
 			drc_index = hp_elog->_drc_u.drc_index;
-			rc = dlpar_memory_remove_by_index(drc_index);
+			rc = dlpar_memory_remove_by_ic(1, drc_index);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_IC:
 			count = hp_elog->_drc_u.ic.count;
-- 
2.24.1

Re: [PATCH] pseries/hotplug-memory: remove dlpar_memory_{add,remove}_by_index() functions

[Nathan Lynch]

Scott Cheloha <cheloha@linux.ibm.com> writes:
> The dlpar_memory_{add,remove}_by_index() functions are just special
> cases of their dlpar_memory_{add,remove}_by_ic() counterparts where
> the LMB count is 1.

I wish that were the case, but there are (gratuitous?) differences:

- dlpar_memory_remove_by_ic() checks DRCONF_MEM_RESERVED and
  DRCONF_MEM_ASSIGNED flags; dlpar_memory_remove_by_index() does not.
- dlpar_memory_remove_by_ic() attempts to roll back failed removal;
  dlpar_memory_remove_by_index() does not.

I'm not sure how much either of these gets used in practice. AFAIK the
usual HMC/drmgr-driven workflow tends to exercise
dlpar_memory_remove_by_count().

I agree this code needs consolidation, but we should proceed a little
carefully because it's likely going to entail changing some user-visible
behaviors.

Re: [PATCH] pseries/hotplug-memory: remove dlpar_memory_{add,remove}_by_index() functions

[Scott Cheloha]

On Mon, Feb 10, 2020 at 02:28:49PM -0600, Nathan Lynch wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > The dlpar_memory_{add,remove}_by_index() functions are just special
> > cases of their dlpar_memory_{add,remove}_by_ic() counterparts where
> > the LMB count is 1.
> 
> I wish that were the case, but there are (gratuitous?) differences:
> 
> - dlpar_memory_remove_by_ic() checks DRCONF_MEM_RESERVED and
>   DRCONF_MEM_ASSIGNED flags; dlpar_memory_remove_by_index() does not.

The lack of a DRCONF_MEM_RESERVED check in add_by_index() and
remove_by_index() might be a mistake.  If I understand the PAPR
correctly, when DRCONF_MEM_RESERVED is set for an LMB the operating
system isn't allowed to touch it.  The LMB could become available for
use later if the platform clears the bit, but if it's set it's
no good.

DRCONF_MEM_ASSIGNED checks are not present in
dlpar_memory_add_by_index() and dlpar_memory_remove_by_index() but
they are done at the top of dlpar_add_lmb() and lmb_is_removable(),
so the checks do happen in those paths.

> - dlpar_memory_remove_by_ic() attempts to roll back failed removal;
>   dlpar_memory_remove_by_index() does not.

The exclusion of rollback in the remove_by_index() path makes sense,
as there are only N=1 possible elements where the operation can fail.
Doing the marking/unmarking for rollback in the N=1 case is harmless
though.  If the removal fails for the given LMB we never call
drmem_mark_reserved() to indicate that we need to re-add it.  The
rollback loop then finds no marked LMBs and does no work.

> I'm not sure how much either of these gets used in practice. AFAIK the
> usual HMC/drmgr-driven workflow tends to exercise
> dlpar_memory_remove_by_count().

drmgr eventually uses dlpar_memory_*_by_count() when you give it a
count of LMBs with the '-q' flag, e.g.:

# drmgr -c mem -a -q 10

drmgr eventually uses dlpar_memory_*_by_index() when you give it a
particular DRC, e.g.:

# drmgr -c mem -a -s <some drc number>

QEMU hotplug uses dlpar_memory_*_by_ic().

> I agree this code needs consolidation, but we should proceed a little
> carefully because it's likely going to entail changing some user-visible
> behaviors.

Sure.

Maybe there are less ambitious ways to start out.