[RFC PATCH v0 0/2] ibm,dynamic-memory-v2 fix

[RFC PATCH v0 0/2] ibm,dynamic-memory-v2 fix

[Bharata B Rao]

Patch 1 fixes a bug that results in unexpected flag bit in
ibm,dynamic-memory-v2 DT property and wrong number of entries
getting created in the same property during hotplug.

Patch 2 can be optionally considered. It renames DRMEM_LMB_RESERVED
to DRMEM_LMB_ISOLATED. May be it could be renamed to even more
appropriate name.

Bharata B Rao (2):
  powerpc,drmem: Fix unexpected flag value in ibm,dynamic-memory-v2
  powerpc,drmem: Rename DRMEM_LMB_RESERVED to DRMEM_LMB_ISOLATED

 arch/powerpc/include/asm/drmem.h                | 14 ++++-----
 arch/powerpc/mm/drmem.c                         |  8 ++---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 40 ++++++++++++-------------
 3 files changed, 31 insertions(+), 31 deletions(-)

-- 
2.7.4

[RFC PATCH v0 1/2] powerpc, drmem: Fix unexpected flag value in ibm, dynamic-memory-v2

[Bharata B Rao]

Memory addtion and removal by count and indexed-count methods
temporarily mark the LMBs that are being added/removed by a special
flag value DRMEM_LMB_RESERVED. Accessing flags value directly at
a few places without proper accessor method is causing two unexpected
side-effects:

- DRMEM_LMB_RESERVED bit is becoming part of the flags word of
  drconf_cell_v2 entries in ibm,dynamic-memory-v2 DT property.
- This results in extra drconf_cell entries in ibm,dynamic-memory-v2.
  For example if 1G memory is added, it leads to one entry for 3 LMBs
  and 1 separate entry for the last LMB. All the 4 LMBs should be
  defined by one entry here.

Fix this by always accessing the flags by its accessor method
drmem_lmb_flags().

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 arch/powerpc/mm/drmem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 916844f..3f18036 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -98,7 +98,7 @@ static void init_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
 	dr_cell->base_addr = cpu_to_be64(lmb->base_addr);
 	dr_cell->drc_index = cpu_to_be32(lmb->drc_index);
 	dr_cell->aa_index = cpu_to_be32(lmb->aa_index);
-	dr_cell->flags = cpu_to_be32(lmb->flags);
+	dr_cell->flags = cpu_to_be32(drmem_lmb_flags(lmb));
 }
 
 static int drmem_update_dt_v2(struct device_node *memory,
@@ -121,7 +121,7 @@ static int drmem_update_dt_v2(struct device_node *memory,
 		}
 
 		if (prev_lmb->aa_index != lmb->aa_index ||
-		    prev_lmb->flags != lmb->flags)
+		    drmem_lmb_flags(prev_lmb) != drmem_lmb_flags(lmb))
 			lmb_sets++;
 
 		prev_lmb = lmb;
@@ -150,7 +150,7 @@ static int drmem_update_dt_v2(struct device_node *memory,
 		}
 
 		if (prev_lmb->aa_index != lmb->aa_index ||
-		    prev_lmb->flags != lmb->flags) {
+		    drmem_lmb_flags(prev_lmb) != drmem_lmb_flags(lmb)) {
 			/* end of one set, start of another */
 			dr_cell->seq_lmbs = cpu_to_be32(seq_lmbs);
 			dr_cell++;
-- 
2.7.4

[RFC PATCH v0 2/2] powerpc, drmem: Rename DRMEM_LMB_RESERVED to DRMEM_LMB_ISOLATED

[Bharata B Rao]

Memory hotplug code uses a temporary LMB flags bit DRMEM_LMB_RESERVED
to mark the LMB which is currently undergoing hotplug or unplug.
It is easy to confuse DRMEM_LMB_RESERVED to mean the LMB is reserved
for which a separate flag bit already exists DRCONF_MEM_RESERVED. Since
both DRMEM_LMB_RESERVED and DRCONF_MEM_RESERVED operate on the same
LMB flags word, rename the former to DRMEM_LMB_ISOLATED.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/drmem.h                | 14 ++++-----
 arch/powerpc/mm/drmem.c                         |  2 +-
 arch/powerpc/platforms/pseries/hotplug-memory.c | 40 ++++++++++++-------------
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
index ce242b9..b3fa3f7 100644
--- a/arch/powerpc/include/asm/drmem.h
+++ b/arch/powerpc/include/asm/drmem.h
@@ -72,21 +72,21 @@ static inline u32 drmem_lmb_size(void)
 	return drmem_info->lmb_size;
 }
 
-#define DRMEM_LMB_RESERVED	0x80000000
+#define DRMEM_LMB_ISOLATED	0x80000000
 
-static inline void drmem_mark_lmb_reserved(struct drmem_lmb *lmb)
+static inline void drmem_mark_lmb_isolated(struct drmem_lmb *lmb)
 {
-	lmb->flags |= DRMEM_LMB_RESERVED;
+	lmb->flags |= DRMEM_LMB_ISOLATED;
 }
 
-static inline void drmem_remove_lmb_reservation(struct drmem_lmb *lmb)
+static inline void drmem_remove_lmb_isolation(struct drmem_lmb *lmb)
 {
-	lmb->flags &= ~DRMEM_LMB_RESERVED;
+	lmb->flags &= ~DRMEM_LMB_ISOLATED;
 }
 
-static inline bool drmem_lmb_reserved(struct drmem_lmb *lmb)
+static inline bool drmem_lmb_isolated(struct drmem_lmb *lmb)
 {
-	return lmb->flags & DRMEM_LMB_RESERVED;
+	return lmb->flags & DRMEM_LMB_ISOLATED;
 }
 
 u64 drmem_lmb_memory_max(void);
diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 3f18036..652bf3a 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -35,7 +35,7 @@ static u32 drmem_lmb_flags(struct drmem_lmb *lmb)
 	 * Return the value of the lmb flags field minus the reserved
 	 * bit used internally for hotplug processing.
 	 */
-	return lmb->flags & ~DRMEM_LMB_RESERVED;
+	return lmb->flags & ~DRMEM_LMB_ISOLATED;
 }
 
 static struct property *clone_property(struct property *prop, u32 prop_sz)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index c1578f5..2f5ca29 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -467,7 +467,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 		/* Mark this lmb so we can add it later if all of the
 		 * requested LMBs cannot be removed.
 		 */
-		drmem_mark_lmb_reserved(lmb);
+		drmem_mark_lmb_isolated(lmb);
 
 		lmbs_removed++;
 		if (lmbs_removed == lmbs_to_remove)
@@ -478,7 +478,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 		pr_err("Memory hot-remove failed, adding LMB's back\n");
 
 		for_each_drmem_lmb(lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			rc = dlpar_add_lmb(lmb);
@@ -486,20 +486,20 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 				pr_err("Failed to add LMB back, drc index %x\n",
 				       lmb->drc_index);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 
 		rc = -EINVAL;
 	} else {
 		for_each_drmem_lmb(lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			dlpar_release_drc(lmb->drc_index);
 			pr_info("Memory at %llx was hot-removed\n",
 				lmb->base_addr);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 		rc = 0;
 	}
@@ -608,7 +608,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 		if (rc)
 			break;
 
-		drmem_mark_lmb_reserved(lmb);
+		drmem_mark_lmb_isolated(lmb);
 	}
 
 	if (rc) {
@@ -616,7 +616,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 
 
 		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			rc = dlpar_add_lmb(lmb);
@@ -624,19 +624,19 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 				pr_err("Failed to add LMB, drc index %x\n",
 				       lmb->drc_index);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 		rc = -EINVAL;
 	} else {
 		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			dlpar_release_drc(lmb->drc_index);
 			pr_info("Memory at %llx (drc index %x) was hot-removed\n",
 				lmb->base_addr, lmb->drc_index);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 	}
 
@@ -760,7 +760,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
 		/* Mark this lmb so we can remove it later if all of the
 		 * requested LMBs cannot be added.
 		 */
-		drmem_mark_lmb_reserved(lmb);
+		drmem_mark_lmb_isolated(lmb);
 
 		lmbs_added++;
 		if (lmbs_added == lmbs_to_add)
@@ -771,7 +771,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
 		pr_err("Memory hot-add failed, removing any added LMBs\n");
 
 		for_each_drmem_lmb(lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			rc = dlpar_remove_lmb(lmb);
@@ -781,17 +781,17 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
 			else
 				dlpar_release_drc(lmb->drc_index);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 		rc = -EINVAL;
 	} else {
 		for_each_drmem_lmb(lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			pr_info("Memory at %llx (drc index %x) was hot-added\n",
 				lmb->base_addr, lmb->drc_index);
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 		rc = 0;
 	}
@@ -874,14 +874,14 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
 			break;
 		}
 
-		drmem_mark_lmb_reserved(lmb);
+		drmem_mark_lmb_isolated(lmb);
 	}
 
 	if (rc) {
 		pr_err("Memory indexed-count-add failed, removing any added LMBs\n");
 
 		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			rc = dlpar_remove_lmb(lmb);
@@ -891,17 +891,17 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
 			else
 				dlpar_release_drc(lmb->drc_index);
 
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 		rc = -EINVAL;
 	} else {
 		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
-			if (!drmem_lmb_reserved(lmb))
+			if (!drmem_lmb_isolated(lmb))
 				continue;
 
 			pr_info("Memory at %llx (drc index %x) was hot-added\n",
 				lmb->base_addr, lmb->drc_index);
-			drmem_remove_lmb_reservation(lmb);
+			drmem_remove_lmb_isolation(lmb);
 		}
 	}
 
-- 
2.7.4

Re: [RFC PATCH v0 1/2] powerpc, drmem: Fix unexpected flag value in ibm, dynamic-memory-v2

[Michael Ellerman]

Bharata B Rao <bharata@linux.vnet.ibm.com> writes:

> Memory addtion and removal by count and indexed-count methods
> temporarily mark the LMBs that are being added/removed by a special
> flag value DRMEM_LMB_RESERVED. Accessing flags value directly at
> a few places without proper accessor method is causing two unexpected
> side-effects:
>
> - DRMEM_LMB_RESERVED bit is becoming part of the flags word of
>   drconf_cell_v2 entries in ibm,dynamic-memory-v2 DT property.
> - This results in extra drconf_cell entries in ibm,dynamic-memory-v2.
>   For example if 1G memory is added, it leads to one entry for 3 LMBs
>   and 1 separate entry for the last LMB. All the 4 LMBs should be
>   defined by one entry here.
>
> Fix this by always accessing the flags by its accessor method
> drmem_lmb_flags().
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Presumably:

  Fixes: 2b31e3aec1db ("powerpc/drmem: Add support for ibm, dynamic-memory-v2 property")

?

cheers

Re: [RFC PATCH v0 0/2] ibm,dynamic-memory-v2 fix

[Balbir Singh]

On Wed, Feb 21, 2018 at 9:36 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> Patch 1 fixes a bug that results in unexpected flag bit in
> ibm,dynamic-memory-v2 DT property and wrong number of entries
> getting created in the same property during hotplug.
>

Could you please elaborate on what this means? Is there a test case -
how do we reproduce this?

Balbir Singh.

Re: [RFC PATCH v0 1/2] powerpc, drmem: Fix unexpected flag value in ibm, dynamic-memory-v2

[Bharata B Rao]

On Wed, Feb 21, 2018 at 10:48:18PM +1100, Michael Ellerman wrote:
> Bharata B Rao <bharata@linux.vnet.ibm.com> writes:
> 
> > Memory addtion and removal by count and indexed-count methods
> > temporarily mark the LMBs that are being added/removed by a special
> > flag value DRMEM_LMB_RESERVED. Accessing flags value directly at
> > a few places without proper accessor method is causing two unexpected
> > side-effects:
> >
> > - DRMEM_LMB_RESERVED bit is becoming part of the flags word of
> >   drconf_cell_v2 entries in ibm,dynamic-memory-v2 DT property.
> > - This results in extra drconf_cell entries in ibm,dynamic-memory-v2.
> >   For example if 1G memory is added, it leads to one entry for 3 LMBs
> >   and 1 separate entry for the last LMB. All the 4 LMBs should be
> >   defined by one entry here.
> >
> > Fix this by always accessing the flags by its accessor method
> > drmem_lmb_flags().
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Presumably:
> 
>   Fixes: 2b31e3aec1db ("powerpc/drmem: Add support for ibm, dynamic-memory-v2 property")

Yes.

Regards,
Bharata.

Re: [RFC PATCH v0 0/2] ibm,dynamic-memory-v2 fix

[Bharata B Rao]

On Wed, Feb 21, 2018 at 11:27:20PM +1100, Balbir Singh wrote:
> On Wed, Feb 21, 2018 at 9:36 PM, Bharata B Rao
> <bharata@linux.vnet.ibm.com> wrote:
> > Patch 1 fixes a bug that results in unexpected flag bit in
> > ibm,dynamic-memory-v2 DT property and wrong number of entries
> > getting created in the same property during hotplug.
> >
> 
> Could you please elaborate on what this means?

Described in 1/2 patch.

> Is there a test case -
> how do we reproduce this?

Hotplug some memory and check the entries in ibm,dynamic-memory-v2 and also
check the flags there, you will see DRMEM_LMB_RESERVED set, which
shouldn't be set.

You would need QEMU which supports ibm,dynamic-memory-v2. I have posted
the initial patches for that at:

http://lists.gnu.org/archive/html/qemu-ppc/2018-02/msg00236.html

Regards,
Bharata.

Re: [RFC PATCH v0 1/2] powerpc, drmem: Fix unexpected flag value in ibm, dynamic-memory-v2

[Nathan Fontenot]

On 02/21/2018 04:36 AM, Bharata B Rao wrote:
> Memory addtion and removal by count and indexed-count methods
> temporarily mark the LMBs that are being added/removed by a special
> flag value DRMEM_LMB_RESERVED. Accessing flags value directly at
> a few places without proper accessor method is causing two unexpected
> side-effects:
> 
> - DRMEM_LMB_RESERVED bit is becoming part of the flags word of
>   drconf_cell_v2 entries in ibm,dynamic-memory-v2 DT property.
> - This results in extra drconf_cell entries in ibm,dynamic-memory-v2.
>   For example if 1G memory is added, it leads to one entry for 3 LMBs
>   and 1 separate entry for the last LMB. All the 4 LMBs should be
>   defined by one entry here.
> 
> Fix this by always accessing the flags by its accessor method
> drmem_lmb_flags().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

> ---
>  arch/powerpc/mm/drmem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 916844f..3f18036 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -98,7 +98,7 @@ static void init_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>  	dr_cell->base_addr = cpu_to_be64(lmb->base_addr);
>  	dr_cell->drc_index = cpu_to_be32(lmb->drc_index);
>  	dr_cell->aa_index = cpu_to_be32(lmb->aa_index);
> -	dr_cell->flags = cpu_to_be32(lmb->flags);
> +	dr_cell->flags = cpu_to_be32(drmem_lmb_flags(lmb));
>  }
> 
>  static int drmem_update_dt_v2(struct device_node *memory,
> @@ -121,7 +121,7 @@ static int drmem_update_dt_v2(struct device_node *memory,
>  		}
> 
>  		if (prev_lmb->aa_index != lmb->aa_index ||
> -		    prev_lmb->flags != lmb->flags)
> +		    drmem_lmb_flags(prev_lmb) != drmem_lmb_flags(lmb))
>  			lmb_sets++;
> 
>  		prev_lmb = lmb;
> @@ -150,7 +150,7 @@ static int drmem_update_dt_v2(struct device_node *memory,
>  		}
> 
>  		if (prev_lmb->aa_index != lmb->aa_index ||
> -		    prev_lmb->flags != lmb->flags) {
> +		    drmem_lmb_flags(prev_lmb) != drmem_lmb_flags(lmb)) {
>  			/* end of one set, start of another */
>  			dr_cell->seq_lmbs = cpu_to_be32(seq_lmbs);
>  			dr_cell++;
> 

Re: [RFC PATCH v0 2/2] powerpc, drmem: Rename DRMEM_LMB_RESERVED to DRMEM_LMB_ISOLATED

[Nathan Fontenot]

On 02/21/2018 04:36 AM, Bharata B Rao wrote:
> Memory hotplug code uses a temporary LMB flags bit DRMEM_LMB_RESERVED
> to mark the LMB which is currently undergoing hotplug or unplug.
> It is easy to confuse DRMEM_LMB_RESERVED to mean the LMB is reserved
> for which a separate flag bit already exists DRCONF_MEM_RESERVED. Since
> both DRMEM_LMB_RESERVED and DRCONF_MEM_RESERVED operate on the same
> LMB flags word, rename the former to DRMEM_LMB_ISOLATED.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

> ---
>  arch/powerpc/include/asm/drmem.h                | 14 ++++-----
>  arch/powerpc/mm/drmem.c                         |  2 +-
>  arch/powerpc/platforms/pseries/hotplug-memory.c | 40 ++++++++++++-------------
>  3 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index ce242b9..b3fa3f7 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -72,21 +72,21 @@ static inline u32 drmem_lmb_size(void)
>  	return drmem_info->lmb_size;
>  }
> 
> -#define DRMEM_LMB_RESERVED	0x80000000
> +#define DRMEM_LMB_ISOLATED	0x80000000
> 
> -static inline void drmem_mark_lmb_reserved(struct drmem_lmb *lmb)
> +static inline void drmem_mark_lmb_isolated(struct drmem_lmb *lmb)
>  {
> -	lmb->flags |= DRMEM_LMB_RESERVED;
> +	lmb->flags |= DRMEM_LMB_ISOLATED;
>  }
> 
> -static inline void drmem_remove_lmb_reservation(struct drmem_lmb *lmb)
> +static inline void drmem_remove_lmb_isolation(struct drmem_lmb *lmb)
>  {
> -	lmb->flags &= ~DRMEM_LMB_RESERVED;
> +	lmb->flags &= ~DRMEM_LMB_ISOLATED;
>  }
> 
> -static inline bool drmem_lmb_reserved(struct drmem_lmb *lmb)
> +static inline bool drmem_lmb_isolated(struct drmem_lmb *lmb)
>  {
> -	return lmb->flags & DRMEM_LMB_RESERVED;
> +	return lmb->flags & DRMEM_LMB_ISOLATED;
>  }
> 
>  u64 drmem_lmb_memory_max(void);
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 3f18036..652bf3a 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -35,7 +35,7 @@ static u32 drmem_lmb_flags(struct drmem_lmb *lmb)
>  	 * Return the value of the lmb flags field minus the reserved
>  	 * bit used internally for hotplug processing.
>  	 */
> -	return lmb->flags & ~DRMEM_LMB_RESERVED;
> +	return lmb->flags & ~DRMEM_LMB_ISOLATED;
>  }
> 
>  static struct property *clone_property(struct property *prop, u32 prop_sz)
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c1578f5..2f5ca29 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -467,7 +467,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  		/* Mark this lmb so we can add it later if all of the
>  		 * requested LMBs cannot be removed.
>  		 */
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
> 
>  		lmbs_removed++;
>  		if (lmbs_removed == lmbs_to_remove)
> @@ -478,7 +478,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  		pr_err("Memory hot-remove failed, adding LMB's back\n");
> 
>  		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			rc = dlpar_add_lmb(lmb);
> @@ -486,20 +486,20 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  				pr_err("Failed to add LMB back, drc index %x\n",
>  				       lmb->drc_index);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
> 
>  		rc = -EINVAL;
>  	} else {
>  		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			dlpar_release_drc(lmb->drc_index);
>  			pr_info("Memory at %llx was hot-removed\n",
>  				lmb->base_addr);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  		rc = 0;
>  	}
> @@ -608,7 +608,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>  		if (rc)
>  			break;
> 
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>  	}
> 
>  	if (rc) {
> @@ -616,7 +616,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
> 
> 
>  		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			rc = dlpar_add_lmb(lmb);
> @@ -624,19 +624,19 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>  				pr_err("Failed to add LMB, drc index %x\n",
>  				       lmb->drc_index);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  		rc = -EINVAL;
>  	} else {
>  		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			dlpar_release_drc(lmb->drc_index);
>  			pr_info("Memory at %llx (drc index %x) was hot-removed\n",
>  				lmb->base_addr, lmb->drc_index);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  	}
> 
> @@ -760,7 +760,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>  		/* Mark this lmb so we can remove it later if all of the
>  		 * requested LMBs cannot be added.
>  		 */
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
> 
>  		lmbs_added++;
>  		if (lmbs_added == lmbs_to_add)
> @@ -771,7 +771,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>  		pr_err("Memory hot-add failed, removing any added LMBs\n");
> 
>  		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			rc = dlpar_remove_lmb(lmb);
> @@ -781,17 +781,17 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>  			else
>  				dlpar_release_drc(lmb->drc_index);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  		rc = -EINVAL;
>  	} else {
>  		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			pr_info("Memory at %llx (drc index %x) was hot-added\n",
>  				lmb->base_addr, lmb->drc_index);
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  		rc = 0;
>  	}
> @@ -874,14 +874,14 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
>  			break;
>  		}
> 
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>  	}
> 
>  	if (rc) {
>  		pr_err("Memory indexed-count-add failed, removing any added LMBs\n");
> 
>  		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			rc = dlpar_remove_lmb(lmb);
> @@ -891,17 +891,17 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
>  			else
>  				dlpar_release_drc(lmb->drc_index);
> 
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  		rc = -EINVAL;
>  	} else {
>  		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>  				continue;
> 
>  			pr_info("Memory at %llx (drc index %x) was hot-added\n",
>  				lmb->base_addr, lmb->drc_index);
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>  		}
>  	}
> 

Re: [RFC, v0, 1/2] powerpc, drmem: Fix unexpected flag value in ibm, dynamic-memory-v2

[Michael Ellerman]

On Wed, 2018-02-21 at 10:36:26 UTC, Bharata B Rao wrote:
> Memory addtion and removal by count and indexed-count methods
> temporarily mark the LMBs that are being added/removed by a special
> flag value DRMEM_LMB_RESERVED. Accessing flags value directly at
> a few places without proper accessor method is causing two unexpected
> side-effects:
> 
> - DRMEM_LMB_RESERVED bit is becoming part of the flags word of
>   drconf_cell_v2 entries in ibm,dynamic-memory-v2 DT property.
> - This results in extra drconf_cell entries in ibm,dynamic-memory-v2.
>   For example if 1G memory is added, it leads to one entry for 3 LMBs
>   and 1 separate entry for the last LMB. All the 4 LMBs should be
>   defined by one entry here.
> 
> Fix this by always accessing the flags by its accessor method
> drmem_lmb_flags().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/2f7d03e0511991f124455682cc9409

cheers

Re: [RFC PATCH v0 2/2] powerpc, drmem: Rename DRMEM_LMB_RESERVED to DRMEM_LMB_ISOLATED

[Christophe Leroy]

Le 21/02/2018 à 11:36, Bharata B Rao a écrit :
> Memory hotplug code uses a temporary LMB flags bit DRMEM_LMB_RESERVED
> to mark the LMB which is currently undergoing hotplug or unplug.
> It is easy to confuse DRMEM_LMB_RESERVED to mean the LMB is reserved
> for which a separate flag bit already exists DRCONF_MEM_RESERVED. Since
> both DRMEM_LMB_RESERVED and DRCONF_MEM_RESERVED operate on the same
> LMB flags word, rename the former to DRMEM_LMB_ISOLATED.

Nothing has happened since this RFC was posted, we still have it as 
'new' in patchwork and it doesn't apply.

I'll flag it as 'RFC' so we keep it in our RFC history.

Christophe

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>   arch/powerpc/include/asm/drmem.h                | 14 ++++-----
>   arch/powerpc/mm/drmem.c                         |  2 +-
>   arch/powerpc/platforms/pseries/hotplug-memory.c | 40 ++++++++++++-------------
>   3 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index ce242b9..b3fa3f7 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -72,21 +72,21 @@ static inline u32 drmem_lmb_size(void)
>   	return drmem_info->lmb_size;
>   }
>   
> -#define DRMEM_LMB_RESERVED	0x80000000
> +#define DRMEM_LMB_ISOLATED	0x80000000
>   
> -static inline void drmem_mark_lmb_reserved(struct drmem_lmb *lmb)
> +static inline void drmem_mark_lmb_isolated(struct drmem_lmb *lmb)
>   {
> -	lmb->flags |= DRMEM_LMB_RESERVED;
> +	lmb->flags |= DRMEM_LMB_ISOLATED;
>   }
>   
> -static inline void drmem_remove_lmb_reservation(struct drmem_lmb *lmb)
> +static inline void drmem_remove_lmb_isolation(struct drmem_lmb *lmb)
>   {
> -	lmb->flags &= ~DRMEM_LMB_RESERVED;
> +	lmb->flags &= ~DRMEM_LMB_ISOLATED;
>   }
>   
> -static inline bool drmem_lmb_reserved(struct drmem_lmb *lmb)
> +static inline bool drmem_lmb_isolated(struct drmem_lmb *lmb)
>   {
> -	return lmb->flags & DRMEM_LMB_RESERVED;
> +	return lmb->flags & DRMEM_LMB_ISOLATED;
>   }
>   
>   u64 drmem_lmb_memory_max(void);
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 3f18036..652bf3a 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -35,7 +35,7 @@ static u32 drmem_lmb_flags(struct drmem_lmb *lmb)
>   	 * Return the value of the lmb flags field minus the reserved
>   	 * bit used internally for hotplug processing.
>   	 */
> -	return lmb->flags & ~DRMEM_LMB_RESERVED;
> +	return lmb->flags & ~DRMEM_LMB_ISOLATED;
>   }
>   
>   static struct property *clone_property(struct property *prop, u32 prop_sz)
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c1578f5..2f5ca29 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -467,7 +467,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>   		/* Mark this lmb so we can add it later if all of the
>   		 * requested LMBs cannot be removed.
>   		 */
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>   
>   		lmbs_removed++;
>   		if (lmbs_removed == lmbs_to_remove)
> @@ -478,7 +478,7 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>   		pr_err("Memory hot-remove failed, adding LMB's back\n");
>   
>   		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			rc = dlpar_add_lmb(lmb);
> @@ -486,20 +486,20 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>   				pr_err("Failed to add LMB back, drc index %x\n",
>   				       lmb->drc_index);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   
>   		rc = -EINVAL;
>   	} else {
>   		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			dlpar_release_drc(lmb->drc_index);
>   			pr_info("Memory at %llx was hot-removed\n",
>   				lmb->base_addr);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   		rc = 0;
>   	}
> @@ -608,7 +608,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>   		if (rc)
>   			break;
>   
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>   	}
>   
>   	if (rc) {
> @@ -616,7 +616,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>   
>   
>   		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			rc = dlpar_add_lmb(lmb);
> @@ -624,19 +624,19 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>   				pr_err("Failed to add LMB, drc index %x\n",
>   				       lmb->drc_index);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   		rc = -EINVAL;
>   	} else {
>   		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			dlpar_release_drc(lmb->drc_index);
>   			pr_info("Memory at %llx (drc index %x) was hot-removed\n",
>   				lmb->base_addr, lmb->drc_index);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   	}
>   
> @@ -760,7 +760,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>   		/* Mark this lmb so we can remove it later if all of the
>   		 * requested LMBs cannot be added.
>   		 */
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>   
>   		lmbs_added++;
>   		if (lmbs_added == lmbs_to_add)
> @@ -771,7 +771,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>   		pr_err("Memory hot-add failed, removing any added LMBs\n");
>   
>   		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			rc = dlpar_remove_lmb(lmb);
> @@ -781,17 +781,17 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
>   			else
>   				dlpar_release_drc(lmb->drc_index);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   		rc = -EINVAL;
>   	} else {
>   		for_each_drmem_lmb(lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			pr_info("Memory at %llx (drc index %x) was hot-added\n",
>   				lmb->base_addr, lmb->drc_index);
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   		rc = 0;
>   	}
> @@ -874,14 +874,14 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
>   			break;
>   		}
>   
> -		drmem_mark_lmb_reserved(lmb);
> +		drmem_mark_lmb_isolated(lmb);
>   	}
>   
>   	if (rc) {
>   		pr_err("Memory indexed-count-add failed, removing any added LMBs\n");
>   
>   		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			rc = dlpar_remove_lmb(lmb);
> @@ -891,17 +891,17 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
>   			else
>   				dlpar_release_drc(lmb->drc_index);
>   
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   		rc = -EINVAL;
>   	} else {
>   		for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -			if (!drmem_lmb_reserved(lmb))
> +			if (!drmem_lmb_isolated(lmb))
>   				continue;
>   
>   			pr_info("Memory at %llx (drc index %x) was hot-added\n",
>   				lmb->base_addr, lmb->drc_index);
> -			drmem_remove_lmb_reservation(lmb);
> +			drmem_remove_lmb_isolation(lmb);
>   		}
>   	}
>