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

Groups > linux.kernel > #1310966 > unrolled thread

Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-16 16:30 +0100
    Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-18 11:40 +0100
    Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-20 10:30 +0100
      Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-21 09:40 +0100
        Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-21 14:10 +0100
          Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-21 14:20 +0100
            Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-21 14:40 +0100

#1310966 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

On Tuesday 12 January 2016 15:02:47 Michał Kępień wrote:
> Extract SMBIOS-related code from dell-laptop to a new kernel module,
> dell-smbios.  The static specifier was removed from exported symbols,
> otherwise code is just moved around.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/Kconfig       |   12 ++-
>  drivers/platform/x86/Makefile      |    1 +
>  drivers/platform/x86/dell-laptop.c |  163 +-----------------------------
>  drivers/platform/x86/dell-smbios.c |  193 ++++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |   51 ++++++++++
>  5 files changed, 257 insertions(+), 163 deletions(-)
>  create mode 100644 drivers/platform/x86/dell-smbios.c
>  create mode 100644 drivers/platform/x86/dell-smbios.h
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f37821f..177a794 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -91,10 +91,20 @@ config ASUS_LAPTOP
>  
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
> +config DELL_SMBIOS
> +	tristate "Dell SMBIOS Support"
> +	depends on DCDBAS
> +	default n
> +	---help---
> +	This module provides common functions for kernel modules using
> +	Dell SMBIOS.
> +
> +	If you have a Dell laptop, say Y or M here.
> +
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
>  	depends on X86
> -	depends on DCDBAS
> +	depends on DELL_SMBIOS
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
>  	depends on RFKILL || RFKILL = n
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 8b8df29..1128595 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_EEEPC_WMI)		+= eeepc-wmi.o
>  obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
>  obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
>  obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
> +obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
>  obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
>  obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
>  obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index aaeeae8..d45d356 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -28,12 +28,11 @@
>  #include <linux/acpi.h>
>  #include <linux/mm.h>
>  #include <linux/i8042.h>
> -#include <linux/slab.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
>  #include <acpi/video.h>
> -#include "../../firmware/dcdbas.h"
>  #include "dell-rbtn.h"
> +#include "dell-smbios.h"
>  
>  #define BRIGHTNESS_TOKEN 0x7d
>  #define KBD_LED_OFF_TOKEN 0x01E1
> @@ -44,33 +43,6 @@
>  #define KBD_LED_AUTO_75_TOKEN 0x02EC
>  #define KBD_LED_AUTO_100_TOKEN 0x02F6
>  
> -/* This structure will be modified by the firmware when we enter
> - * system management mode, hence the volatiles */
> -
> -struct calling_interface_buffer {
> -	u16 class;
> -	u16 select;
> -	volatile u32 input[4];
> -	volatile u32 output[4];
> -} __packed;
> -
> -struct calling_interface_token {
> -	u16 tokenID;
> -	u16 location;
> -	union {
> -		u16 value;
> -		u16 stringlength;
> -	};
> -};
> -
> -struct calling_interface_structure {
> -	struct dmi_header header;
> -	u16 cmdIOAddress;
> -	u8 cmdIOCode;
> -	u32 supportedCmds;
> -	struct calling_interface_token tokens[];
> -} __packed;
> -
>  struct quirk_entry {
>  	u8 touchpad_led;
>  
> @@ -103,11 +75,6 @@ static struct quirk_entry quirk_dell_xps13_9333 = {
>  	.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
>  };
>  
> -static int da_command_address;
> -static int da_command_code;
> -static int da_num_tokens;
> -static struct calling_interface_token *da_tokens;
> -
>  static struct platform_driver platform_driver = {
>  	.driver = {
>  		.name = "dell-laptop",
> @@ -306,112 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static struct calling_interface_buffer *buffer;
> -static DEFINE_MUTEX(buffer_mutex);
> -
> -static void clear_buffer(void)
> -{
> -	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> -}
> -
> -static void get_buffer(void)
> -{
> -	mutex_lock(&buffer_mutex);
> -	clear_buffer();
> -}
> -
> -static void release_buffer(void)
> -{
> -	mutex_unlock(&buffer_mutex);
> -}
> -
> -static void __init parse_da_table(const struct dmi_header *dm)
> -{
> -	/* Final token is a terminator, so we don't want to copy it */
> -	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
> -	struct calling_interface_token *new_da_tokens;
> -	struct calling_interface_structure *table =
> -		container_of(dm, struct calling_interface_structure, header);
> -
> -	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
> -	   6 bytes of entry */
> -
> -	if (dm->length < 17)
> -		return;
> -
> -	da_command_address = table->cmdIOAddress;
> -	da_command_code = table->cmdIOCode;
> -
> -	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> -				 sizeof(struct calling_interface_token),
> -				 GFP_KERNEL);
> -
> -	if (!new_da_tokens)
> -		return;
> -	da_tokens = new_da_tokens;
> -
> -	memcpy(da_tokens+da_num_tokens, table->tokens,
> -	       sizeof(struct calling_interface_token) * tokens);
> -
> -	da_num_tokens += tokens;
> -}
> -
> -static void __init find_tokens(const struct dmi_header *dm, void *dummy)
> -{
> -	switch (dm->type) {
> -	case 0xd4: /* Indexed IO */
> -	case 0xd5: /* Protected Area Type 1 */
> -	case 0xd6: /* Protected Area Type 2 */
> -		break;
> -	case 0xda: /* Calling interface */
> -		parse_da_table(dm);
> -		break;
> -	}
> -}
> -
> -static int find_token_id(int tokenid)
> -{
> -	int i;
> -
> -	for (i = 0; i < da_num_tokens; i++) {
> -		if (da_tokens[i].tokenID == tokenid)
> -			return i;
> -	}
> -
> -	return -1;
> -}
> -
> -static int find_token_location(int tokenid)
> -{
> -	int id;
> -
> -	id = find_token_id(tokenid);
> -	if (id == -1)
> -		return -1;
> -
> -	return da_tokens[id].location;
> -}
> -
> -static struct calling_interface_buffer *
> -dell_send_request(struct calling_interface_buffer *buffer, int class,
> -		  int select)
> -{
> -	struct smi_cmd command;
> -
> -	command.magic = SMI_CMD_MAGIC;
> -	command.command_address = da_command_address;
> -	command.command_code = da_command_code;
> -	command.ebx = virt_to_phys(buffer);
> -	command.ecx = 0x42534931;
> -
> -	buffer->class = class;
> -	buffer->select = select;
> -
> -	dcdbas_smi_request(&command);
> -
> -	return buffer;
> -}
> -
>  static inline int dell_smi_error(int value)
>  {
>  	switch (value) {
> @@ -2122,13 +1983,6 @@ static int __init dell_init(void)
>  	/* find if this machine support other functions */
>  	dmi_check_system(dell_quirks);
>  
> -	dmi_walk(find_tokens, NULL);
> -
> -	if (!da_tokens)  {
> -		pr_info("Unable to find dmi tokens\n");
> -		return -ENODEV;
> -	}
> -
>  	ret = platform_driver_register(&platform_driver);
>  	if (ret)
>  		goto fail_platform_driver;
> @@ -2141,16 +1995,6 @@ static int __init dell_init(void)
>  	if (ret)
>  		goto fail_platform_device2;
>  
> -	/*
> -	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
> -	 * is passed to SMI handler.
> -	 */
> -	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> -	if (!buffer) {
> -		ret = -ENOMEM;
> -		goto fail_buffer;
> -	}
> -
>  	ret = dell_setup_rfkill();
>  
>  	if (ret) {
> @@ -2208,15 +2052,12 @@ static int __init dell_init(void)
>  fail_backlight:
>  	dell_cleanup_rfkill();
>  fail_rfkill:
> -	free_page((unsigned long)buffer);
> -fail_buffer:
>  	platform_device_del(platform_device);
>  fail_platform_device2:
>  	platform_device_put(platform_device);
>  fail_platform_device1:
>  	platform_driver_unregister(&platform_driver);
>  fail_platform_driver:
> -	kfree(da_tokens);
>  	return ret;
>  }
>  
> @@ -2232,8 +2073,6 @@ static void __exit dell_exit(void)
>  		platform_device_unregister(platform_device);
>  		platform_driver_unregister(&platform_driver);
>  	}
> -	kfree(da_tokens);
> -	free_page((unsigned long)buffer);
>  }
>  
>  /* dell-rbtn.c driver export functions which will not work correctly (and could
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> new file mode 100644
> index 0000000..260a32a
> --- /dev/null
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -0,0 +1,193 @@
> +/*
> + *  Common functions for kernel modules using Dell SMBIOS
> + *
> + *  Copyright (c) Red Hat <mjg@redhat.com>
> + *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
> + *
> + *  Based on documentation in the libsmbios package:
> + *  Copyright (C) 2005-2014 Dell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/gfp.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include "../../firmware/dcdbas.h"
> +#include "dell-smbios.h"
> +
> +struct calling_interface_structure {
> +	struct dmi_header header;
> +	u16 cmdIOAddress;
> +	u8 cmdIOCode;
> +	u32 supportedCmds;
> +	struct calling_interface_token tokens[];
> +} __packed;
> +
> +static DEFINE_MUTEX(buffer_mutex);
> +struct calling_interface_buffer *buffer;
> +EXPORT_SYMBOL_GPL(buffer);
> +
> +static int da_command_address;
> +static int da_command_code;
> +static int da_num_tokens;
> +struct calling_interface_token *da_tokens;
> +EXPORT_SYMBOL_GPL(da_tokens);
> +
> +void clear_buffer(void)
> +{
> +	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> +}
> +EXPORT_SYMBOL_GPL(clear_buffer);
> +
> +void get_buffer(void)
> +{
> +	mutex_lock(&buffer_mutex);
> +	clear_buffer();
> +}
> +EXPORT_SYMBOL_GPL(get_buffer);
> +
> +void release_buffer(void)
> +{
> +	mutex_unlock(&buffer_mutex);
> +}
> +EXPORT_SYMBOL_GPL(release_buffer);
> +
> +int find_token_id(int tokenid)
> +{
> +	int i;
> +
> +	for (i = 0; i < da_num_tokens; i++) {
> +		if (da_tokens[i].tokenID == tokenid)
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +EXPORT_SYMBOL_GPL(find_token_id);
> +
> +int find_token_location(int tokenid)
> +{
> +	int id;
> +
> +	id = find_token_id(tokenid);
> +	if (id == -1)
> +		return -1;
> +
> +	return da_tokens[id].location;
> +}
> +EXPORT_SYMBOL_GPL(find_token_location);
> +
> +struct calling_interface_buffer *
> +dell_send_request(struct calling_interface_buffer *buffer, int class,
> +		  int select)
> +{
> +	struct smi_cmd command;
> +
> +	command.magic = SMI_CMD_MAGIC;
> +	command.command_address = da_command_address;
> +	command.command_code = da_command_code;
> +	command.ebx = virt_to_phys(buffer);
> +	command.ecx = 0x42534931;
> +
> +	buffer->class = class;
> +	buffer->select = select;
> +
> +	dcdbas_smi_request(&command);
> +
> +	return buffer;
> +}
> +EXPORT_SYMBOL_GPL(dell_send_request);
> +
> +static void __init parse_da_table(const struct dmi_header *dm)
> +{
> +	/* Final token is a terminator, so we don't want to copy it */
> +	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
> +	struct calling_interface_token *new_da_tokens;
> +	struct calling_interface_structure *table =
> +		container_of(dm, struct calling_interface_structure, header);
> +
> +	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
> +	   6 bytes of entry */
> +
> +	if (dm->length < 17)
> +		return;
> +
> +	da_command_address = table->cmdIOAddress;
> +	da_command_code = table->cmdIOCode;
> +
> +	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> +				 sizeof(struct calling_interface_token),
> +				 GFP_KERNEL);
> +
> +	if (!new_da_tokens)
> +		return;
> +	da_tokens = new_da_tokens;
> +
> +	memcpy(da_tokens+da_num_tokens, table->tokens,
> +	       sizeof(struct calling_interface_token) * tokens);
> +
> +	da_num_tokens += tokens;
> +}
> +
> +static void __init find_tokens(const struct dmi_header *dm, void *dummy)
> +{
> +	switch (dm->type) {
> +	case 0xd4: /* Indexed IO */
> +	case 0xd5: /* Protected Area Type 1 */
> +	case 0xd6: /* Protected Area Type 2 */
> +		break;
> +	case 0xda: /* Calling interface */
> +		parse_da_table(dm);
> +		break;
> +	}
> +}
> +
> +static int __init dell_smbios_init(void)
> +{
> +	int ret;
> +
> +	dmi_walk(find_tokens, NULL);
> +
> +	if (!da_tokens)  {
> +		pr_info("Unable to find dmi tokens\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
> +	 * is passed to SMI handler.
> +	 */
> +	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto fail_buffer;
> +	}
> +
> +	return 0;
> +
> +fail_buffer:
> +	kfree(da_tokens);
> +	return ret;
> +}
> +
> +static void __exit dell_smbios_exit(void)
> +{
> +	kfree(da_tokens);
> +	free_page((unsigned long)buffer);
> +}
> +
> +subsys_initcall(dell_smbios_init);
> +module_exit(dell_smbios_exit);
> +
> +MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> +MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
> +MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> +MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> new file mode 100644
> index 0000000..00e03b2
> --- /dev/null
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -0,0 +1,51 @@
> +/*
> + *  Common functions for kernel modules using Dell SMBIOS
> + *
> + *  Copyright (c) Red Hat <mjg@redhat.com>
> + *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
> + *
> + *  Based on documentation in the libsmbios package:
> + *  Copyright (C) 2005-2014 Dell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#ifndef _DELL_SMBIOS_H_
> +#define _DELL_SMBIOS_H_
> +
> +/* This structure will be modified by the firmware when we enter
> + * system management mode, hence the volatiles */
> +
> +struct calling_interface_buffer {
> +	u16 class;
> +	u16 select;
> +	volatile u32 input[4];
> +	volatile u32 output[4];
> +} __packed;
> +
> +struct calling_interface_token {
> +	u16 tokenID;
> +	u16 location;
> +	union {
> +		u16 value;
> +		u16 stringlength;
> +	};
> +};

After patch 12/14 you do not need to define this struct in header file.

> +extern struct calling_interface_buffer *buffer;
> +extern struct calling_interface_token *da_tokens;

Better hide this variable in dell-smbios.c code ...

> +void clear_buffer(void);
> +void get_buffer(void);
> +void release_buffer(void);

... and let those functions to get parameter to buffer.

E.g. get_buffer will return buffer and other two functions will take
buffer parameter.

> +int find_token_id(int tokenid);
> +int find_token_location(int tokenid);
> +
> +struct calling_interface_buffer *
> +dell_send_request(struct calling_interface_buffer *buffer, int class,
> +		  int select);
> +#endif

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [next] | [standalone]

#1311463

Hi Pali,

Thanks for taking a look.

> > +struct calling_interface_token {
> > +	u16 tokenID;
> > +	u16 location;
> > +	union {
> > +		u16 value;
> > +		u16 stringlength;
> > +	};
> > +};
> 
> After patch 12/14 you do not need to define this struct in header file.

dell_smbios_find_token() returns a struct calling_interface_token *,
which can then be dereferenced by the caller.  See patch 13.

> > +extern struct calling_interface_buffer *buffer;
> > +extern struct calling_interface_token *da_tokens;
> 
> Better hide this variable in dell-smbios.c code ...

Patch 12 makes da_tokens static, but I believe you were referring to
buffer.

> > +void clear_buffer(void);
> > +void get_buffer(void);
> > +void release_buffer(void);
> 
> ... and let those functions to get parameter to buffer.
> 
> E.g. get_buffer will return buffer and other two functions will take
> buffer parameter.

You're right.  My original approach looked tempting because it reduces
the amount of changes required in dell-laptop, but on second thought, it
_assumes_ that users of this API would play nicely with the SMBIOS
buffer while your way _enforces_ it.

I will prepare a v2 including this suggestion within the next couple of
days.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1313027

> > +extern struct calling_interface_buffer *buffer;
> > +extern struct calling_interface_token *da_tokens;
> 
> Better hide this variable in dell-smbios.c code ...
> 
> > +void clear_buffer(void);
> > +void get_buffer(void);
> > +void release_buffer(void);
> 
> ... and let those functions to get parameter to buffer.
> 
> E.g. get_buffer will return buffer and other two functions will take
> buffer parameter.

Before I spam everyone with another set of 15 patches, I'd like to
discuss this a bit further.  There is no point in passing the buffer to
release_buffer(), because it only unlocks a mutex.  I also see no point
in passing the buffer to clear_buffer() and dell_send_request(), because
there is always just one buffer to operate on.

A total of four functions have something to do with the SMBIOS buffer:

  * get_buffer()
  * clear_buffer()
  * release_buffer()
  * dell_send_request()

This rework is a chance to make them all consistent, i.e. remove the
SMBIOS buffer from their argument lists.  This way we can "signal" this
API's users that there is only one SMBIOS buffer ever involved while
still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
also see little point in returning the buffer from dell_send_request()
as none of its callers in dell-laptop assign its return value to
anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
the code).

To sum up, I'd suggest that function prototypes could look like this:

    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);

What do you think?

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1314000

On Wednesday 20 January 2016 10:21:07 Michał Kępień wrote:
> > > +extern struct calling_interface_buffer *buffer;
> > > +extern struct calling_interface_token *da_tokens;
> > 
> > Better hide this variable in dell-smbios.c code ...
> > 
> > > +void clear_buffer(void);
> > > +void get_buffer(void);
> > > +void release_buffer(void);
> > 
> > ... and let those functions to get parameter to buffer.
> > 
> > E.g. get_buffer will return buffer and other two functions will take
> > buffer parameter.
> 
> Before I spam everyone with another set of 15 patches, I'd like to
> discuss this a bit further.  There is no point in passing the buffer to
> release_buffer(), because it only unlocks a mutex.  I also see no point
> in passing the buffer to clear_buffer() and dell_send_request(), because
> there is always just one buffer to operate on.
> 
> A total of four functions have something to do with the SMBIOS buffer:
> 
>   * get_buffer()
>   * clear_buffer()
>   * release_buffer()
>   * dell_send_request()
> 
> This rework is a chance to make them all consistent, i.e. remove the
> SMBIOS buffer from their argument lists.  This way we can "signal" this
> API's users that there is only one SMBIOS buffer ever involved while
> still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> also see little point in returning the buffer from dell_send_request()
> as none of its callers in dell-laptop assign its return value to
> anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> the code).
> 
> To sum up, I'd suggest that function prototypes could look like this:
> 
>     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);
> 
> What do you think?
> 

In other scenario functions should do something like this:

struct buf *buf_alloc(void);
buf_clear(struct buf *buf);
buf_free(struct buf *buf);
buf_do_something(struct buf *buf, ...);

But here I do not know how hard is to create alloc/free functions and
what is cost for creating that buffer in first 4GB memory...

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1314189

> > A total of four functions have something to do with the SMBIOS buffer:
> > 
> >   * get_buffer()
> >   * clear_buffer()
> >   * release_buffer()
> >   * dell_send_request()
> > 
> > This rework is a chance to make them all consistent, i.e. remove the
> > SMBIOS buffer from their argument lists.  This way we can "signal" this
> > API's users that there is only one SMBIOS buffer ever involved while
> > still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> > also see little point in returning the buffer from dell_send_request()
> > as none of its callers in dell-laptop assign its return value to
> > anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> > the code).
> > 
> > To sum up, I'd suggest that function prototypes could look like this:
> > 
> >     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);
> > 
> > What do you think?
> > 
> 
> In other scenario functions should do something like this:
> 
> struct buf *buf_alloc(void);
> buf_clear(struct buf *buf);
> buf_free(struct buf *buf);
> buf_do_something(struct buf *buf, ...);
> 
> But here I do not know how hard is to create alloc/free functions and
> what is cost for creating that buffer in first 4GB memory...

I'm guessing the cost is negligible, given that SMBIOS calls are not
present in any hot path.  Writing these functions is also pretty
straightforward, but the inconvenience of this approach is that it
forces the callers to do the error-checking for each buf_alloc() call.
It also seems pretty inefficient - notice we only need 36 bytes for the
calling interface buffer, yet we would be allocating a whole page in
each buf_alloc() call.

On the other hand, I believe returning a separate buffer for each
buf_alloc() caller makes it possible to drop buffer_mutex altogether.
Yet, the approach I suggested is more similar to what the Dell-supplied
dcdbas driver does internally (it manages a single, resizable buffer,
which is protected by a mutex and controllable from userspace through
sysfs), which is why I think it's a good idea to stick to that concept
for consistency.

As this patch series already touches a lot of code, I would like to
avoid changing the underlying concepts as much as possible.  If that's
okay with you, I'll post a v2 which includes your suggestion to make the
buffer pointer static while keeping the interface similar to the
original one.  If you would really like me to take a different path,
please let me know and I'll comply.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1314192

On Thursday 21 January 2016 14:06:03 Michał Kępień wrote:
> > > A total of four functions have something to do with the SMBIOS buffer:
> > > 
> > >   * get_buffer()
> > >   * clear_buffer()
> > >   * release_buffer()
> > >   * dell_send_request()
> > > 
> > > This rework is a chance to make them all consistent, i.e. remove the
> > > SMBIOS buffer from their argument lists.  This way we can "signal" this
> > > API's users that there is only one SMBIOS buffer ever involved while
> > > still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> > > also see little point in returning the buffer from dell_send_request()
> > > as none of its callers in dell-laptop assign its return value to
> > > anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> > > the code).
> > > 
> > > To sum up, I'd suggest that function prototypes could look like this:
> > > 
> > >     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);
> > > 
> > > What do you think?
> > > 
> > 
> > In other scenario functions should do something like this:
> > 
> > struct buf *buf_alloc(void);
> > buf_clear(struct buf *buf);
> > buf_free(struct buf *buf);
> > buf_do_something(struct buf *buf, ...);
> > 
> > But here I do not know how hard is to create alloc/free functions and
> > what is cost for creating that buffer in first 4GB memory...
> 
> I'm guessing the cost is negligible, given that SMBIOS calls are not
> present in any hot path.  Writing these functions is also pretty
> straightforward, but the inconvenience of this approach is that it
> forces the callers to do the error-checking for each buf_alloc() call.
> It also seems pretty inefficient - notice we only need 36 bytes for the
> calling interface buffer, yet we would be allocating a whole page in
> each buf_alloc() call.
> 
> On the other hand, I believe returning a separate buffer for each
> buf_alloc() caller makes it possible to drop buffer_mutex altogether.
> Yet, the approach I suggested is more similar to what the Dell-supplied
> dcdbas driver does internally (it manages a single, resizable buffer,
> which is protected by a mutex and controllable from userspace through
> sysfs), which is why I think it's a good idea to stick to that concept
> for consistency.
> 
> As this patch series already touches a lot of code, I would like to
> avoid changing the underlying concepts as much as possible.  If that's
> okay with you, I'll post a v2 which includes your suggestion to make the
> buffer pointer static while keeping the interface similar to the
> original one.  If you would really like me to take a different path,
> please let me know and I'll comply.
> 

Another idea:

What about passing struct calling_interface_buffer from caller allocated
memory (either from stack or kernel alloc) to dell-smbios which will
copy it into own buffer under 4GB and then pass it to dcdbas?

This will avoid to use that get/release function and there will be only
one send_request.

But I will let decision for API to other people as I do not know what
the best API to use here...

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1314196

> Another idea:
> 
> What about passing struct calling_interface_buffer from caller allocated
> memory (either from stack or kernel alloc) to dell-smbios which will
> copy it into own buffer under 4GB and then pass it to dcdbas?
> 
> This will avoid to use that get/release function and there will be only
> one send_request.

Well, yes, these two functions could then be ripped out, but the callers
would have to do the error checking on their own.  Of course that's not
a bad thing per se, but it changes the currently used concept.

> But I will let decision for API to other people as I do not know what
> the best API to use here...

In order to avoid delaying this any further, I'll post a v2 soon and
hopefully it'll be good enough for your Acked-by.  If it turns out more
people have misgivings about it, I'll adjust the code.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web