| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Aug 2008 23:46:37 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Matthew Garrett <mjg59@...f.ucam.org>
Cc: linux-kernel@...r.kernel.org, michael_e_brown@...l.com
Subject: Re: [PATCH 2/2] Add Dell laptop driver
On Sat, 16 Aug 2008 21:30:27 +0100 Matthew Garrett <mjg59@...f.ucam.org> wrote:
> This driver provides in-kernel control for the backlight and rfkill
> functionality on Dell laptops. It's based on the publically available
> information in the libsmbios package.
>
> Signed-off-by: Matthew Garrett <mjg@...hat.com>
>
> ---
>
> commit 472d8eed1891ba440f8e6ffde4787f6308390f7d
> Author: Matthew Garrett <mjg59@...00.(none)>
> Date: Sat Aug 16 21:12:51 2008 +0100
>
> Add Dell laptop driver
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a726f3b..c77eab3 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -475,4 +475,16 @@ config SGI_GRU_DEBUG
> This option enables addition debugging code for the SGI GRU driver. If
> you are unsure, say N.
>
> +config DELL_LAPTOP
> + tristate "Dell Laptop Extras (EXPERIMENTAL)"
> + depends on X86
> + depends on DCDBAS
> + depends on EXPERIMENTAL
> + depends on BACKLIGHT_CLASS_DEVICE
> + depends on RFKILL
> + default n
> + ---help---
> + This driver adds support for rfkill and backlight control to Dell
> + laptops.
whitespace gone wild.
> endif # MISC_DEVICES
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c6c13f6..865343d 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o
> obj-$(CONFIG_SGI_XP) += sgi-xp/
> obj-$(CONFIG_SGI_GRU) += sgi-gru/
> obj-$(CONFIG_HP_ILO) += hpilo.o
> +obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
> \ No newline at end of file
> diff --git a/drivers/misc/dell-laptop.c b/drivers/misc/dell-laptop.c
> new file mode 100644
> index 0000000..ab09a69
> --- /dev/null
> +++ b/drivers/misc/dell-laptop.c
> @@ -0,0 +1,356 @@
> +/*
> + * Driver for Dell laptop extras
> + *
> + * Copyright (c) Red Hat <mjg@...hat.com>
> + *
> + * Based on documentation in the libsmbios package, Copyright (C) 2005 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/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/backlight.h>
> +#include <linux/err.h>
> +#include <linux/dmi.h>
> +#include <linux/io.h>
> +#include <linux/rfkill.h>
> +#include "../firmware/dcdbas.h"
> +
> +struct calling_interface_buffer {
> + u16 class;
> + u16 select;
> + volatile u32 input[4];
> + volatile u32 output[4];
> +} __attribute__ ((packed));
checkpatch will direct you to a fine document.
If the `volatiles' are unavoidable then I'd suggest that both the
changelog and code comments explain the reason(s) for their addition.
> +struct calling_interface_token {
> + u16 tokenID;
> + u16 location;
> + union {
> + u16 value;
> + u16 stringlength;
> + };
> +};
> +
> +struct calling_interface_structure {
> + u8 type;
> + u8 length;
> + u16 handle;
Make the above three members a `struct dmi_header' ...
> + u16 cmdIOAddress;
> + u8 cmdIOCode;
> + u32 supportedCmds;
> + struct calling_interface_token tokens[];
> +} __attribute__ ((packed));
> +
> +static int da_command_address;
> +static int da_command_code;
> +static int da_num_tokens;
> +static struct calling_interface_token *da_tokens;
> +
> +static struct backlight_device *dell_backlight_device;
> +static struct rfkill *wifi_rfkill;
> +static struct rfkill *bluetooth_rfkill;
> +static struct rfkill *wwan_rfkill;
> +
> +static struct dmi_system_id __initdata dell_device_table[] = {
> + {
> + .ident = "Dell laptop",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
> + },
> + },
> + { }
> +};
> +
> +static void 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_structure *table =
> + (struct calling_interface_structure *)dm;
... and switch this to container_of().
> + /* 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;
> +
> + da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> + sizeof(struct calling_interface_token),
> + GFP_KERNEL);
> +
> + memcpy(da_tokens+da_num_tokens, table->tokens,
> + sizeof(struct calling_interface_token) * tokens);
krealloc() can fail.
> + da_num_tokens += tokens;
> +}
> +
> +static void find_tokens(const struct dmi_header *dm)
> +{
> + switch (dm->type) {
> + case 0xd4: /* Indexed IO */
> + break;
> + case 0xd5: /* Protected Area Type 1 */
> + break;
> + case 0xd6: /* Protected Area Type 2 */
> + break;
> + case 0xda: /* Calling interface */
> + parse_da_table(dm);
> + break;
> + }
> +}
> +
> +static int find_token_location(int tokenid)
> +{
> + int i;
> + for (i = 0; i < da_num_tokens; i++) {
> + if (da_tokens[i].tokenID == tokenid)
> + return da_tokens[i].location;
> + }
> + return -1;
> +}
> +
> +static struct calling_interface_buffer *dell_send_request(
> + struct calling_interface_buffer *buffer, int class, int select)
static struct calling_interface_buffer *
dell_send_request(struct calling_interface_buffer *buffer, int class,
int select)
would be more typical layout, although not vastly better.
> +{
> + struct smi_cmd *command = kzalloc(sizeof(struct smi_cmd), GFP_KERNEL);
> + 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;
Please put a blank line between end-of-locals and start-of-code.
Everywhere, all the time.
kzalloc() can fail and must be checked.
> + buffer->class = class;
> + buffer->select = select;
> +
> + dcdbas_smi_request(command);
> +
> + kfree(command);
Can we make `command' a local? It's pretty small. Perhaps
dcdbas_smi_request() (nee smi_request()) has some secret, undocumented
alignment restriction, similar to DMA?
> + return buffer;
> +}
> +
> +static int dell_rfkill_set(int radio, enum rfkill_state state)
> +{
> + struct calling_interface_buffer *buffer =
> + kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
> + int disable = (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1;
> + buffer->input[0] = (1 | (radio<<8) | (disable << 16));
> + dell_send_request(buffer, 17, 11);
> + kfree(buffer);
> + return 0;
> +}
kzalloc() can fail.
calling_interface_buffer is small enough to be a local.
coding-style.
> +static int dell_wifi_set(void *data, enum rfkill_state state)
> +{
> + return dell_rfkill_set(1, state);
> +}
> +
> +static int dell_bluetooth_set(void *data, enum rfkill_state state)
> +{
> + return dell_rfkill_set(2, state);
> +}
> +
> +static int dell_wwan_set(void *data, enum rfkill_state state)
> +{
> + return dell_rfkill_set(3, state);
> +}
> +
> +static int dell_rfkill_get(int bit, enum rfkill_state *state)
> +{
> + struct calling_interface_buffer *buffer =
> + kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
> + int status;
> + int new_state = RFKILL_STATE_HARD_BLOCKED;
> +
> + dell_send_request(buffer, 17, 11);
> + status = buffer->output[1];
kzalloc() can fail.
> + if (status & (1<<16))
> + new_state = RFKILL_STATE_SOFT_BLOCKED;
> +
> + if (status & (1<<bit))
> + *state = new_state;
> + else
> + *state = RFKILL_STATE_UNBLOCKED;
> +
> + kfree(buffer);
`buffer' is small enough to be a local.
> + return 0;
> +}
> +
> +static int dell_wifi_get(void *data, enum rfkill_state *state)
> +{
> + return dell_rfkill_get(17, state);
> +}
> +
> +static int dell_bluetooth_get(void *data, enum rfkill_state *state)
> +{
> + return dell_rfkill_get(18, state);
> +}
> +
> +static int dell_wwan_get(void *data, enum rfkill_state *state)
> +{
> + return dell_rfkill_get(19, state);
> +}
> +
> +static void dell_setup_rfkill(void)
> +{
> + struct calling_interface_buffer *buffer =
> + kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
ditto, ditto
> + int status;
> +
> + dell_send_request(buffer, 17, 11);
> + status = buffer->output[1];
> +
> + if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {
Where does the reader go to find the meaning of these bits?
(code comment needed)
> + wifi_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WLAN);
> + wifi_rfkill->name = "dell-wifi";
> + wifi_rfkill->toggle_radio = dell_wifi_set;
> + wifi_rfkill->get_state = dell_wifi_get;
> + rfkill_register(wifi_rfkill);
> + }
> +
> + if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
> + bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
> + bluetooth_rfkill->name = "dell-bluetooth";
> + bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
> + bluetooth_rfkill->get_state = dell_bluetooth_get;
> + rfkill_register(bluetooth_rfkill);
> + }
> +
> + if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
> + wwan_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WWAN);
> + wwan_rfkill->name = "dell-wwan";
> + wwan_rfkill->toggle_radio = dell_wwan_set;
> + wwan_rfkill->get_state = dell_wwan_get;
> + rfkill_register(wwan_rfkill);
> + }
dittoes
> + kfree(buffer);
> +}
> +
> +static int dell_send_intensity(struct backlight_device *bd)
> +{
> + int intensity = bd->props.brightness;
> + struct calling_interface_buffer *buffer =
> + kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
blah
> + buffer->input[0] = find_token_location(0x7d);
> + buffer->input[1] = intensity;
I don't think we really needed local variable `intensity'. I guess it
has some commentary value.
> + if (buffer->input[0] == -1) {
> + kfree(buffer);
> + return -ENODEV;
> + }
> +
> + dell_send_request(buffer, 1, 1);
> + dell_send_request(buffer, 1, 2);
> +
> + kfree(buffer);
> +
> + return 0;
> +}
> +
> +static int dell_get_intensity(struct backlight_device *bd)
> +{
> + struct calling_interface_buffer *buffer =
> + kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
ditto, ditto
> + int brightness;
> +
> + buffer->input[0] = find_token_location(0x7d);
> +
> + if (buffer->input[0] == -1) {
> + kfree(buffer);
> + return -ENODEV;
> + }
> +
> + dell_send_request(buffer, 0, 1);
> +
> + brightness = buffer->output[1];
> +
> + kfree(buffer);
> +
> + return brightness;
> +}
> +
> +static struct backlight_ops dell_ops = {
> + .get_brightness = dell_get_intensity,
> + .update_status = dell_send_intensity,
> +};
> +
> +static int __init dell_init(void)
> +{
> + struct calling_interface_buffer *buffer;
> + int max_intensity = 0;
> +
> + if (!dmi_check_system(dell_device_table))
> + return -ENODEV;
> +
> + dmi_walk(find_tokens);
> +
> + if (!da_tokens)
> + return -ENODEV;
> +
> + buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
ditto
> + buffer->input[0] = find_token_location(0x7d);
> +
> + if (buffer->input[0] != -1) {
> + dell_send_request(buffer, 2, 1);
> + max_intensity = buffer->output[3];
> + }
> +
> + kfree(buffer);
> +
> + if (max_intensity) {
> + dell_backlight_device = backlight_device_register(
> + "dell_backlight",
> + NULL, NULL,
> + &dell_ops);
> +
> + if (IS_ERR(dell_backlight_device)) {
> + dell_backlight_device = NULL;
> + goto out;
Should propagate PTR_ERR(dell_backlight_device) back to caller.
> + }
> +
> + dell_backlight_device->props.max_brightness = max_intensity;
> + dell_backlight_device->props.brightness =
> + dell_get_intensity(dell_backlight_device);
> + backlight_update_status(dell_backlight_device);
> + }
> +
> + dell_setup_rfkill();
> +
> +out:
> + return 0;
> +}
> +
> +static void __exit dell_exit(void)
> +{
> + if (dell_backlight_device)
> + backlight_device_unregister(dell_backlight_device);
backlight_device_unregister(NULL) is legal
> + if (wifi_rfkill)
> + rfkill_unregister(wifi_rfkill);
> + if (bluetooth_rfkill)
> + rfkill_unregister(bluetooth_rfkill);
> + if (wwan_rfkill)
> + rfkill_unregister(wwan_rfkill);
> +}
> +
> +module_init(dell_init);
> +module_exit(dell_exit);
> +
> +MODULE_AUTHOR("Matthew Garrett <mjg@...hat.com>");
> +MODULE_DESCRIPTION("Dell laptop driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("dmi:*svnDellInc.:*:ct8:*");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists