| Message ID | 20210415134637.17770-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/1] powerpc/papr_scm: Properly handle UUID types and API | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (0702e74703f57173e70cfab2a79a3e682e9e96ec) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
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.
On Thu, Apr 15, 2021 at 8:10 PM Vaibhav Jain <vaibhav@linux.ibm.com> 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.
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. > 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 <aneesh.kumar@linux.ibm.com> > 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; >
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. > > 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 <aneesh.kumar@linux.ibm.com> > 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 */ > /* 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: > > 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 <aneesh.kumar@linux.ibm.com> > > 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. > > /* might be zero */ > > p->metadata_size = metadata_size;
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: >> 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. >>> >>> 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 <aneesh.kumar@linux.ibm.com> >>> 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. > Will you be sending an update? Also it will be good to list the sparse warning in the commit message? -aneesh
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Thu, Apr 15, 2021 at 8:10 PM Vaibhav Jain <vaibhav@linux.ibm.com> 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. 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
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: > > 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: > > > > 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 <aneesh.kumar@linux.ibm.com> > > > > 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. > > > > 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.
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;
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 <aneesh.kumar@linux.ibm.com> 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(-)