[PATCH v1 1/1] powerpc/papr_scm: Properly handle UUID types and API

Parse to and export from UUID own type, before dereferencing.
This also fixes wrong comment (Little Endian UUID is something else)
and should fix Sparse warnings about assigning strict types to POD.

Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Not tested
 arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ae6f5d80d5ce..4366e1902890 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
 	u32 drc_index, metadata_size;
 	u64 blocks, block_size;
 	struct papr_scm_priv *p;
+	u8 uuid_raw[UUID_SIZE];
 	const char *uuid_str;
-	u64 uuid[2];
+	uuid_t uuid;
 	int rc;
 
 	/* check we have all the required DT properties */

@@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
 	p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
 
 	/* We just need to ensure that set cookies are unique across */
-	uuid_parse(uuid_str, (uuid_t *) uuid);
+	uuid_parse(uuid_str, &uuid);
+
 	/*
 	 * cookie1 and cookie2 are not really little endian
-	 * we store a little endian representation of the
+	 * we store a raw buffer representation of the
 	 * uuid str so that we can compare this with the label
 	 * area cookie irrespective of the endian config with which
 	 * the kernel is built.
 	 */
-	p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
-	p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
+	export_uuid(uuid_raw, &uuid);
+	p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
+	p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);
 
 	/* might be zero */
 	p->metadata_size = metadata_size;

-- 
2.30.2

Thanks for the patch Andy,

Unfortunately ran into a compilation issue due to missing "#include
<asm/unaligned.h>" that provides definition for
get_unaligned_le64(). Gcc reported following error:
 
error: implicit declaration of function ‘get_unaligned_le64’

After including the necessary header file, kernel compiled fine and I
was able to test & verify the patch.

-- 
Cheers
~ Vaibhav

Andy Shevchenko [off-list ref] writes:

On Thu, Apr 15, 2021 at 8:10 PM Vaibhav Jain [off-list ref] wrote:

Thanks for the patch Andy,

Unfortunately ran into a compilation issue due to missing "#include
<asm/unaligned.h>" that provides definition for
get_unaligned_le64(). Gcc reported following error:

error: implicit declaration of function ‘get_unaligned_le64’

Right, I have not tested it (as mentioned in the comments to the patch)

After including the necessary header file, kernel compiled fine and I
was able to test & verify the patch.

Thank you very much for the testing.

I'm not sure what the coverage of your test is. That's why I have an
additional question below. Is the byte ordering kept the same in BE
(32- and 64-bit) cases? Because I'm worrying that I might have missed
something.


-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko [off-list ref] writes:

On Thu, Apr 15, 2021 at 8:10 PM Vaibhav Jain [off-list ref] wrote:

quoted

Thanks for the patch Andy,

Unfortunately ran into a compilation issue due to missing "#include
<asm/unaligned.h>" that provides definition for
get_unaligned_le64(). Gcc reported following error:

error: implicit declaration of function ‘get_unaligned_le64’

Right, I have not tested it (as mentioned in the comments to the patch)

quoted

After including the necessary header file, kernel compiled fine and I
was able to test & verify the patch.

Thank you very much for the testing.

I'm not sure what the coverage of your test is.

Your patch updates the way the interleaved set-cookies are populated in
papr_scm which are then used to populate label entry for a namespace. I
verified that the reported region setcookie hasnt changed for an nvdimm
region before and after applying your patch for both BE and LE variants:

# 64-bit Little endian kernel before applying the patch
$ sudo cat /sys/devices/ndbus0/region0/set_cookie
0x8b6b26cbc930e2b5

# 64-bit Little endian kernel after applying your patch
$ sudo cat /sys/devices/ndbus0/region0/set_cookie
0x8b6b26cbc930e2b5

# 64-bit Big endian kernel before applying your patch
$ sudo cat /sys/devices/ndbus0/region0/set_cookie
0x8b6b26cbc930e2b5

# 64-bit Big endian kernel after applying your patch
$ sudo cat /sys/devices/ndbus0/region0/set_cookie
0x8b6b26cbc930e2b5

That's why I have an
additional question below. Is the byte ordering kept the same in BE
(32- and 64-bit) cases? Because I'm worrying that I might have missed
something.

Libnvdimm store these cookies in label area as little endian values and
based on the results above I think we are good.

-- 
With Best Regards,
Andy Shevchenko

-- 
Cheers
~ Vaibhav

On 4/15/21 7:16 PM, Andy Shevchenko wrote:

Parse to and export from UUID own type, before dereferencing.
This also fixes wrong comment (Little Endian UUID is something else)
and should fix Sparse warnings about assigning strict types to POD.

I am wondering whether this will break older namespace created. IIRC 
that cpu_to_le64 was done to be backward compatible with namespaces 
created before 259a948c4ba1.

What we need to test is create a namespace in little endian kernel and 
read it back in via big endian and vice versa. Also we need to make sure 
we can read the already created namespace before this patch.

On 4/15/21 7:16 PM, Andy Shevchenko wrote:

ok that does the equivalent of cpu_to_le64 there. So we are good. But 
the comment update is missing the details why we did that 
get_unaligned_le64. Maybe raw buffer representation is the correct term?
Should we add an example in the comment. ie,

/*
  * Historically we stored the cookie in the below format.
for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
cookie1 was  0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
*/

  	/* might be zero */
  	p->metadata_size = metadata_size;

On Fri, Apr 16, 2021 at 01:28:21PM +0530, Aneesh Kumar K.V wrote:

On 4/15/21 7:16 PM, Andy Shevchenko wrote:

quoted

Parse to and export from UUID own type, before dereferencing.
This also fixes wrong comment (Little Endian UUID is something else)
and should fix Sparse warnings about assigning strict types to POD.

Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Not tested
  arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ae6f5d80d5ce..4366e1902890 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
  	u32 drc_index, metadata_size;
  	u64 blocks, block_size;
  	struct papr_scm_priv *p;
+	u8 uuid_raw[UUID_SIZE];
  	const char *uuid_str;
-	u64 uuid[2];
+	uuid_t uuid;
  	int rc;
  	/* check we have all the required DT properties */

@@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
  	p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
  	/* We just need to ensure that set cookies are unique across */
-	uuid_parse(uuid_str, (uuid_t *) uuid);
+	uuid_parse(uuid_str, &uuid);
+
  	/*
  	 * cookie1 and cookie2 are not really little endian
-	 * we store a little endian representation of the
+	 * we store a raw buffer representation of the
  	 * uuid str so that we can compare this with the label
  	 * area cookie irrespective of the endian config with which
  	 * the kernel is built.
  	 */
-	p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
-	p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
+	export_uuid(uuid_raw, &uuid);
+	p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
+	p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);

ok that does the equivalent of cpu_to_le64 there. So we are good. But the
comment update is missing the details why we did that get_unaligned_le64.
Maybe raw buffer representation is the correct term?
Should we add an example in the comment. ie,

/*
 * Historically we stored the cookie in the below format.
for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
cookie1 was  0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
*/

I'm fine with the comment. At least it will shed a light on the byte ordering
we are expecting.

quoted

  	/* might be zero */
  	p->metadata_size = metadata_size;

-- 
With Best Regards,
Andy Shevchenko

On 4/16/21 2:39 PM, Andy Shevchenko wrote:

On Fri, Apr 16, 2021 at 01:28:21PM +0530, Aneesh Kumar K.V wrote:

quoted

On 4/15/21 7:16 PM, Andy Shevchenko wrote:

quoted

Parse to and export from UUID own type, before dereferencing.
This also fixes wrong comment (Little Endian UUID is something else)
and should fix Sparse warnings about assigning strict types to POD.

Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Not tested
   arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
   1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ae6f5d80d5ce..4366e1902890 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
   	u32 drc_index, metadata_size;
   	u64 blocks, block_size;
   	struct papr_scm_priv *p;
+	u8 uuid_raw[UUID_SIZE];
   	const char *uuid_str;
-	u64 uuid[2];
+	uuid_t uuid;
   	int rc;
   	/* check we have all the required DT properties */

@@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
   	p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
   	/* We just need to ensure that set cookies are unique across */
-	uuid_parse(uuid_str, (uuid_t *) uuid);
+	uuid_parse(uuid_str, &uuid);
+
   	/*
   	 * cookie1 and cookie2 are not really little endian
-	 * we store a little endian representation of the
+	 * we store a raw buffer representation of the
   	 * uuid str so that we can compare this with the label
   	 * area cookie irrespective of the endian config with which
   	 * the kernel is built.
   	 */
-	p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
-	p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
+	export_uuid(uuid_raw, &uuid);
+	p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
+	p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);

ok that does the equivalent of cpu_to_le64 there. So we are good. But the
comment update is missing the details why we did that get_unaligned_le64.
Maybe raw buffer representation is the correct term?
Should we add an example in the comment. ie,

quoted

/*
  * Historically we stored the cookie in the below format.
for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
cookie1 was  0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
*/

I'm fine with the comment. At least it will shed a light on the byte ordering
we are expecting.

Will you be sending an update? Also it will be good to list the sparse 
warning in the commit message?

-aneesh

On Fri, Apr 16, 2021 at 03:05:31PM +0530, Aneesh Kumar K.V wrote:

On 4/16/21 2:39 PM, Andy Shevchenko wrote:

quoted

On Fri, Apr 16, 2021 at 01:28:21PM +0530, Aneesh Kumar K.V wrote:

quoted

On 4/15/21 7:16 PM, Andy Shevchenko wrote:

quoted

Parse to and export from UUID own type, before dereferencing.
This also fixes wrong comment (Little Endian UUID is something else)
and should fix Sparse warnings about assigning strict types to POD.

Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Not tested
   arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
   1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ae6f5d80d5ce..4366e1902890 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
   	u32 drc_index, metadata_size;
   	u64 blocks, block_size;
   	struct papr_scm_priv *p;
+	u8 uuid_raw[UUID_SIZE];
   	const char *uuid_str;
-	u64 uuid[2];
+	uuid_t uuid;
   	int rc;
   	/* check we have all the required DT properties */

@@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
   	p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
   	/* We just need to ensure that set cookies are unique across */
-	uuid_parse(uuid_str, (uuid_t *) uuid);
+	uuid_parse(uuid_str, &uuid);
+
   	/*
   	 * cookie1 and cookie2 are not really little endian
-	 * we store a little endian representation of the
+	 * we store a raw buffer representation of the
   	 * uuid str so that we can compare this with the label
   	 * area cookie irrespective of the endian config with which
   	 * the kernel is built.
   	 */
-	p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
-	p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
+	export_uuid(uuid_raw, &uuid);
+	p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
+	p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);

ok that does the equivalent of cpu_to_le64 there. So we are good. But the
comment update is missing the details why we did that get_unaligned_le64.
Maybe raw buffer representation is the correct term?
Should we add an example in the comment. ie,

quoted

/*
  * Historically we stored the cookie in the below format.
for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
cookie1 was  0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
*/

I'm fine with the comment. At least it will shed a light on the byte ordering
we are expecting.

Will you be sending an update? Also it will be good to list the sparse
warning in the commit message?

I'll send an update but I rephrase to remove mention of Sparse. I have no
Sparse build for this architecture.

If you have one, try to build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
which will enable warnings about restricted types assignment.

-- 
With Best Regards,
Andy Shevchenko