Re: [PATCH v3 1/2] dell-smbios: Add a method for more complex SMI requests

[MichaÅ KÄpieÅ]

> More complex SMI requests can return data that exceeds the 4 32 bit
> arguments that are traditionally part of a request.
> 
> To support more complex requests, the first input argument can be a
> 32 bit physical address with a buffer properly built in advance.
> 
> This new method prepares the buffer as the BIOS will look for
> it in these requests.

I guess rephrasing this sentence might improve clarity, e.g.:

    This patch adds a new method which prepares the buffer as required
    by the BIOS and then issues the specified SMI request.

> Signed-off-by: Mario Limonciello <mario_limonciello@xxxxxxxx>
> ---
>  drivers/platform/x86/dell-smbios.c | 43 ++++++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..e3bbbb3 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -33,12 +33,14 @@ struct calling_interface_structure {
>  } __packed;
>  
>  static struct calling_interface_buffer *buffer;
> +static unsigned char *extended_buffer;
>  static DEFINE_MUTEX(buffer_mutex);
>  
>  static int da_command_address;
>  static int da_command_code;
>  static int da_num_tokens;
>  static struct calling_interface_token *da_tokens;
> +static const char extended_key[4] = {'D', 'S', 'C', 'I'};

Is there any reason why you preferred declaring a char array over using
a string literal in the memcpy() call below?

>  
>  int dell_smbios_error(int value)
>  {
> @@ -92,6 +94,38 @@ void dell_smbios_send_request(int class, int select)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
>  
> +/* More complex requests are served by sending
> + * a pointer to a pre-allocated buffer
> + * Bytes 0:3 are the size of the return value
> + * Bytes 4:length are the returned value
> + *
> + * The return value is the data of the extended buffer request
> + * The value of length will be updated to the length of the actual
> + * buffer content
> + *
> + */

Nit: could you please format this comment so that it resembles the rest
of the kernel in terms of whitespace placement, i.e.:

 -> /*
     * More complex requests are served by sending
    ...
     * The value of length will be updated to the length of the actual
     * buffer content
 ->  */

> +unsigned char *dell_smbios_send_extended_request(int class, int select,
> +						 size_t *length)

Nice touch with length being an in/out parameter, I like it.

> +{
> +	u32 i;
> +	u32 *buffer_length = (u32 *)extended_buffer;

Please swap these two lines so that local variable declarations are in
"Reverse Christmas Tree" order.

> +
> +	if (*length < 5 || *length - 4 > PAGE_SIZE)
> +		return NULL;
> +
> +	*buffer_length = *length - 4;
> +	for (i = 4; i < *length; i += 4)
> +		if (*length - i > 4)
> +			memcpy(&extended_buffer[i], &extended_key, 4);

I guess a comment just before the for loop might be useful for
documenting the format expected by the BIOS, e.g.:

    /*
     * BIOS will reject the buffer with error code -5 if it does not
     * contain repeating "DSCI" strings from byte 4 onward
     */

> +
> +	*length = buffer_length[0];

This assignment should be moved below the dell_smbios_send_request()
call as at this point the BIOS hasn't yet had a chance to update the
buffer.

By the way, I think it would be nice to use the dereference operator
here instead of the subscript operator for consistency with a similar
assignment 4 lines earlier.

> +	buffer->input[0] = virt_to_phys(extended_buffer);
> +	dell_smbios_send_request(class, select);
> +
> +	return &extended_buffer[4];
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_send_extended_request);
> +
>  struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  {
>  	int i;
> @@ -170,8 +204,16 @@ static int __init dell_smbios_init(void)
>  		goto fail_buffer;
>  	}
>  
> +	extended_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> +	if (!extended_buffer) {
> +		ret = -ENOMEM;
> +		goto fail_extended_buffer;
> +	}
> +
>  	return 0;
>  
> +fail_extended_buffer:
> +	kfree(buffer);

Please use free_page() instead, as in dell_smbios_exit().

-- 
Best regards,
MichaÅ KÄpieÅ