From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Huang\, Ying" Subject: Re: [PATCH v1 06/10] device property: switch to use UUID API Date: Fri, 08 Apr 2016 09:27:12 +0800 Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1460047286.6620.26.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> (Andy Shevchenko's message of "Thu, 7 Apr 2016 19:41:26 +0300") Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Andy Shevchenko Cc: "Rafael J. Wysocki" , Huang Ying , 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-api-u79uwXL29TaqPxH82wqD4g@public.gmane.org List-Id: linux-api@vger.kernel.org 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: >> >=20 >> > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote: >> > >=20 >> > > Switch to use a generic UUID API instead of custom approach. It >> > > allows to >> > > define UUIDs, compare them, and validate. >> [] >>=20 > > 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 ad= d > 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) !=3D = 0) goto skip; Looks better? 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)) { 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. Best Regards, Huang, Ying >> >=20 >> > >=20 >> > > +static const uuid_le ads_uuid =3D >> > > + UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6, >> > > + 0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b); >> > > =C2=A0 >> > > =C2=A0static bool acpi_enumerate_nondev_subnodes(acpi_handle sco= pe, >> > > =C2=A0 =C2=A0=C2=A0=C2=A0const union >> > > acpi_object >> > > *desc, >> > > @@ -138,7 +136,7 @@ static bool >> > > acpi_enumerate_nondev_subnodes(acpi_handle scope, >> > > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0|| links->type !=3D ACPI_TYPE_PA= CKAGE) >> > > =C2=A0 break; >> > > =C2=A0 >> > > - 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. >> >=20 >> > 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 ar= e >> 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 a= t >> compile time. >>=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by ml01.01.org (Postfix) with ESMTP id A0B7A1A1F24 for ; Thu, 7 Apr 2016 18:27:17 -0700 (PDT) From: "Huang\, Ying" Subject: Re: [PATCH v1 06/10] device property: switch to use UUID API 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> Date: Fri, 08 Apr 2016 09:27:12 +0800 In-Reply-To: <1460047286.6620.26.camel@linux.intel.com> (Andy Shevchenko's message of "Thu, 7 Apr 2016 19:41:26 +0300") Message-ID: <877fg828lr.fsf@yhuang-dev.intel.com> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Andy Shevchenko Cc: linux-efi@vger.kernel.org, David Airlie , Rasmus Villemoes , dri-devel@lists.freedesktop.org, Benjamin Tissoires , Lv Zheng , Matt Fleming , Arnd Bergmann , linux-nvdimm@lists.01.org, linux-acpi@vger.kernel.org, Huang Ying , Zhang Rui , Mathias Nyman , Jani Nikula , Mark Brown , Jarkko Sakkinen , Bjorn Helgaas , Mika Westerberg , Theodore Ts'o , Greg Kroah-Hartman , "Rafael J. Wysocki" , linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Andrew Morton List-ID: QW5keSBTaGV2Y2hlbmtvIDxhbmRyaXkuc2hldmNoZW5rb0BsaW51eC5pbnRlbC5jb20+IHdyaXRl czoKCj4gT24gRnJpLCAyMDE2LTAyLTI2IGF0IDE2OjExICswMjAwLCBBbmR5IFNoZXZjaGVua28g d3JvdGU6Cj4+IE9uIFRodSwgMjAxNi0wMi0xOCBhdCAwMTowMyArMDEwMCwgUmFmYWVsIEouIFd5 c29ja2kgd3JvdGU6Cj4+ID4gCj4+ID4gT24gV2VkbmVzZGF5LCBGZWJydWFyeSAxNywgMjAxNiAw MjoxNzoyNCBQTSBBbmR5IFNoZXZjaGVua28gd3JvdGU6Cj4+ID4gPiAKPj4gPiA+IFN3aXRjaCB0 byB1c2UgYSBnZW5lcmljIFVVSUQgQVBJIGluc3RlYWQgb2YgY3VzdG9tIGFwcHJvYWNoLiBJdAo+ PiA+ID4gYWxsb3dzIHRvCj4+ID4gPiBkZWZpbmUgVVVJRHMsIGNvbXBhcmUgdGhlbSwgYW5kIHZh bGlkYXRlLgo+PiBbXQo+PiAKPgo+IFN1bW1vbiBpbml0aWFsIGF1dGhvciBvZiB0aGUgVVVJRCBs aWJyYXJ5Lgo+Cj4gU3VtbWFyeTogdGhlIEFQSSBvZiBjb21wYXJpc29uIGZ1bmN0aW9ucyBpcyBy YXRoZXIgc3RyYW5nZS4gV2hhdCB0aGUKPiBwb2ludCB0byBub3QgdGFrZSBwb2ludGVycyBkaXJl Y3RseT8gKE1vcmVvdmVyIEkgaG9wZSBjb21waWxlciB0b28KPiBjbGV2ZXIgbm90IHRvIG1ha2Ug YSBjb3B5IG9mIGNvbnN0YW50IGFyZ3VtZW50cyB0aGVyZSkKPgo+IEkgY291bGQgb25seSBpbWFn aW5lIHRoZSBjYXNlIHlvdSBhcmUgdHJ5aW5nIHRvIGF2b2lkIHRlbXBvcmFyeQo+IHZhcmlhYmxl cyBmb3IgY29uc3RhbnRzIGxpa2UgTlVMTF9VVUlELgo+Cj4gSXNzdWUgd2l0aCB0aGlzIGlzIHRo ZSB1Z2xpbmVzcyBpbiB0aGUgdXNlcnMgb2YgdGhhdCwgaW4gcGFydGljdWxhcmx5Cj4gcHJlc2Vu dCBpbiBBQ1BJIChkcml2ZXJzL2FjcGkvYXBlaS9naGVzLmMpLgo+Cj4gSSB3b3VsZCBsaWtlIHRv IGhhdmUgbW9yZSBjbGVhciBpbnRlcmZhY2UgZm9yIHRoYXQuIFBlcmhhcHMgd2UgbWF5IGFkZAo+ IHNvbWV0aGluZyBsaWtlCj4KPiBjbXBfcChwb2ludGVyLCBub24tcG9pbnRlcik7Cj4gY21wX3Bw KHBvaW50ZXIsIHBvaW50ZXIpOwo+Cj4gdG8gbm90IGJyZWFrIGV4aXN0aW5nIEFQSSBmb3Igbm93 Lgo+Cj4gSXQgd291bGQgYmUgdXNlZnVsIGZvciBtYW55IGNhc2VzIGluIHRoZSBrZXJuZWwuCgpZ b3UgY2FuIHRha2UgYSBsb29rIGF0IHRoZSBkcml2ZXJzL2FjcGkvYXBlaS9lcnN0LmMgZm9yIHV1 aWRfbGVfY21wCnVzYWdlLgoKI2RlZmluZSBDUEVSX0NSRUFUT1JfUFNUT1JFICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgXAogICAgICAgIFVVSURfTEUoMHg3NWE1 NzRlMywgMHg1MDUyLCAweDRiMjksIDB4OGEsIDB4OGUsIDB4YmUsIDB4MmMsICAgICBcCiAgICAg ICAgICAgICAgICAweDY0LCAweDkwLCAweGI4LCAweDlkKQoKICAgICAgICBpZiAodXVpZF9sZV9j bXAocmNkLT5oZHIuY3JlYXRvcl9pZCwgQ1BFUl9DUkVBVE9SX1BTVE9SRSkgIT0gMCkKICAgICAg ICAgICAgICAgIGdvdG8gc2tpcDsKCkxvb2tzIGJldHRlcj8KClRoaXMgaXMgdGhlIHR5cGljYWwg dXNlIGNhc2UgaW4gbWluZCB3aGVuIEkgd3JpdGUgdGhlIHV1aWQuaC4KCkFzIGZvciB1dWlkX2xl X2NtcCB1c2FnZSBpbiBkcml2ZXJzL2FjcGkvYXBlaS9naGVzLmMsCgoJCWlmICghdXVpZF9sZV9j bXAoKih1dWlkX2xlICopZ2RhdGEtPnNlY3Rpb25fdHlwZSwKCQkJCSBDUEVSX1NFQ19QTEFURk9S TV9NRU0pKSB7CgpUaGUgY29kZSBsb29rcyBub3QgZ29vZCBtYWlubHkgYmVjYXVzZSBhY3BpX2hl c3RfZ2VuZXJpY19kYXRhIGlzIG5vdApkZWZpbmVkIHdpdGggdXVpZF9sZSBpbiBtaW5kLgoKc3Ry dWN0IGFjcGlfaGVzdF9nZW5lcmljX2RhdGEgewoJdTggc2VjdGlvbl90eXBlWzE2XTsKCXUzMiBl cnJvcl9zZXZlcml0eTsKCXUxNiByZXZpc2lvbjsKCXU4IHZhbGlkYXRpb25fYml0czsKCXU4IGZs YWdzOwoJdTMyIGVycm9yX2RhdGFfbGVuZ3RoOwoJdTggZnJ1X2lkWzE2XTsKCXU4IGZydV90ZXh0 WzIwXTsKfTsKCklmIHNlY3Rpb25fdHlwZSB3YXMgZGVmaW5lZCBhcyB1dWlkX2xlIGluc3RlYWQg b2YgdThbMTZdLCB0aGUKdXVpZF9sZV9jbXAgdXNhZ2Ugd291bGQgbG9vayBiZXR0ZXIuICBTbyBJ IHN1Z2dlc3QgdG8gdXNlIHV1aWRfbGUvYmUgaW4KZGF0YSBzdHJ1Y3R1cmUgZGVmaW5pdGlvbiBp biBuZXcgY29kZSBpZiBwb3NzaWJsZS4KCkJlc3QgUmVnYXJkcywKSHVhbmcsIFlpbmcKCj4+ID4g Cj4+ID4gPiAKPj4gPiA+ICtzdGF0aWMgY29uc3QgdXVpZF9sZSBhZHNfdXVpZCA9Cj4+ID4gPiAr CVVVSURfTEUoMHhkYmI4ZTNlNiwgMHg1ODg2LCAweDRiYTYsCj4+ID4gPiArCQkweDg3LCAweDk1 LCAweDEzLCAweDE5LCAweGY1LCAweDJhLCAweDk2LCAweDZiKTsKPj4gPiA+IMKgCj4+ID4gPiDC oHN0YXRpYyBib29sIGFjcGlfZW51bWVyYXRlX25vbmRldl9zdWJub2RlcyhhY3BpX2hhbmRsZSBz Y29wZSwKPj4gPiA+IMKgCQkJCQnCoMKgwqBjb25zdCB1bmlvbgo+PiA+ID4gYWNwaV9vYmplY3QK Pj4gPiA+ICpkZXNjLAo+PiA+ID4gQEAgLTEzOCw3ICsxMzYsNyBAQCBzdGF0aWMgYm9vbAo+PiA+ ID4gYWNwaV9lbnVtZXJhdGVfbm9uZGV2X3N1Ym5vZGVzKGFjcGlfaGFuZGxlIHNjb3BlLAo+PiA+ ID4gwqAJCcKgwqDCoMKgfHwgbGlua3MtPnR5cGUgIT0gQUNQSV9UWVBFX1BBQ0tBR0UpCj4+ID4g PiDCoAkJCWJyZWFrOwo+PiA+ID4gwqAKPj4gPiA+IC0JCWlmIChtZW1jbXAodXVpZC0+YnVmZmVy LnBvaW50ZXIsIGFkc191dWlkLAo+PiA+ID4gc2l6ZW9mKGFkc191dWlkKSkpCj4+ID4gPiArCQlp ZiAodXVpZF9sZV9jbXAoKih1dWlkX2xlICopdXVpZC0+YnVmZmVyLnBvaW50ZXIsCj4+ID4gPiBh ZHNfdXVpZCkpCj4+ID4gTWF5YmUgaXQncyB0b28gbGF0ZSwgYnV0IEkgZG9uJ3QgcXVpdGUgdW5k ZXJzdGFuZCB0aGUgcG9pbnRlcgo+PiA+IG1hbmlwdWxhdGlvbnMgaGVyZS4KPj4gPiAKPj4gPiBJ IGNhbiBzZWUgd2h5IHlvdSBuZWVkIGEgdHlwZSBjb252ZXJzaW9uIChhbHRob3VnaCBpdCBsb29r cyB1Z2x5KSwKPj4gPiBidXQgd2h5IGRvIHlvdQo+PiA+IG5lZWQgdG8gZGVyZWZlcmVuY2UgaXQg dG9vPwo+PiBUaGUgZnVuY3Rpb24gdGFrZXMgdGhhdCBraW5kIG9mIHR5cGUgb24gaW5wdXQuIFRo ZSBvdGhlciB2YXJpYW50cyBhcmUKPj4gbm90IGNvbXBpbGVkLgo+PiBQZXJoYXBzIHdlIGJldHRl ciBjaGFuZ2UgdXVpZF97bGJ9ZV9jbXAoKSBmaXJzdCB0byB0YWtlIG5vcm1hbAo+PiBwb2ludGVy cywgdGhvdWdoIEkgdGhpbmsgdGhlIGluaXRpYWwgaWRlYSB3YXMgdG8gZ2V0IHR5cGUgY2hlY2tp bmcgYXQKPj4gY29tcGlsZSB0aW1lLgo+PiAKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18KTGludXgtbnZkaW1tIG1haWxpbmcgbGlzdApMaW51eC1udmRpbW1A bGlzdHMuMDEub3JnCmh0dHBzOi8vbGlzdHMuMDEub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgt bnZkaW1tCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756419AbcDHB1V (ORCPT ); Thu, 7 Apr 2016 21:27:21 -0400 Received: from mga03.intel.com ([134.134.136.65]:12799 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751076AbcDHB1T (ORCPT ); Thu, 7 Apr 2016 21:27:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,449,1455004800"; d="scan'208";a="780481899" From: "Huang\, Ying" To: Andy Shevchenko Cc: "Rafael J. Wysocki" , Huang Ying , "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 , , , , , , Subject: Re: [PATCH v1 06/10] device property: switch to use UUID API 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> Date: Fri, 08 Apr 2016 09:27:12 +0800 In-Reply-To: <1460047286.6620.26.camel@linux.intel.com> (Andy Shevchenko's message of "Thu, 7 Apr 2016 19:41:26 +0300") Message-ID: <877fg828lr.fsf@yhuang-dev.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? 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)) { 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. 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. >>