Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397168 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 1/2] dell-smbios: Add a method for more complex SMI requests Mario Limonciello <mario_limonciello@dell.com> - 2016-05-09 18:30 +0200
    Re: [PATCH v3 1/2] dell-smbios: Add a method for more complex SMI  requests Michał Kępień <kernel@kempniu.pl> - 2016-05-11 14:50 +0200

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

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.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 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'};
 
 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
+ *
+ */
+unsigned char *dell_smbios_send_extended_request(int class, int select,
+						 size_t *length)
+{
+	u32 i;
+	u32 *buffer_length = (u32 *)extended_buffer;
+
+	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);
+
+	*length = buffer_length[0];
+	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);
 fail_buffer:
 	kfree(da_tokens);
 	return ret;
@@ -181,6 +223,7 @@ static void __exit dell_smbios_exit(void)
 {
 	kfree(da_tokens);
 	free_page((unsigned long)buffer);
+	free_page((unsigned long)extended_buffer);
 }
 
 subsys_initcall(dell_smbios_init);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..c0f4395 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -41,6 +41,8 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
+unsigned char *dell_smbios_send_extended_request(int class, int select,
+						 size_t *length);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
 #endif
-- 
2.7.4

[toc] | [next] | [standalone]

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

> 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@dell.com>
> ---
>  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ń

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web