Re: [PATCH v1 2/2] wmi: use generic UUID library

[Darren Hart]

On Thu, Apr 07, 2016 at 08:00:55PM +0300, Andy Shevchenko wrote:
> Instead of opencoding let's use generic UUID library functions here.
> 
> Cc: Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/platform/x86/wmi.c | 104 ++++++---------------------------------------
>  1 file changed, 13 insertions(+), 91 deletions(-)
> 

My favorite kind of patches :-)

The only functional difference I detected was an additional check/errcode-return
in uuid_le_to_bin, but this is a bug fix in and of itself.

I'm happy to see this go in once the details of the generic uuid library as
discussed in the related threads are ironed out.

Andrew, will you be pulling this in as part of that series?

Reviewed-by: Darren Hart <dvhart@xxxxxxxxxxxxxxx>

> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index eb391a2..ceeb8c1 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -37,6 +37,7 @@
>  #include <linux/acpi.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/uuid.h>
>  
>  ACPI_MODULE_NAME("wmi");
>  MODULE_AUTHOR("Carlos Corbacho");
> @@ -115,100 +116,21 @@ static struct acpi_driver acpi_wmi_driver = {
>   * GUID parsing functions
>   */
>  
> -/**
> - * wmi_parse_hexbyte - Convert a ASCII hex number to a byte
> - * @src:  Pointer to at least 2 characters to convert.
> - *
> - * Convert a two character ASCII hex string to a number.
> - *
> - * Return:  0-255  Success, the byte was parsed correctly
> - *          -1     Error, an invalid character was supplied
> - */
> -static int wmi_parse_hexbyte(const u8 *src)
> -{
> -	int h;
> -	int value;
> -
> -	/* high part */
> -	h = value = hex_to_bin(src[0]);
> -	if (value < 0)
> -		return -1;
> -
> -	/* low part */
> -	value = hex_to_bin(src[1]);
> -	if (value >= 0)
> -		return (h << 4) | value;
> -	return -1;
> -}
> -
> -/**
> - * wmi_swap_bytes - Rearrange GUID bytes to match GUID binary
> - * @src:   Memory block holding binary GUID (16 bytes)
> - * @dest:  Memory block to hold byte swapped binary GUID (16 bytes)
> - *
> - * Byte swap a binary GUID to match it's real GUID value
> - */
> -static void wmi_swap_bytes(u8 *src, u8 *dest)
> -{
> -	int i;
> -
> -	for (i = 0; i <= 3; i++)
> -		memcpy(dest + i, src + (3 - i), 1);
> -
> -	for (i = 0; i <= 1; i++)
> -		memcpy(dest + 4 + i, src + (5 - i), 1);
> -
> -	for (i = 0; i <= 1; i++)
> -		memcpy(dest + 6 + i, src + (7 - i), 1);
> -
> -	memcpy(dest + 8, src + 8, 8);
> -}
> -
> -/**
> - * wmi_parse_guid - Convert GUID from ASCII to binary
> - * @src:   36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
> - * @dest:  Memory block to hold binary GUID (16 bytes)
> - *
> - * N.B. The GUID need not be NULL terminated.
> - *
> - * Return:  'true'   @dest contains binary GUID
> - *          'false'  @dest contents are undefined
> - */
> -static bool wmi_parse_guid(const u8 *src, u8 *dest)
> -{
> -	static const int size[] = { 4, 2, 2, 2, 6 };
> -	int i, j, v;
> -
> -	if (src[8]  != '-' || src[13] != '-' ||
> -		src[18] != '-' || src[23] != '-')
> -		return false;
> -
> -	for (j = 0; j < 5; j++, src++) {
> -		for (i = 0; i < size[j]; i++, src += 2, *dest++ = v) {
> -			v = wmi_parse_hexbyte(src);
> -			if (v < 0)
> -				return false;
> -		}
> -	}
> -
> -	return true;
> -}
> -
>  static bool find_guid(const char *guid_string, struct wmi_block **out)
>  {
> -	char tmp[16], guid_input[16];
> +	uuid_le guid_input;
>  	struct wmi_block *wblock;
>  	struct guid_block *block;
>  	struct list_head *p;
>  
> -	wmi_parse_guid(guid_string, tmp);
> -	wmi_swap_bytes(tmp, guid_input);
> +	if (uuid_le_to_bin(guid_string, &guid_input))
> +		return false;
>  
>  	list_for_each(p, &wmi_block_list) {
>  		wblock = list_entry(p, struct wmi_block, list);
>  		block = &wblock->gblock;
>  
> -		if (memcmp(block->guid, guid_input, 16) == 0) {
> +		if (memcmp(block->guid, &guid_input, 16) == 0) {
>  			if (out)
>  				*out = wblock;
>  			return true;
> @@ -498,20 +420,20 @@ wmi_notify_handler handler, void *data)
>  {
>  	struct wmi_block *block;
>  	acpi_status status = AE_NOT_EXIST;
> -	char tmp[16], guid_input[16];
> +	uuid_le guid_input;
>  	struct list_head *p;
>  
>  	if (!guid || !handler)
>  		return AE_BAD_PARAMETER;
>  
> -	wmi_parse_guid(guid, tmp);
> -	wmi_swap_bytes(tmp, guid_input);
> +	if (uuid_le_to_bin(guid, &guid_input))
> +		return AE_BAD_PARAMETER;
>  
>  	list_for_each(p, &wmi_block_list) {
>  		acpi_status wmi_status;
>  		block = list_entry(p, struct wmi_block, list);
>  
> -		if (memcmp(block->gblock.guid, guid_input, 16) == 0) {
> +		if (memcmp(block->gblock.guid, &guid_input, 16) == 0) {
>  			if (block->handler &&
>  			    block->handler != wmi_notify_debug)
>  				return AE_ALREADY_ACQUIRED;
> @@ -539,20 +461,20 @@ acpi_status wmi_remove_notify_handler(const char *guid)
>  {
>  	struct wmi_block *block;
>  	acpi_status status = AE_NOT_EXIST;
> -	char tmp[16], guid_input[16];
> +	uuid_le guid_input;
>  	struct list_head *p;
>  
>  	if (!guid)
>  		return AE_BAD_PARAMETER;
>  
> -	wmi_parse_guid(guid, tmp);
> -	wmi_swap_bytes(tmp, guid_input);
> +	if (uuid_le_to_bin(guid, &guid_input))
> +		return AE_BAD_PARAMETER;
>  
>  	list_for_each(p, &wmi_block_list) {
>  		acpi_status wmi_status;
>  		block = list_entry(p, struct wmi_block, list);
>  
> -		if (memcmp(block->gblock.guid, guid_input, 16) == 0) {
> +		if (memcmp(block->gblock.guid, &guid_input, 16) == 0) {
>  			if (!block->handler ||
>  			    block->handler == wmi_notify_debug)
>  				return AE_NULL_ENTRY;
> -- 
> 2.8.0.rc3
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center