Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758169AbcDHJ7x (ORCPT ); Fri, 8 Apr 2016 05:59:53 -0400 Received: from mga09.intel.com ([134.134.136.24]:11725 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753088AbcDHJ7v (ORCPT ); Fri, 8 Apr 2016 05:59:51 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,449,1455004800"; d="scan'208";a="940980485" Message-ID: <1460109638.6620.41.camel@linux.intel.com> Subject: Re: [PATCH v1 06/10] device property: switch to use UUID API From: Andy Shevchenko To: "Huang, Ying" Cc: "Rafael J. Wysocki" , "Theodore Ts'o" , Arnd Bergmann , Greg Kroah-Hartman , Jarkko Sakkinen , Jani Nikula , David Airlie , Benjamin Tissoires , Bjorn Helgaas , Mathias Nyman , Matt Fleming , Lv Zheng , Mark Brown , Zhang Rui , Mika Westerberg , Andrew Morton , Rasmus Villemoes , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-efi@vger.kernel.org, linux-api@vger.kernel.org, linux-nvdimm@ml01.01.org Date: Fri, 08 Apr 2016 13:00:38 +0300 In-Reply-To: <877fg828lr.fsf@yhuang-dev.intel.com> References: <1455711448-124103-1-git-send-email-andriy.shevchenko@linux.intel.com> <1455711448-124103-7-git-send-email-andriy.shevchenko@linux.intel.com> <7544228.v4QPX4F7J7@vostro.rjw.lan> <1456495897.13244.144.camel@linux.intel.com> <1460047286.6620.26.camel@linux.intel.com> <877fg828lr.fsf@yhuang-dev.intel.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4682 Lines: 154 On Fri, 2016-04-08 at 09:27 +0800, Huang, Ying wrote: > Andy Shevchenko writes: > > > > > On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote: > > > > > > On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote: > > > > > > > > > > > > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko > > > > wrote: > > > > > > > > > > > > > > > Switch to use a generic UUID API instead of custom approach. > > > > > It > > > > > allows to > > > > > define UUIDs, compare them, and validate. > > > [] > > > > > Summon initial author of the UUID library. > > > > Summary: the API of comparison functions is rather strange. What the > > point to not take pointers directly? (Moreover I hope compiler too > > clever not to make a copy of constant arguments there) > > > > I could only imagine the case you are trying to avoid temporary > > variables for constants like NULL_UUID. > > > > Issue with this is the ugliness in the users of that, in > > particularly > > present in ACPI (drivers/acpi/apei/ghes.c). > > > > I would like to have more clear interface for that. Perhaps we may > > add > > something like > > > > cmp_p(pointer, non-pointer); > > cmp_pp(pointer, pointer); > > > > to not break existing API for now. > > > > It would be useful for many cases in the kernel. > You can take a look at the drivers/acpi/apei/erst.c for uuid_le_cmp > usage. > > #define > CPER_CREATOR_PSTORE                                             \ >         UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, > 0x2c,     \ >                 0x64, 0x90, 0xb8, 0x9d) > >         if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != > 0) >                 goto skip; > > Looks better? I don't quite understand the issues with  if (uuid_le_cmp(&rcd->hdr.creator_id, &CPER_CREATOR_PSTORE) != 0) or, like I mentioned previously, we may introduce _cmp_p() and use like if (uuid_le_cmp_p(&rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0) if it looks better (again, I don't know if compiler is going to copy the last argument). > > This is the typical use case in mind when I write the uuid.h. > > As for uuid_le_cmp usage in drivers/acpi/apei/ghes.c, > > if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, >  CPER_SEC_PLATFORM_MEM)) { Ditto if (!uuid_le_cmp_p((uuid_le *)gdata->section_type, CPER_SEC_PLATFORM_MEM)) { > > The code looks not good mainly because acpi_hest_generic_data is not > defined with uuid_le in mind. > > struct acpi_hest_generic_data { > u8 section_type[16]; > u32 error_severity; > u16 revision; > u8 validation_bits; > u8 flags; > u32 error_data_length; > u8 fru_id[16]; > u8 fru_text[20]; > }; > > If section_type was defined as uuid_le instead of u8[16], the > uuid_le_cmp usage would look better.  So I suggest to use uuid_le/be > in > data structure definition in new code if possible. This is understandable for such structures, but we might get a UUID from a buffer which is pointer to u8. It's not possible to convert to uuid_* since it's too generic stuff and might require to introduce ACPI_TYPE_UUID with standardization and all necessary work. Apparently not the shortest way. > > Best Regards, > Huang, Ying > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +static const uuid_le ads_uuid = > > > > > + UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6, > > > > > + 0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, > > > > > 0x6b); > > > > >   > > > > >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope, > > > > >      const union > > > > > acpi_object > > > > > *desc, > > > > > @@ -138,7 +136,7 @@ static bool > > > > > acpi_enumerate_nondev_subnodes(acpi_handle scope, > > > > >       || links->type != ACPI_TYPE_PACKAGE) > > > > >   break; > > > > >   > > > > > - if (memcmp(uuid->buffer.pointer, ads_uuid, > > > > > sizeof(ads_uuid))) > > > > > + if (uuid_le_cmp(*(uuid_le *)uuid- > > > > > >buffer.pointer, > > > > > ads_uuid)) > > > > Maybe it's too late, but I don't quite understand the pointer > > > > manipulations here. > > > > > > > > I can see why you need a type conversion (although it looks > > > > ugly), > > > > but why do you > > > > need to dereference it too? > > > The function takes that kind of type on input. The other variants > > > are > > > not compiled. > > > Perhaps we better change uuid_{lb}e_cmp() first to take normal > > > pointers, though I think the initial idea was to get type checking > > > at > > > compile time. > > > -- Andy Shevchenko Intel Finland Oy