> 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 <[email protected]>
> ---
> 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ń
