Re: [char-misc-next 04/11 V2] uuid: extract macros for assigning raw arrays

[Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>]

On Wed, May 27, 2015 at 04:14:20PM -0700, Greg KH wrote:
> On Wed, May 27, 2015 at 05:42:22PM +0000, Winkler, Tomas wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org]
> > > Sent: Wednesday, May 27, 2015 20:29
> > > To: Winkler, Tomas
> > > Cc: arnd-r2nGTMty4D4@public.gmane.org; Stephen Rothwell; linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Subject: Re: [char-misc-next 04/11 V2] uuid: extract macros for assigning raw
> > > arrays
> > > 
> > > On Wed, May 27, 2015 at 05:24:01PM +0000, Winkler, Tomas wrote:
> > > >
> > > > >
> > > > > On Wed, May 27, 2015 at 06:42:13PM +0300, Tomas Winkler wrote:
> > > > > > In order for mei client devices to use device id based on uuid we
> > > > > > have to use common types between user space (file2alias.c).
> > > > > > Similarly to vmbus,  mei  uses raw 16 byte array for that.
> > > > > > To leverage on existing infrastructure around uuid_le type
> > > > > > defined in uuid.h we add helper macros to handle conversions between
> > > > > > raw 16 byte array and uuid_{le,be} types.
> > > > >
> > > > > You aren't adding a helper macro, you are just redefining the existing
> > > > > macros using a different one.
> > > >
> > > > Not exactly I'm using both the one I've added for device ids and the old one for
> > > all the other flows.
> > > >
> > > >  But I can't see why this is needed, what
> > > > > does this solve that vmbus and other uses of the existing macros don't
> > > > > need?  In other words, what makes mei so special that it needs a "lower"
> > > > > level macro than every other subsystem?
> > > >
> > > > It's not special there is actually a lot of code duplication around uuid handling
> > > > every subsystem is using their own macros but it can be consolidated around
> > > uuid.h
> > > >
> > > > So vmbus can use that
> > > > Instead of
> > > > /*
> > > >  * Network GUID
> > > >  * {f8615163-df3e-46c5-913f-f2d2f965ed0e}
> > > >  */
> > > > #define HV_NIC_GUID \
> > > >         .guid = { \
> > > >                         0x63, 0x51, 0x61, 0xf8, 0x3e, 0xdf, 0xc5, 0x46, \
> > > >                         0x91, 0x3f, 0xf2, 0xd2, 0xf9, 0x65, 0xed, 0x0e \
> > > >                 }
> > > >
> > > > The can use the new macro to make it more readable, something in spirit of:
> > > >
> > > > #define HV_NIC_GUID __UUID_LE(f8615163-df3e-46c5-913f-f2d2f965ed0e)
> > > 
> > > Why the "__" usage here?  That signifies a "private" namespace, why add
> > > that to the user visible header files?
> > 
> > I take any other suggestion for macro names.  
> > Not sure how I would reuse the macros if I don't export them both,
> > second this can be used also by user space.  
> 
> But it's not needed at all.
> 
> Below is a patch on top of my current tree that makes this patch not
> needed.  Any objection to me just applying it instead?
> 
> > > And are you going to send patches for vmbus and other drivers to fix
> > > everything up to use these new macros?  Someone has to...
> > 
> > Can be done but I cannot test their code and  now I'm busy with splitting the big bus patch :)
> 
> Ok, that means no one is ever going to do that work, so it's not a valid
> reason to accept such a change.  I prefer the patch below.

Oops, that didn't build, I forgot one change in the mei core (getting
rid of two casts.)  Try this one instead.

thanks,

greg k-h


diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index de8fd089a8a4..dd52f224027e 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -54,9 +54,9 @@ static int mei_cl_device_match(struct device *dev, struct device_driver *drv)
 
 	id = driver->id_table;
 
-	while (uuid_le_cmp(NULL_UUID_LE, uuid_le_cast(id->uuid))) {
+	while (uuid_le_cmp(NULL_UUID_LE, id->uuid)) {
 
-		if (!uuid_le_cmp(*uuid, uuid_le_cast(id->uuid))) {
+		if (!uuid_le_cmp(*uuid, id->uuid)) {
 			if (id->name[0]) {
 				if (!strncmp(name, id->name, sizeof(id->name)))
 					return 1;
diff --git a/drivers/nfc/mei_phy.h b/drivers/nfc/mei_phy.h
index a51f8f2685cc..fbfa3e61738f 100644
--- a/drivers/nfc/mei_phy.h
+++ b/drivers/nfc/mei_phy.h
@@ -5,7 +5,7 @@
 #include <net/nfc/hci.h>
 #include <linux/uuid.h>
 
-#define MEI_NFC_UUID __UUID_LE(0x0bb17a78, 0x2a8e, 0x4c50, \
+#define MEI_NFC_UUID UUID_LE(0x0bb17a78, 0x2a8e, 0x4c50, \
 		0x94, 0xd4, 0x50, 0x26, 0x67, 0x23, 0x77, 0x5c)
 #define MEI_NFC_HEADER_SIZE 10
 #define MEI_NFC_MAX_HCI_PAYLOAD 300
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 2d2b2b571d61..048c270822f9 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -614,7 +614,7 @@ struct ipack_device_id {
  */
 struct mei_cl_device_id {
 	char name[MEI_CL_NAME_SIZE];
-	__u8 uuid[16];
+	uuid_le uuid;
 	kernel_ulong_t driver_info;
 };
 
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 62c517f4b592..718b2a29bd43 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -34,6 +34,9 @@ typedef Elf64_Addr	kernel_ulong_t;
 typedef uint32_t	__u32;
 typedef uint16_t	__u16;
 typedef unsigned char	__u8;
+typedef struct {
+	__u8 b[16];
+} uuid_le;
 
 /* Big exception to the "don't include kernel headers into userspace, which
  * even potentially has different endianness and word sizes, since
@@ -131,13 +134,13 @@ static inline void add_wildcard(char *str)
 		strcat(str + len, "*");
 }
 
-static inline void add_uuid(char *str, __u8 uuid[16])
+static inline void add_uuid(char *str, uuid_le uuid)
 {
 	int len = strlen(str);
 	int i;
 
 	for (i = 0; i < 16; i++)
-		sprintf(str + len + (i << 1), "%02x", uuid[i]);
+		sprintf(str + len + (i << 1), "%02x", uuid.b[i]);
 }
 
 /**