Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module
From: Pali RohÃr
Date: Sat Jan 16 2016 - 10:20:34 EST
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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@xxxxxxxxx>
> + * Copyright (c) 2014 Pali RohÃr <pali.rohar@xxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@xxxxxxxxx>");
> +MODULE_AUTHOR("Pali RohÃr <pali.rohar@xxxxxxxxx>");
> +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@xxxxxxxxxx>
> + * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@xxxxxxxxx>
> + * Copyright (c) 2014 Pali RohÃr <pali.rohar@xxxxxxxxx>
> + *
> + * 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@xxxxxxxxx