| Message ID | 20210415143754.16553-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] libnvdimm: Don't use GUID APIs against raw buffer | expand |
On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > Strictly speaking the comparison between guid_t and raw buffer > is not correct. Return to plain memcmp() since the data structures > haven't changed to use uuid_t / guid_t the current state of affairs > is inconsistent. Either it should be changed altogether or left > as is. Dan, please review this one as well. I think here you may agree with me.
[ add Erik ] On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > Strictly speaking the comparison between guid_t and raw buffer > > is not correct. Return to plain memcmp() since the data structures > > haven't changed to use uuid_t / guid_t the current state of affairs > > is inconsistent. Either it should be changed altogether or left > > as is. > > Dan, please review this one as well. I think here you may agree with me. You know, this is all a problem because ACPICA is using a raw buffer. Erik, would it be possible to use the guid_t type in ACPICA? That would allow NFIT to drop some ugly casts.
On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > [ add Erik ] > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > Strictly speaking the comparison between guid_t and raw buffer > > > is not correct. Return to plain memcmp() since the data structures > > > haven't changed to use uuid_t / guid_t the current state of affairs > > > is inconsistent. Either it should be changed altogether or left > > > as is. > > > > Dan, please review this one as well. I think here you may agree with me. > > You know, this is all a problem because ACPICA is using a raw buffer. And this is fine. It might be any other representation as well. > Erik, would it be possible to use the guid_t type in ACPICA? That > would allow NFIT to drop some ugly casts. guid_t is internal kernel type. If we ever decide to deviate from the current representation it wouldn't be possible in case a 3rd party is using it 1:1 (via typedef or so).
On Fri, Apr 16, 2021 at 1:42 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > > [ add Erik ] > > > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > > Strictly speaking the comparison between guid_t and raw buffer > > > > is not correct. Return to plain memcmp() since the data structures > > > > haven't changed to use uuid_t / guid_t the current state of affairs > > > > is inconsistent. Either it should be changed altogether or left > > > > as is. > > > > > > Dan, please review this one as well. I think here you may agree with me. > > > > You know, this is all a problem because ACPICA is using a raw buffer. > > And this is fine. It might be any other representation as well. > > > Erik, would it be possible to use the guid_t type in ACPICA? That > > would allow NFIT to drop some ugly casts. > > guid_t is internal kernel type. If we ever decide to deviate from the current > representation it wouldn't be possible in case a 3rd party is using it 1:1 > (via typedef or so). I'm thinking something like ACPICA defining that space as a union with the correct typing just for Linux.
+Bob and Rafael > -----Original Message----- > From: Dan Williams <dan.j.williams@intel.com> > Sent: Friday, April 16, 2021 3:09 PM > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; Verma, Vishal L > <vishal.l.verma@intel.com>; Jiang, Dave <dave.jiang@intel.com>; Weiny, Ira > <ira.weiny@intel.com>; Kaneda, Erik <erik.kaneda@intel.com> > Subject: Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw > buffer > > On Fri, Apr 16, 2021 at 1:42 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > > > [ add Erik ] > > > > > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > > > Strictly speaking the comparison between guid_t and raw buffer > > > > > is not correct. Return to plain memcmp() since the data structures > > > > > haven't changed to use uuid_t / guid_t the current state of affairs > > > > > is inconsistent. Either it should be changed altogether or left > > > > > as is. > > > > > > > > Dan, please review this one as well. I think here you may agree with me. > > > > > > You know, this is all a problem because ACPICA is using a raw buffer. > > > > And this is fine. It might be any other representation as well. > > > > > Erik, would it be possible to use the guid_t type in ACPICA? That > > > would allow NFIT to drop some ugly casts. > > > > guid_t is internal kernel type. If we ever decide to deviate from the current > > representation it wouldn't be possible in case a 3rd party is using it 1:1 > > (via typedef or so). > Hi Dan, > I'm thinking something like ACPICA defining that space as a union with > the correct typing just for Linux. I'm assuming that you mean using something like guid_t type for ACPI data table fields like NFIT rather than objects returned by ACPI namespace object such as _DSD. For ACPI data tables, we could to use macros so that different operating systems can provide their own definitions for a guid. For ACPICA, it will expands to a 16-byte array. Linux can have it's own definition that contains a union or their own guid type (guid_t). As long as the OS-supplied definition is 16bytes, I don't think there would be an issue. Bob, do you have any thoughts on this? Erik
-----Original Message----- From: Kaneda, Erik <erik.kaneda@intel.com> Sent: Monday, April 19, 2021 3:56 PM To: Williams, Dan J <dan.j.williams@intel.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Moore, Robert <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Verma, Vishal L <vishal.l.verma@intel.com>; Jiang, Dave <dave.jiang@intel.com>; Weiny, Ira <ira.weiny@intel.com> Subject: RE: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer +Bob and Rafael > -----Original Message----- > From: Dan Williams <dan.j.williams@intel.com> > Sent: Friday, April 16, 2021 3:09 PM > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org>; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Verma, Vishal L > <vishal.l.verma@intel.com>; Jiang, Dave <dave.jiang@intel.com>; Weiny, > Ira <ira.weiny@intel.com>; Kaneda, Erik <erik.kaneda@intel.com> > Subject: Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw > buffer > > On Fri, Apr 16, 2021 at 1:42 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > > > [ add Erik ] > > > > > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > > > Strictly speaking the comparison between guid_t and raw buffer > > > > > is not correct. Return to plain memcmp() since the data > > > > > structures haven't changed to use uuid_t / guid_t the current > > > > > state of affairs is inconsistent. Either it should be changed > > > > > altogether or left as is. > > > > > > > > Dan, please review this one as well. I think here you may agree with me. > > > > > > You know, this is all a problem because ACPICA is using a raw buffer. > > > > And this is fine. It might be any other representation as well. > > > > > Erik, would it be possible to use the guid_t type in ACPICA? That > > > would allow NFIT to drop some ugly casts. > > > > guid_t is internal kernel type. If we ever decide to deviate from > > the current representation it wouldn't be possible in case a 3rd > > party is using it 1:1 (via typedef or so). > Hi Dan, > I'm thinking something like ACPICA defining that space as a union with > the correct typing just for Linux. I'm assuming that you mean using something like guid_t type for ACPI data table fields like NFIT rather than objects returned by ACPI namespace object such as _DSD. For ACPI data tables, we could to use macros so that different operating systems can provide their own definitions for a guid. For ACPICA, it will expands to a 16-byte array. Linux can have it's own definition that contains a union or their own guid type (guid_t). As long as the OS-supplied definition is 16bytes, I don't think there would be an issue. Erik, I don't like to add these kinds of macros, since it is simply another item that needs to be added to port ACPICA to different hosts. (And must be known by the porter.) Other than that, I suppose we can add such a macro. Bob, do you have any thoughts on this? Erik
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c index 05feb97e11ce..82bcd2e86a18 100644 --- a/drivers/nvdimm/btt_devs.c +++ b/drivers/nvdimm/btt_devs.c @@ -244,13 +244,14 @@ struct device *nd_btt_create(struct nd_region *nd_region) */ bool nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super) { + static const u8 null_uuid[16]; const u8 *parent_uuid = nd_dev_to_uuid(&nd_btt->ndns->dev); u64 checksum; if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) return false; - if (!guid_is_null((guid_t *)&super->parent_uuid)) + if (memcmp(super->parent_uuid, null_uuid, 16) != 0) if (memcmp(super->parent_uuid, parent_uuid, 16) != 0) return false;
Strictly speaking the comparison between guid_t and raw buffer is not correct. Return to plain memcmp() since the data structures haven't changed to use uuid_t / guid_t the current state of affairs is inconsistent. Either it should be changed altogether or left as is. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/nvdimm/btt_devs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)