* [patch 0/8] ACPI Video various cleanups & fixes
@ 2007-11-05 16:43 Dmitry Torokhov
2007-11-05 16:43 ` [patch 1/8] ACPI: video - fit input device into sysfs tree Dmitry Torokhov
` (8 more replies)
0 siblings, 9 replies; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
Hi Len, Rui,
Here is a group of cleanups and patches to the acpi video driver.
Originally I just wanted to fix sysfs placement of input devices
created by the driver, but then I saw some more potential issues.
The patches are against Linus's tree, please let me know if you
want them redone against something else.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 1/8] ACPI: video - fit input device into sysfs tree
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-05 16:43 ` [patch 2/8] ACPI: video - add missing input_free_device() Dmitry Torokhov
` (7 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-input-set-parent.patch --]
[-- Type: text/plain, Size: 944 bytes --]
ACPI: video - fit input device into sysfs tree
Properly set up parent on input device registered by the video driver.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -1961,6 +1961,7 @@ static int acpi_video_bus_add(struct acp
input->phys = video->phys;
input->id.bustype = BUS_HOST;
input->id.product = 0x06;
+ input->dev.parent = &device->dev;
input->evbit[0] = BIT(EV_KEY);
set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
set_bit(KEY_VIDEO_NEXT, input->keybit);
@@ -1990,7 +1991,7 @@ static int acpi_video_bus_add(struct acp
video->flags.rom ? "yes" : "no",
video->flags.post ? "yes" : "no");
- end:
+ end:
if (result)
kfree(video);
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 2/8] ACPI: video - add missing input_free_device()
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
2007-11-05 16:43 ` [patch 1/8] ACPI: video - fit input device into sysfs tree Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:00 ` Len Brown
2007-11-05 16:43 ` [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe() Dmitry Torokhov
` (6 subsequent siblings)
8 siblings, 2 replies; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-fix-error-registering.patch --]
[-- Type: text/plain, Size: 3671 bytes --]
ACPI: video - add missing input_free_device()
If input_register_device() fails input_free_device() must
be called to release memory allocated for the device.
Also consolidate error handling in acpi_bus_video_add()
and handle input_allocate_device() failures.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 71 +++++++++++++++++++++++++--------------------------
1 file changed, 35 insertions(+), 36 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -1897,14 +1897,10 @@ static void acpi_video_device_notify(acp
static int instance;
static int acpi_video_bus_add(struct acpi_device *device)
{
- int result = 0;
- acpi_status status = 0;
- struct acpi_video_bus *video = NULL;
+ acpi_status status;
+ struct acpi_video_bus *video;
struct input_dev *input;
-
-
- if (!device)
- return -EINVAL;
+ int error;
video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
if (!video)
@@ -1923,13 +1919,13 @@ static int acpi_video_bus_add(struct acp
acpi_driver_data(device) = video;
acpi_video_bus_find_cap(video);
- result = acpi_video_bus_check(video);
- if (result)
- goto end;
-
- result = acpi_video_bus_add_fs(device);
- if (result)
- goto end;
+ error = acpi_video_bus_check(video);
+ if (error)
+ goto err_free_video;
+
+ error = acpi_video_bus_add_fs(device);
+ if (error)
+ goto err_free_video;
init_MUTEX(&video->sem);
INIT_LIST_HEAD(&video->video_device_list);
@@ -1943,16 +1939,15 @@ static int acpi_video_bus_add(struct acp
if (ACPI_FAILURE(status)) {
ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
"Error installing notify handler\n"));
- acpi_video_bus_stop_devices(video);
- acpi_video_bus_put_devices(video);
- kfree(video->attached_array);
- acpi_video_bus_remove_fs(device);
- result = -ENODEV;
- goto end;
+ error = -ENODEV;
+ goto err_stop_video;
}
-
video->input = input = input_allocate_device();
+ if (!input) {
+ error = -ENOMEM;
+ goto err_uninstall_notify;
+ }
snprintf(video->phys, sizeof(video->phys),
"%s/video/input0", acpi_device_hid(video->device));
@@ -1972,18 +1967,10 @@ static int acpi_video_bus_add(struct acp
set_bit(KEY_BRIGHTNESS_ZERO, input->keybit);
set_bit(KEY_DISPLAY_OFF, input->keybit);
set_bit(KEY_UNKNOWN, input->keybit);
- result = input_register_device(input);
- if (result) {
- acpi_remove_notify_handler(video->device->handle,
- ACPI_DEVICE_NOTIFY,
- acpi_video_bus_notify);
- acpi_video_bus_stop_devices(video);
- acpi_video_bus_put_devices(video);
- kfree(video->attached_array);
- acpi_video_bus_remove_fs(device);
- goto end;
- }
+ error = input_register_device(input);
+ if (error)
+ goto err_free_input_dev;
printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n",
ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
@@ -1991,11 +1978,23 @@ static int acpi_video_bus_add(struct acp
video->flags.rom ? "yes" : "no",
video->flags.post ? "yes" : "no");
- end:
- if (result)
- kfree(video);
+ return 0;
+
+ err_free_input_dev:
+ input_free_device(input);
+ err_uninstall_notify:
+ acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+ acpi_video_bus_notify);
+ err_stop_video:
+ acpi_video_bus_stop_devices(video);
+ acpi_video_bus_put_devices(video);
+ kfree(video->attached_array);
+ acpi_video_bus_remove_fs(device);
+ err_free_video:
+ kfree(video);
+ acpi_driver_data(device) = NULL;
- return result;
+ return error;
}
static int acpi_video_bus_remove(struct acpi_device *device, int type)
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe()
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
2007-11-05 16:43 ` [patch 1/8] ACPI: video - fit input device into sysfs tree Dmitry Torokhov
2007-11-05 16:43 ` [patch 2/8] ACPI: video - add missing input_free_device() Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:46 ` Len Brown
2007-11-05 16:43 ` [patch 4/8] ACPI: video - convert semaphore to a mutex Dmitry Torokhov
` (5 subsequent siblings)
8 siblings, 2 replies; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-dont-use-list-safe.patch --]
[-- Type: text/plain, Size: 4479 bytes --]
ACPI: video - remove unsafe uses of list_for_each_safe()
list_for_each_safe() only protects list from list alterations
performed by the same thread. One still needs to implement
proper locking when list is being accessed from several threads.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 71 ++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 37 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -1462,12 +1462,14 @@ acpi_video_bus_get_one_device(struct acp
static void acpi_video_device_rebind(struct acpi_video_bus *video)
{
- struct list_head *node, *next;
- list_for_each_safe(node, next, &video->video_device_list) {
- struct acpi_video_device *dev =
- container_of(node, struct acpi_video_device, entry);
+ struct acpi_video_device *dev;
+
+ down(&video->sem);
+
+ list_for_each_entry(dev, &video->video_device_list, entry)
acpi_video_device_bind(video, dev);
- }
+
+ up(&video->sem);
}
/*
@@ -1592,30 +1594,33 @@ static int acpi_video_device_enumerate(s
static int acpi_video_switch_output(struct acpi_video_bus *video, int event)
{
- struct list_head *node, *next;
+ struct list_head *node;
struct acpi_video_device *dev = NULL;
struct acpi_video_device *dev_next = NULL;
struct acpi_video_device *dev_prev = NULL;
unsigned long state;
int status = 0;
+ down(&video->sem);
- list_for_each_safe(node, next, &video->video_device_list) {
+ list_for_each(node, &video->video_device_list) {
dev = container_of(node, struct acpi_video_device, entry);
status = acpi_video_device_get_state(dev, &state);
if (state & 0x2) {
- dev_next =
- container_of(node->next, struct acpi_video_device,
- entry);
- dev_prev =
- container_of(node->prev, struct acpi_video_device,
- entry);
+ dev_next = container_of(node->next,
+ struct acpi_video_device, entry);
+ dev_prev = container_of(node->prev,
+ struct acpi_video_device, entry);
goto out;
}
}
+
dev_next = container_of(node->next, struct acpi_video_device, entry);
dev_prev = container_of(node->prev, struct acpi_video_device, entry);
- out:
+
+ out:
+ up(&video->sem);
+
switch (event) {
case ACPI_VIDEO_NOTIFY_CYCLE:
case ACPI_VIDEO_NOTIFY_NEXT_OUTPUT:
@@ -1691,24 +1696,17 @@ acpi_video_bus_get_devices(struct acpi_v
struct acpi_device *device)
{
int status = 0;
- struct list_head *node, *next;
-
+ struct acpi_device *dev;
acpi_video_device_enumerate(video);
- list_for_each_safe(node, next, &device->children) {
- struct acpi_device *dev =
- list_entry(node, struct acpi_device, node);
-
- if (!dev)
- continue;
+ list_for_each_entry(dev, &device->children, node) {
status = acpi_video_bus_get_one_device(dev, video);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status, "Cant attach device"));
continue;
}
-
}
return status;
}
@@ -1724,9 +1722,6 @@ static int acpi_video_bus_put_one_device
video = device->video;
- down(&video->sem);
- list_del(&device->entry);
- up(&video->sem);
acpi_video_device_remove_fs(device->dev);
status = acpi_remove_notify_handler(device->dev->handle,
@@ -1734,32 +1729,34 @@ static int acpi_video_bus_put_one_device
acpi_video_device_notify);
backlight_device_unregister(device->backlight);
video_output_unregister(device->output_dev);
+
return 0;
}
static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
{
int status;
- struct list_head *node, *next;
+ struct acpi_video_device *dev, *next;
+ down(&video->sem);
- list_for_each_safe(node, next, &video->video_device_list) {
- struct acpi_video_device *data =
- list_entry(node, struct acpi_video_device, entry);
- if (!data)
- continue;
+ list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
- status = acpi_video_bus_put_one_device(data);
+ status = acpi_video_bus_put_one_device(dev);
if (ACPI_FAILURE(status))
printk(KERN_WARNING PREFIX
"hhuuhhuu bug in acpi video driver.\n");
- if (data->brightness)
- kfree(data->brightness->levels);
- kfree(data->brightness);
- kfree(data);
+ if (dev->brightness) {
+ kfree(dev->brightness->levels);
+ kfree(dev->brightness);
+ }
+ list_del(&dev->entry);
+ kfree(dev);
}
+ up(&video->sem);
+
return 0;
}
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 4/8] ACPI: video - convert semaphore to a mutex
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (2 preceding siblings ...)
2007-11-05 16:43 ` [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe() Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-05 16:43 ` [patch 5/8] ACPI: video - simplify handling of attached devices Dmitry Torokhov
` (4 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-sem-to-mutex.patch --]
[-- Type: text/plain, Size: 2814 bytes --]
ACPI: video - convert semaphore to a mutex
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -29,6 +29,7 @@
#include <linux/init.h>
#include <linux/types.h>
#include <linux/list.h>
+#include <linux/mutex.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/input.h>
@@ -135,8 +136,8 @@ struct acpi_video_bus {
u8 attached_count;
struct acpi_video_bus_cap cap;
struct acpi_video_bus_flags flags;
- struct semaphore sem;
struct list_head video_device_list;
+ struct mutex device_list_lock; /* protects video_device_list */
struct proc_dir_entry *dir;
struct input_dev *input;
char phys[32]; /* for input device */
@@ -1436,9 +1437,9 @@ acpi_video_bus_get_one_device(struct acp
return -ENODEV;
}
- down(&video->sem);
+ mutex_lock(&video->device_list_lock);
list_add_tail(&data->entry, &video->video_device_list);
- up(&video->sem);
+ mutex_unlock(&video->device_list_lock);
acpi_video_device_add_fs(device);
@@ -1464,12 +1465,12 @@ static void acpi_video_device_rebind(str
{
struct acpi_video_device *dev;
- down(&video->sem);
+ mutex_lock(&video->device_list_lock);
list_for_each_entry(dev, &video->video_device_list, entry)
acpi_video_device_bind(video, dev);
- up(&video->sem);
+ mutex_unlock(&video->device_list_lock);
}
/*
@@ -1601,7 +1602,7 @@ static int acpi_video_switch_output(stru
unsigned long state;
int status = 0;
- down(&video->sem);
+ mutex_lock(&video->device_list_lock);
list_for_each(node, &video->video_device_list) {
dev = container_of(node, struct acpi_video_device, entry);
@@ -1619,7 +1620,7 @@ static int acpi_video_switch_output(stru
dev_prev = container_of(node->prev, struct acpi_video_device, entry);
out:
- up(&video->sem);
+ mutex_unlock(&video->device_list_lock);
switch (event) {
case ACPI_VIDEO_NOTIFY_CYCLE:
@@ -1738,7 +1739,7 @@ static int acpi_video_bus_put_devices(st
int status;
struct acpi_video_device *dev, *next;
- down(&video->sem);
+ mutex_lock(&video->device_list_lock);
list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
@@ -1755,7 +1756,7 @@ static int acpi_video_bus_put_devices(st
kfree(dev);
}
- up(&video->sem);
+ mutex_unlock(&video->device_list_lock);
return 0;
}
@@ -1924,7 +1925,7 @@ static int acpi_video_bus_add(struct acp
if (error)
goto err_free_video;
- init_MUTEX(&video->sem);
+ mutex_init(&video->device_list_lock);
INIT_LIST_HEAD(&video->video_device_list);
acpi_video_bus_get_devices(video, device);
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 5/8] ACPI: video - simplify handling of attached devices
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (3 preceding siblings ...)
2007-11-05 16:43 ` [patch 4/8] ACPI: video - convert semaphore to a mutex Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-05 16:43 ` [patch 6/8] ACPI: video - properly handle errors when registering proc elements Dmitry Torokhov
` (3 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-attached-confusion.patch --]
[-- Type: text/plain, Size: 3984 bytes --]
ACPI: video - simplify handling of attached devices
The old code needlessly stored invalid entries in attached_array list.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 58 +++++++++++++++++++++++----------------------------
1 file changed, 27 insertions(+), 31 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -56,8 +56,6 @@
#define ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS 0x88
#define ACPI_VIDEO_NOTIFY_DISPLAY_OFF 0x89
-#define ACPI_VIDEO_HEAD_INVALID (~0u - 1)
-#define ACPI_VIDEO_HEAD_END (~0u)
#define MAX_NAME_LEN 20
#define ACPI_VIDEO_DISPLAY_CRT 1
@@ -1359,11 +1357,15 @@ static int acpi_video_bus_remove_fs(stru
static struct acpi_video_device_attrib*
acpi_video_get_device_attr(struct acpi_video_bus *video, unsigned long device_id)
{
- int count;
+ struct acpi_video_enumerated_device *ids;
+ int i;
+
+ for (i = 0; i < video->attached_count; i++) {
+ ids = &video->attached_array[i];
+ if ((ids->value.int_val & 0xffff) == device_id)
+ return &ids->value.attrib;
+ }
- for(count = 0; count < video->attached_count; count++)
- if((video->attached_array[count].value.int_val & 0xffff) == device_id)
- return &(video->attached_array[count].value.attrib);
return NULL;
}
@@ -1490,20 +1492,16 @@ static void
acpi_video_device_bind(struct acpi_video_bus *video,
struct acpi_video_device *device)
{
+ struct acpi_video_enumerated_device *ids;
int i;
-#define IDS_VAL(i) video->attached_array[i].value.int_val
-#define IDS_BIND(i) video->attached_array[i].bind_info
-
- for (i = 0; IDS_VAL(i) != ACPI_VIDEO_HEAD_INVALID &&
- i < video->attached_count; i++) {
- if (device->device_id == (IDS_VAL(i) & 0xffff)) {
- IDS_BIND(i) = device;
+ for (i = 0; i < video->attached_count; i++) {
+ ids = &video->attached_array[i];
+ if (device->device_id == (ids->value.int_val & 0xffff)) {
+ ids->bind_info = device;
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "device_bind %d\n", i));
}
}
-#undef IDS_VAL
-#undef IDS_BIND
}
/*
@@ -1522,7 +1520,7 @@ static int acpi_video_device_enumerate(s
int status;
int count;
int i;
- struct acpi_video_enumerated_device *active_device_list;
+ struct acpi_video_enumerated_device *active_list;
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *dod = NULL;
union acpi_object *obj;
@@ -1543,13 +1541,10 @@ static int acpi_video_device_enumerate(s
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d video heads in _DOD\n",
dod->package.count));
- active_device_list = kmalloc((1 +
- dod->package.count) *
- sizeof(struct
- acpi_video_enumerated_device),
- GFP_KERNEL);
-
- if (!active_device_list) {
+ active_list = kcalloc(1 + dod->package.count,
+ sizeof(struct acpi_video_enumerated_device),
+ GFP_KERNEL);
+ if (!active_list) {
status = -ENOMEM;
goto out;
}
@@ -1559,23 +1554,24 @@ static int acpi_video_device_enumerate(s
obj = &dod->package.elements[i];
if (obj->type != ACPI_TYPE_INTEGER) {
- printk(KERN_ERR PREFIX "Invalid _DOD data\n");
- active_device_list[i].value.int_val =
- ACPI_VIDEO_HEAD_INVALID;
+ printk(KERN_ERR PREFIX
+ "Invalid _DOD data in element %d\n", i);
+ continue;
}
- active_device_list[i].value.int_val = obj->integer.value;
- active_device_list[i].bind_info = NULL;
+
+ active_list[count].value.int_val = obj->integer.value;
+ active_list[count].bind_info = NULL;
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "dod element[%d] = %d\n", i,
(int)obj->integer.value));
count++;
}
- active_device_list[count].value.int_val = ACPI_VIDEO_HEAD_END;
kfree(video->attached_array);
- video->attached_array = active_device_list;
+ video->attached_array = active_list;
video->attached_count = count;
- out:
+
+ out:
kfree(buffer.pointer);
return status;
}
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (4 preceding siblings ...)
2007-11-05 16:43 ` [patch 5/8] ACPI: video - simplify handling of attached devices Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-05 16:43 ` [patch 7/8] ACPI: video - more cleanups Dmitry Torokhov
` (2 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-proc-error-handling.patch --]
[-- Type: text/plain, Size: 9681 bytes --]
ACPI: video - properly handle errors when registering proc elements
Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
properly unwind proc creation after error.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 230 +++++++++++++++++++++++++--------------------------
1 file changed, 115 insertions(+), 115 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -967,87 +967,90 @@ acpi_video_device_EDID_open_fs(struct in
static int acpi_video_device_add_fs(struct acpi_device *device)
{
- struct proc_dir_entry *entry = NULL;
+ struct proc_dir_entry *entry, *device_dir;
struct acpi_video_device *vid_dev;
-
- if (!device)
- return -ENODEV;
-
vid_dev = acpi_driver_data(device);
if (!vid_dev)
return -ENODEV;
- if (!acpi_device_dir(device)) {
- acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
- vid_dev->video->dir);
- if (!acpi_device_dir(device))
- return -ENODEV;
- acpi_device_dir(device)->owner = THIS_MODULE;
- }
+ device_dir = proc_mkdir(acpi_device_bid(device),
+ vid_dev->video->dir);
+ if (!device_dir)
+ return -ENOMEM;
+
+ device_dir->owner = THIS_MODULE;
/* 'info' [R] */
- entry = create_proc_entry("info", S_IRUGO, acpi_device_dir(device));
+ entry = create_proc_entry("info", S_IRUGO, device_dir);
if (!entry)
- return -ENODEV;
- else {
- entry->proc_fops = &acpi_video_device_info_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_dir;
+
+ entry->proc_fops = &acpi_video_device_info_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'state' [R/W] */
- entry =
- create_proc_entry("state", S_IFREG | S_IRUGO | S_IWUSR,
- acpi_device_dir(device));
+ entry = create_proc_entry("state", S_IFREG | S_IRUGO | S_IWUSR,
+ device_dir);
if (!entry)
- return -ENODEV;
- else {
- acpi_video_device_state_fops.write = acpi_video_device_write_state;
- entry->proc_fops = &acpi_video_device_state_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_info;
+
+ acpi_video_device_state_fops.write = acpi_video_device_write_state;
+ entry->proc_fops = &acpi_video_device_state_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'brightness' [R/W] */
- entry =
- create_proc_entry("brightness", S_IFREG | S_IRUGO | S_IWUSR,
- acpi_device_dir(device));
+ entry = create_proc_entry("brightness", S_IFREG | S_IRUGO | S_IWUSR,
+ device_dir);
if (!entry)
- return -ENODEV;
- else {
- acpi_video_device_brightness_fops.write = acpi_video_device_write_brightness;
- entry->proc_fops = &acpi_video_device_brightness_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_state;
+
+ acpi_video_device_brightness_fops.write =
+ acpi_video_device_write_brightness;
+ entry->proc_fops = &acpi_video_device_brightness_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'EDID' [R] */
- entry = create_proc_entry("EDID", S_IRUGO, acpi_device_dir(device));
+ entry = create_proc_entry("EDID", S_IRUGO, device_dir);
if (!entry)
- return -ENODEV;
- else {
- entry->proc_fops = &acpi_video_device_EDID_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_brightness;
+ entry->proc_fops = &acpi_video_device_EDID_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
+
+ acpi_device_dir(device) = device_dir;
return 0;
+
+ err_remove_brightness:
+ remove_proc_entry("brightness", device_dir);
+ err_remove_state:
+ remove_proc_entry("state", device_dir);
+ err_remove_info:
+ remove_proc_entry("info", device_dir);
+ err_remove_dir:
+ remove_proc_entry(acpi_device_bid(device), vid_dev->video->dir);
+ return -ENOMEM;
}
static int acpi_video_device_remove_fs(struct acpi_device *device)
{
struct acpi_video_device *vid_dev;
+ struct proc_dir_entry *device_dir;
vid_dev = acpi_driver_data(device);
if (!vid_dev || !vid_dev->video || !vid_dev->video->dir)
return -ENODEV;
- if (acpi_device_dir(device)) {
- remove_proc_entry("info", acpi_device_dir(device));
- remove_proc_entry("state", acpi_device_dir(device));
- remove_proc_entry("brightness", acpi_device_dir(device));
- remove_proc_entry("EDID", acpi_device_dir(device));
+ device_dir = acpi_device_dir(device);
+ if (device_dir) {
+ remove_proc_entry("info", device_dir);
+ remove_proc_entry("state", device_dir);
+ remove_proc_entry("brightness", device_dir);
+ remove_proc_entry("EDID", device_dir);
remove_proc_entry(acpi_device_bid(device), vid_dev->video->dir);
acpi_device_dir(device) = NULL;
}
@@ -1254,94 +1257,91 @@ acpi_video_bus_write_DOS(struct file *fi
static int acpi_video_bus_add_fs(struct acpi_device *device)
{
- struct proc_dir_entry *entry = NULL;
- struct acpi_video_bus *video;
+ struct acpi_video_bus *video = acpi_driver_data(device);
+ struct proc_dir_entry *device_dir;
+ struct proc_dir_entry *entry;
+ device_dir = proc_mkdir(acpi_device_bid(device), acpi_video_dir);
+ if (!device_dir)
+ return -ENOMEM;
- video = acpi_driver_data(device);
-
- if (!acpi_device_dir(device)) {
- acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
- acpi_video_dir);
- if (!acpi_device_dir(device))
- return -ENODEV;
- video->dir = acpi_device_dir(device);
- acpi_device_dir(device)->owner = THIS_MODULE;
- }
+ device_dir->owner = THIS_MODULE;
/* 'info' [R] */
- entry = create_proc_entry("info", S_IRUGO, acpi_device_dir(device));
+ entry = create_proc_entry("info", S_IRUGO, device_dir);
if (!entry)
- return -ENODEV;
- else {
- entry->proc_fops = &acpi_video_bus_info_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_dir;
+
+ entry->proc_fops = &acpi_video_bus_info_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'ROM' [R] */
- entry = create_proc_entry("ROM", S_IRUGO, acpi_device_dir(device));
+ entry = create_proc_entry("ROM", S_IRUGO, device_dir);
if (!entry)
- return -ENODEV;
- else {
- entry->proc_fops = &acpi_video_bus_ROM_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_info;
+
+ entry->proc_fops = &acpi_video_bus_ROM_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'POST_info' [R] */
- entry =
- create_proc_entry("POST_info", S_IRUGO, acpi_device_dir(device));
+ entry = create_proc_entry("POST_info", S_IRUGO, device_dir);
if (!entry)
- return -ENODEV;
- else {
- entry->proc_fops = &acpi_video_bus_POST_info_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_rom;
+
+ entry->proc_fops = &acpi_video_bus_POST_info_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'POST' [R/W] */
- entry =
- create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
- acpi_device_dir(device));
+ entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
+ device_dir);
if (!entry)
- return -ENODEV;
- else {
- acpi_video_bus_POST_fops.write = acpi_video_bus_write_POST;
- entry->proc_fops = &acpi_video_bus_POST_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_post_info;
+
+ acpi_video_bus_POST_fops.write = acpi_video_bus_write_POST;
+ entry->proc_fops = &acpi_video_bus_POST_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
/* 'DOS' [R/W] */
- entry =
- create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
- acpi_device_dir(device));
+ entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
+ device_dir);
if (!entry)
- return -ENODEV;
- else {
- acpi_video_bus_DOS_fops.write = acpi_video_bus_write_DOS;
- entry->proc_fops = &acpi_video_bus_DOS_fops;
- entry->data = acpi_driver_data(device);
- entry->owner = THIS_MODULE;
- }
+ goto err_remove_post;
+
+ acpi_video_bus_DOS_fops.write = acpi_video_bus_write_DOS;
+ entry->proc_fops = &acpi_video_bus_DOS_fops;
+ entry->data = acpi_driver_data(device);
+ entry->owner = THIS_MODULE;
+ video->dir = acpi_device_dir(device) = device_dir;
return 0;
+
+ err_remove_post:
+ remove_proc_entry("POST", device_dir);
+ err_remove_post_info:
+ remove_proc_entry("POST_info", device_dir);
+ err_remove_rom:
+ remove_proc_entry("ROM", device_dir);
+ err_remove_info:
+ remove_proc_entry("info", device_dir);
+ err_remove_dir:
+ remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
+ return -ENOMEM;
}
static int acpi_video_bus_remove_fs(struct acpi_device *device)
{
- struct acpi_video_bus *video;
-
-
- video = acpi_driver_data(device);
+ struct proc_dir_entry *device_dir = acpi_device_dir(device);
- if (acpi_device_dir(device)) {
- remove_proc_entry("info", acpi_device_dir(device));
- remove_proc_entry("ROM", acpi_device_dir(device));
- remove_proc_entry("POST_info", acpi_device_dir(device));
- remove_proc_entry("POST", acpi_device_dir(device));
- remove_proc_entry("DOS", acpi_device_dir(device));
+ if (device_dir) {
+ remove_proc_entry("info", device_dir);
+ remove_proc_entry("ROM", device_dir);
+ remove_proc_entry("POST_info", device_dir);
+ remove_proc_entry("POST", device_dir);
+ remove_proc_entry("DOS", device_dir);
remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
acpi_device_dir(device) = NULL;
}
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 7/8] ACPI: video - more cleanups
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (5 preceding siblings ...)
2007-11-05 16:43 ` [patch 6/8] ACPI: video - properly handle errors when registering proc elements Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-05 16:43 ` [patch 8/8] ACPI: video - fix permissions on some proc entries Dmitry Torokhov
2007-11-14 7:17 ` [patch 0/8] ACPI Video various cleanups & fixes Zhang Rui
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-cleanup.patch --]
[-- Type: text/plain, Size: 35427 bytes --]
ACPI: video - more cleanups
Remove unneeded checks and initializations, implement proper
unwinding after errors in initialization code, get rid of
unneeded casts, adjust formatting.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 778 +++++++++++++++++++++++++--------------------------
1 file changed, 388 insertions(+), 390 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -74,8 +74,8 @@ static int acpi_video_bus_add(struct acp
static int acpi_video_bus_remove(struct acpi_device *device, int type);
static const struct acpi_device_id video_device_ids[] = {
- {ACPI_VIDEO_HID, 0},
- {"", 0},
+ { ACPI_VIDEO_HID, 0 },
+ { "", 0 },
};
MODULE_DEVICE_TABLE(acpi, video_device_ids);
@@ -86,7 +86,7 @@ static struct acpi_driver acpi_video_bus
.ops = {
.add = acpi_video_bus_add,
.remove = acpi_video_bus_remove,
- },
+ },
};
struct acpi_video_bus_flags {
@@ -97,26 +97,28 @@ struct acpi_video_bus_flags {
};
struct acpi_video_bus_cap {
- u8 _DOS:1; /*Enable/Disable output switching */
- u8 _DOD:1; /*Enumerate all devices attached to display adapter */
- u8 _ROM:1; /*Get ROM Data */
- u8 _GPD:1; /*Get POST Device */
- u8 _SPD:1; /*Set POST Device */
- u8 _VPO:1; /*Video POST Options */
+ u8 _DOS:1; /* Enable/Disable output switching */
+ u8 _DOD:1; /* Enumerate all devices attached to
+ display adapter */
+ u8 _ROM:1; /* Get ROM Data */
+ u8 _GPD:1; /* Get POST Device */
+ u8 _SPD:1; /* Set POST Device */
+ u8 _VPO:1; /* Video POST Options */
u8 reserved:2;
};
struct acpi_video_device_attrib {
u32 display_index:4; /* A zero-based instance of the Display */
- u32 display_port_attachment:4; /*This field differentiates the display type */
- u32 display_type:4; /*Describe the specific type in use */
- u32 vendor_specific:4; /*Chipset Vendor Specific */
- u32 bios_can_detect:1; /*BIOS can detect the device */
- u32 depend_on_vga:1; /*Non-VGA output device whose power is related to
- the VGA device. */
- u32 pipe_id:3; /*For VGA multiple-head devices. */
- u32 reserved:10; /*Must be 0 */
- u32 device_id_scheme:1; /*Device ID Scheme */
+ u32 display_port_attachment:4; /* This field differentiates the
+ display type */
+ u32 display_type:4; /* Describe the specific type in use */
+ u32 vendor_specific:4; /* Chipset Vendor Specific */
+ u32 bios_can_detect:1; /* BIOS can detect the device */
+ u32 depend_on_vga:1; /* Non-VGA output device whose power is
+ related to the VGA device. */
+ u32 pipe_id:3; /* For VGA multiple-head devices. */
+ u32 reserved:10; /* Must be 0 */
+ u32 device_id_scheme:1; /* Device ID Scheme */
};
struct acpi_video_enumerated_device {
@@ -284,24 +286,27 @@ static void acpi_video_switch_brightness
static int acpi_video_device_get_state(struct acpi_video_device *device,
unsigned long *state);
static int acpi_video_output_get(struct output_device *od);
-static int acpi_video_device_set_state(struct acpi_video_device *device, int state);
+static int acpi_video_device_set_state(struct acpi_video_device *device,
+ int state);
/*backlight device sysfs support*/
static int acpi_video_get_brightness(struct backlight_device *bd)
{
+ struct acpi_video_device *vd = bl_get_data(bd);
unsigned long cur_level;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+
acpi_video_device_lcd_get_level_current(vd, &cur_level);
+
return (int) cur_level;
}
static int acpi_video_set_brightness(struct backlight_device *bd)
{
int request_level = bd->props.brightness;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+ struct acpi_video_device *vd = bl_get_data(bd);
+
acpi_video_device_lcd_set_level(vd, request_level);
+
return 0;
}
@@ -314,17 +319,18 @@ static struct backlight_ops acpi_backlig
static int acpi_video_output_get(struct output_device *od)
{
unsigned long state;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)dev_get_drvdata(&od->dev);
+ struct acpi_video_device *vd = dev_get_drvdata(&od->dev);
+
acpi_video_device_get_state(vd, &state);
+
return (int)state;
}
static int acpi_video_output_set(struct output_device *od)
{
unsigned long state = od->request_state;
- struct acpi_video_device *vd=
- (struct acpi_video_device *)dev_get_drvdata(&od->dev);
+ struct acpi_video_device *vd = dev_get_drvdata(&od->dev);
+
return acpi_video_device_set_state(vd, state);
}
@@ -341,37 +347,25 @@ static struct output_properties acpi_out
static int
acpi_video_device_query(struct acpi_video_device *device, unsigned long *state)
{
- int status;
-
- status = acpi_evaluate_integer(device->dev->handle, "_DGS", NULL, state);
-
- return status;
+ return acpi_evaluate_integer(device->dev->handle, "_DGS", NULL, state);
}
static int
acpi_video_device_get_state(struct acpi_video_device *device,
unsigned long *state)
{
- int status;
-
- status = acpi_evaluate_integer(device->dev->handle, "_DCS", NULL, state);
-
- return status;
+ return acpi_evaluate_integer(device->dev->handle, "_DCS", NULL, state);
}
static int
acpi_video_device_set_state(struct acpi_video_device *device, int state)
{
- int status;
union acpi_object arg0 = { ACPI_TYPE_INTEGER };
struct acpi_object_list args = { 1, &arg0 };
unsigned long ret;
-
arg0.integer.value = state;
- status = acpi_evaluate_integer(device->dev->handle, "_DSS", &args, &ret);
-
- return status;
+ return acpi_evaluate_integer(device->dev->handle, "_DSS", &args, &ret);
}
static int
@@ -382,26 +376,25 @@ acpi_video_device_lcd_query_levels(struc
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
-
*levels = NULL;
- status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL, &buffer);
+ status = acpi_evaluate_object(device->dev->handle, "_BCL",
+ NULL, &buffer);
if (!ACPI_SUCCESS(status))
return status;
+
obj = (union acpi_object *)buffer.pointer;
- if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+ if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
printk(KERN_ERR PREFIX "Invalid _BCL data\n");
status = -EFAULT;
goto err;
}
*levels = obj;
-
return 0;
- err:
+ err:
kfree(buffer.pointer);
-
return status;
}
@@ -412,7 +405,6 @@ acpi_video_device_lcd_set_level(struct a
union acpi_object arg0 = { ACPI_TYPE_INTEGER };
struct acpi_object_list args = { 1, &arg0 };
-
arg0.integer.value = level;
if (device->cap._BCM)
@@ -443,11 +435,11 @@ acpi_video_device_EDID(struct acpi_video
union acpi_object arg0 = { ACPI_TYPE_INTEGER };
struct acpi_object_list args = { 1, &arg0 };
-
*edid = NULL;
if (!device)
return -ENODEV;
+
if (length == 128)
arg0.integer.value = 1;
else if (length == 256)
@@ -455,7 +447,8 @@ acpi_video_device_EDID(struct acpi_video
else
return -EINVAL;
- status = acpi_evaluate_object(device->dev->handle, "_DDC", &args, &buffer);
+ status = acpi_evaluate_object(device->dev->handle, "_DDC", &args,
+ &buffer);
if (ACPI_FAILURE(status))
return -ENODEV;
@@ -482,12 +475,12 @@ acpi_video_bus_set_POST(struct acpi_vide
union acpi_object arg0 = { ACPI_TYPE_INTEGER };
struct acpi_object_list args = { 1, &arg0 };
-
arg0.integer.value = option;
- status = acpi_evaluate_integer(video->device->handle, "_SPD", &args, &tmp);
+ status = acpi_evaluate_integer(video->device->handle, "_SPD", &args,
+ &tmp);
if (ACPI_SUCCESS(status))
- status = tmp ? (-EINVAL) : (AE_OK);
+ status = tmp ? -EINVAL : AE_OK;
return status;
}
@@ -495,11 +488,7 @@ acpi_video_bus_set_POST(struct acpi_vide
static int
acpi_video_bus_get_POST(struct acpi_video_bus *video, unsigned long *id)
{
- int status;
-
- status = acpi_evaluate_integer(video->device->handle, "_GPD", NULL, id);
-
- return status;
+ return acpi_evaluate_integer(video->device->handle, "_GPD", NULL, id);
}
static int
@@ -508,7 +497,8 @@ acpi_video_bus_POST_options(struct acpi_
{
int status;
- status = acpi_evaluate_integer(video->device->handle, "_VPO", NULL, options);
+ status = acpi_evaluate_integer(video->device->handle, "_VPO", NULL,
+ options);
*options &= 3;
return status;
@@ -516,23 +506,25 @@ acpi_video_bus_POST_options(struct acpi_
/*
* Arg:
- * video : video bus device pointer
- * bios_flag :
- * 0. The system BIOS should NOT automatically switch(toggle)
- * the active display output.
- * 1. The system BIOS should automatically switch (toggle) the
- * active display output. No switch event.
+ * video : video bus device pointer
+ * bios_flag :
+ * 0. The system BIOS should NOT automatically switch
+ * (toggle) the active display output.
+ * 1. The system BIOS should automatically switch
+ * (toggle) the active display output. No switch event.
* 2. The _DGS value should be locked.
- * 3. The system BIOS should not automatically switch (toggle) the
- * active display output, but instead generate the display switch
- * event notify code.
+ * 3. The system BIOS should not automatically switch
+ * (toggle) the active display output, but instead
+ * generate the display switch event notify code.
* lcd_flag :
- * 0. The system BIOS should automatically control the brightness level
- * of the LCD when the power changes from AC to DC
- * 1. The system BIOS should NOT automatically control the brightness
- * level of the LCD when the power changes from AC to DC.
+ * 0. The system BIOS should automatically control the
+ * brightness level of the LCD when the power changes
+ * from AC to DC
+ * 1. The system BIOS should NOT automatically control
+ * the brightness level of the LCD when the power
+ * changes from AC to DC.
* Return Value:
- * -1 wrong arg.
+ * -1 wrong arg.
*/
static int
@@ -542,178 +534,199 @@ acpi_video_bus_DOS(struct acpi_video_bus
union acpi_object arg0 = { ACPI_TYPE_INTEGER };
struct acpi_object_list args = { 1, &arg0 };
-
if (bios_flag < 0 || bios_flag > 3 || lcd_flag < 0 || lcd_flag > 1) {
status = -1;
goto Failed;
}
+
arg0.integer.value = (lcd_flag << 2) | bios_flag;
video->dos_setting = arg0.integer.value;
acpi_evaluate_object(video->device->handle, "_DOS", &args, NULL);
- Failed:
+ Failed:
return status;
}
-/*
- * Arg:
- * device : video output device (LCD, CRT, ..)
- *
- * Return Value:
- * None
- *
- * Find out all required AML methods defined under the output
- * device.
- */
-
-static void acpi_video_device_find_cap(struct acpi_video_device *device)
+static void
+acpi_video_get_brightness_levels(struct acpi_video_device *device,
+ u32 *max_level)
{
- acpi_handle h_dummy1;
int i;
- u32 max_level = 0;
+ int count = 0;
union acpi_object *obj = NULL;
- struct acpi_video_device_brightness *br = NULL;
+ union acpi_object *o;
+ struct acpi_video_device_brightness *br;
+ *max_level = 0;
- memset(&device->cap, 0, 4);
-
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR", &h_dummy1))) {
- device->cap._ADR = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL", &h_dummy1))) {
- device->cap._BCL = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM", &h_dummy1))) {
- device->cap._BCM = 1;
+ if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Could not query available LCD brightness level\n"));
+ goto out;
}
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle,"_BQC",&h_dummy1)))
- device->cap._BQC = 1;
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC", &h_dummy1))) {
- device->cap._DDC = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DCS", &h_dummy1))) {
- device->cap._DCS = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DGS", &h_dummy1))) {
- device->cap._DGS = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DSS", &h_dummy1))) {
- device->cap._DSS = 1;
- }
-
- if (ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
-
- if (obj->package.count >= 2) {
- int count = 0;
- union acpi_object *o;
-
- br = kzalloc(sizeof(*br), GFP_KERNEL);
- if (!br) {
- printk(KERN_ERR "can't allocate memory\n");
- } else {
- br->levels = kmalloc(obj->package.count *
- sizeof *(br->levels), GFP_KERNEL);
- if (!br->levels)
- goto out;
-
- for (i = 0; i < obj->package.count; i++) {
- o = (union acpi_object *)&obj->package.
- elements[i];
- if (o->type != ACPI_TYPE_INTEGER) {
- printk(KERN_ERR PREFIX "Invalid data\n");
- continue;
- }
- br->levels[count] = (u32) o->integer.value;
-
- if (br->levels[count] > max_level)
- max_level = br->levels[count];
- count++;
- }
- out:
- if (count < 2) {
- kfree(br->levels);
- kfree(br);
- } else {
- br->count = count;
- device->brightness = br;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "found %d brightness levels\n",
- count));
- }
+
+ if (obj->package.count >= 2) {
+
+ br = kzalloc(sizeof(*br), GFP_KERNEL);
+ if (!br) {
+ printk(KERN_ERR PREFIX "can't allocate memory\n");
+ goto out;
+ }
+
+ br->levels = kcalloc(obj->package.count, sizeof *(br->levels),
+ GFP_KERNEL);
+ if (!br->levels) {
+ printk(KERN_ERR PREFIX
+ "can't allocate memory for levels\n");
+ kfree(br);
+ goto out;
+ }
+
+ for (i = 0; i < obj->package.count; i++) {
+ o = (union acpi_object *)&obj->package.elements[i];
+ if (o->type != ACPI_TYPE_INTEGER) {
+ printk(KERN_ERR PREFIX "Invalid data\n");
+ continue;
}
+ br->levels[count] = (u32) o->integer.value;
+
+ if (br->levels[count] > *max_level)
+ *max_level = br->levels[count];
+
+ count++;
}
- } else {
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available LCD brightness level\n"));
+ if (count < 2) {
+ kfree(br->levels);
+ kfree(br);
+ } else {
+ br->count = count;
+ device->brightness = br;
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "found %d brightness levels\n",
+ count));
+ }
}
+ out:
kfree(obj);
+}
+
+static int
+acpi_video_register_backlight(struct acpi_video_device *device,
+ u32 max_level)
+{
+ static atomic_t count = ATOMIC_INIT(0);
+ struct backlight_device *bl;
+ unsigned long tmp;
+ char name[MAX_NAME_LEN];
+
+ snprintf(name, sizeof(name), "acpi_video%ld",
+ (long)atomic_inc_return(&count) - 1);
+
+ acpi_video_device_lcd_get_level_current(device, &tmp);
+
+ bl = backlight_device_register(name, &device->dev->dev, device,
+ &acpi_backlight_ops);
+ if (IS_ERR(bl)) {
+ printk(KERN_ERR PREFIX
+ "can't register backlight device %s, error %ld\n",
+ name, PTR_ERR(bl));
+ return PTR_ERR(bl);
+ }
+
+ bl->props.max_brightness = max_level;
+ bl->props.brightness = (int)tmp;
+
+ device->backlight = bl;
+ backlight_update_status(device->backlight);
+
+ return 0;
+}
- if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
- unsigned long tmp;
- static int count = 0;
- char *name;
- name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
- if (!name)
- return;
-
- sprintf(name, "acpi_video%d", count++);
- acpi_video_device_lcd_get_level_current(device, &tmp);
- device->backlight = backlight_device_register(name,
- NULL, device, &acpi_backlight_ops);
- device->backlight->props.max_brightness = max_level;
- device->backlight->props.brightness = (int)tmp;
- backlight_update_status(device->backlight);
-
- kfree(name);
- }
- if (device->cap._DCS && device->cap._DSS){
- static int count = 0;
- char *name;
- name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
- if (!name)
- return;
- sprintf(name, "acpi_video%d", count++);
- device->output_dev = video_output_register(name,
- NULL, device, &acpi_output_properties);
- kfree(name);
+static int acpi_video_register_output(struct acpi_video_device *device)
+{
+ static atomic_t count = ATOMIC_INIT(0);
+ struct output_device *od;
+ char name[MAX_NAME_LEN];
+
+ snprintf(name, sizeof(name), "acpi_video%ld",
+ (long)atomic_inc_return(&count) - 1);
+
+ od = video_output_register(name, &device->dev->dev, device,
+ &acpi_output_properties);
+ if (IS_ERR(od)) {
+ printk(KERN_ERR PREFIX
+ "can't register output device %s, error %ld\n",
+ name, PTR_ERR(od));
+ return PTR_ERR(od);
}
- return;
+
+ device->output_dev = od;
+ return 0;
+}
+
+/*
+ * Arg:
+ * device : video output device (LCD, CRT, ..)
+ *
+ * Return Value:
+ * None
+ *
+ * Find out all required AML methods defined under the output
+ * device.
+ */
+
+#define VIDEO_DEV_CHECK_CAP(_cap) \
+ do { \
+ acpi_handle h; \
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, \
+ __stringify(_cap), &h))) \
+ device->cap._cap = 1; \
+ } while (0)
+
+static void acpi_video_device_find_cap(struct acpi_video_device *device)
+{
+ memset(&device->cap, 0, 4);
+
+ VIDEO_DEV_CHECK_CAP(_ADR);
+ VIDEO_DEV_CHECK_CAP(_BCL);
+ VIDEO_DEV_CHECK_CAP(_BCM);
+ VIDEO_DEV_CHECK_CAP(_BQC);
+ VIDEO_DEV_CHECK_CAP(_DDC);
+ VIDEO_DEV_CHECK_CAP(_DCS);
+ VIDEO_DEV_CHECK_CAP(_DGS);
+ VIDEO_DEV_CHECK_CAP(_DSS);
}
/*
- * Arg:
- * device : video output device (VGA)
+ * Arg:
+ * device : video output device (VGA)
*
* Return Value:
- * None
+ * None
*
* Find out all required AML methods defined under the video bus device.
*/
+#define VIDEO_BUS_CHECK_CAP(_cap) \
+ do { \
+ acpi_handle h; \
+ if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, \
+ __stringify(_cap), &h))) \
+ video->cap._cap = 1; \
+ } while (0)
+
static void acpi_video_bus_find_cap(struct acpi_video_bus *video)
{
- acpi_handle h_dummy1;
-
memset(&video->cap, 0, 4);
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS", &h_dummy1))) {
- video->cap._DOS = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD", &h_dummy1))) {
- video->cap._DOD = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_ROM", &h_dummy1))) {
- video->cap._ROM = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_GPD", &h_dummy1))) {
- video->cap._GPD = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_SPD", &h_dummy1))) {
- video->cap._SPD = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_VPO", &h_dummy1))) {
- video->cap._VPO = 1;
- }
+
+ VIDEO_BUS_CHECK_CAP(_DOS);
+ VIDEO_BUS_CHECK_CAP(_DOD);
+ VIDEO_BUS_CHECK_CAP(_ROM);
+ VIDEO_BUS_CHECK_CAP(_GPD);
+ VIDEO_BUS_CHECK_CAP(_SPD);
+ VIDEO_BUS_CHECK_CAP(_VPO);
}
/*
@@ -725,10 +738,6 @@ static int acpi_video_bus_check(struct a
{
acpi_status status = -ENOENT;
-
- if (!video)
- return -EINVAL;
-
/* Since there is no HID, CID and so on for VGA driver, we have
* to check well known required nodes.
*/
@@ -766,10 +775,6 @@ static int acpi_video_device_info_seq_sh
{
struct acpi_video_device *dev = seq->private;
-
- if (!dev)
- goto end;
-
seq_printf(seq, "device_id: 0x%04x\n", (u32) dev->device_id);
seq_printf(seq, "type: ");
if (dev->flags.crt)
@@ -785,7 +790,6 @@ static int acpi_video_device_info_seq_sh
seq_printf(seq, "known by bios: %s\n", dev->flags.bios ? "yes" : "no");
- end:
return 0;
}
@@ -802,10 +806,6 @@ static int acpi_video_device_state_seq_s
struct acpi_video_device *dev = seq->private;
unsigned long state;
-
- if (!dev)
- goto end;
-
status = acpi_video_device_get_state(dev, &state);
seq_printf(seq, "state: ");
if (ACPI_SUCCESS(status))
@@ -820,7 +820,6 @@ static int acpi_video_device_state_seq_s
else
seq_printf(seq, "<not supported>\n");
- end:
return 0;
}
@@ -842,7 +841,6 @@ acpi_video_device_write_state(struct fil
char str[12] = { 0 };
u32 state = 0;
-
if (!dev || count + 1 > sizeof str)
return -EINVAL;
@@ -867,8 +865,7 @@ acpi_video_device_brightness_seq_show(st
struct acpi_video_device *dev = seq->private;
int i;
-
- if (!dev || !dev->brightness) {
+ if (!dev->brightness) {
seq_printf(seq, "<not supported>\n");
return 0;
}
@@ -899,7 +896,6 @@ acpi_video_device_write_brightness(struc
unsigned int level = 0;
int i;
-
if (!dev || !dev->brightness || count + 1 > sizeof str)
return -EINVAL;
@@ -931,25 +927,18 @@ static int acpi_video_device_EDID_seq_sh
int i;
union acpi_object *edid = NULL;
-
- if (!dev)
- goto out;
-
status = acpi_video_device_EDID(dev, &edid, 128);
- if (ACPI_FAILURE(status)) {
+ if (ACPI_FAILURE(status))
status = acpi_video_device_EDID(dev, &edid, 256);
- }
- if (ACPI_FAILURE(status)) {
+ if (ACPI_FAILURE(status))
goto out;
- }
- if (edid && edid->type == ACPI_TYPE_BUFFER) {
+ if (edid && edid->type == ACPI_TYPE_BUFFER)
for (i = 0; i < edid->buffer.length; i++)
seq_putc(seq, edid->buffer.pointer[i]);
- }
- out:
+ out:
if (!edid)
seq_printf(seq, "<not supported>\n");
else
@@ -1095,7 +1084,7 @@ static int acpi_video_bus_ROM_seq_show(s
printk(KERN_INFO PREFIX "Please implement %s\n", __FUNCTION__);
seq_printf(seq, "<TODO>\n");
- end:
+ end:
return 0;
}
@@ -1110,17 +1099,15 @@ static int acpi_video_bus_POST_info_seq_
unsigned long options;
int status;
-
- if (!video)
- goto end;
-
status = acpi_video_bus_POST_options(video, &options);
if (ACPI_SUCCESS(status)) {
if (!(options & 1)) {
printk(KERN_WARNING PREFIX
- "The motherboard VGA device is not listed as a possible POST device.\n");
+ "The motherboard VGA device is not listed "
+ "as a possible POST device.\n");
printk(KERN_WARNING PREFIX
- "This indicates a BIOS bug. Please contact the manufacturer.\n");
+ "This indicates a BIOS bug. "
+ "Please contact the manufacturer.\n");
}
printk("%lx\n", options);
seq_printf(seq, "can POST: <integrated video>");
@@ -1131,7 +1118,7 @@ static int acpi_video_bus_POST_info_seq_
seq_putc(seq, '\n');
} else
seq_printf(seq, "<not supported>\n");
- end:
+
return 0;
}
@@ -1148,10 +1135,6 @@ static int acpi_video_bus_POST_seq_show(
int status;
unsigned long id;
-
- if (!video)
- goto end;
-
status = acpi_video_bus_get_POST(video, &id);
if (!ACPI_SUCCESS(status)) {
seq_printf(seq, "<not supported>\n");
@@ -1159,7 +1142,7 @@ static int acpi_video_bus_POST_seq_show(
}
seq_printf(seq, "device POSTed is <%s>\n", device_decode[id & 3]);
- end:
+ end:
return 0;
}
@@ -1167,7 +1150,6 @@ static int acpi_video_bus_DOS_seq_show(s
{
struct acpi_video_bus *video = seq->private;
-
seq_printf(seq, "DOS setting: <%d>\n", video->dos_setting);
return 0;
@@ -1195,7 +1177,6 @@ acpi_video_bus_write_POST(struct file *f
char str[12] = { 0 };
unsigned long opt, options;
-
if (!video || count + 1 > sizeof str)
return -EINVAL;
@@ -1354,8 +1335,9 @@ static int acpi_video_bus_remove_fs(stru
-------------------------------------------------------------------------- */
/* device interface */
-static struct acpi_video_device_attrib*
-acpi_video_get_device_attr(struct acpi_video_bus *video, unsigned long device_id)
+static struct acpi_video_device_attrib *
+acpi_video_get_device_attr(struct acpi_video_bus *video,
+ unsigned long device_id)
{
struct acpi_video_enumerated_device *ids;
int i;
@@ -1369,96 +1351,132 @@ acpi_video_get_device_attr(struct acpi_v
return NULL;
}
+static void
+acpi_video_get_device_flags(struct acpi_video_device *device)
+{
+ struct acpi_video_device_attrib *attribute =
+ acpi_video_get_device_attr(device->video, device->device_id);
+
+ if (attribute && attribute->device_id_scheme) {
+ switch (attribute->display_type) {
+ case ACPI_VIDEO_DISPLAY_CRT:
+ device->flags.crt = 1;
+ break;
+ case ACPI_VIDEO_DISPLAY_TV:
+ device->flags.tvout = 1;
+ break;
+ case ACPI_VIDEO_DISPLAY_DVI:
+ device->flags.dvi = 1;
+ break;
+ case ACPI_VIDEO_DISPLAY_LCD:
+ device->flags.lcd = 1;
+ break;
+ default:
+ device->flags.unknown = 1;
+ break;
+ }
+ if (attribute->bios_can_detect)
+ device->flags.bios = 1;
+ } else
+ device->flags.unknown = 1;
+}
+
static int
acpi_video_bus_get_one_device(struct acpi_device *device,
struct acpi_video_bus *video)
{
+ struct acpi_video_device *data;
unsigned long device_id;
+ u32 max_brightness;
int status;
- struct acpi_video_device *data;
- struct acpi_video_device_attrib* attribute;
+ int error;
- if (!device || !video)
- return -EINVAL;
+ status = acpi_evaluate_integer(device->handle, "_ADR", NULL,
+ &device_id);
+ if (!ACPI_SUCCESS(status))
+ return -ENOENT;
- status =
- acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
- if (ACPI_SUCCESS(status)) {
+ data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
- data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
- strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
- acpi_driver_data(device) = data;
-
- data->device_id = device_id;
- data->video = video;
- data->dev = device;
-
- attribute = acpi_video_get_device_attr(video, device_id);
-
- if((attribute != NULL) && attribute->device_id_scheme) {
- switch (attribute->display_type) {
- case ACPI_VIDEO_DISPLAY_CRT:
- data->flags.crt = 1;
- break;
- case ACPI_VIDEO_DISPLAY_TV:
- data->flags.tvout = 1;
- break;
- case ACPI_VIDEO_DISPLAY_DVI:
- data->flags.dvi = 1;
- break;
- case ACPI_VIDEO_DISPLAY_LCD:
- data->flags.lcd = 1;
- break;
- default:
- data->flags.unknown = 1;
- break;
- }
- if(attribute->bios_can_detect)
- data->flags.bios = 1;
- } else
- data->flags.unknown = 1;
-
- acpi_video_device_bind(video, data);
- acpi_video_device_find_cap(data);
-
- status = acpi_install_notify_handler(device->handle,
- ACPI_DEVICE_NOTIFY,
- acpi_video_device_notify,
- data);
- if (ACPI_FAILURE(status)) {
- ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
- "Error installing notify handler\n"));
- if(data->brightness)
- kfree(data->brightness->levels);
- kfree(data->brightness);
- kfree(data);
- return -ENODEV;
- }
+ strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
+ strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
+ acpi_driver_data(device) = data;
- mutex_lock(&video->device_list_lock);
- list_add_tail(&data->entry, &video->video_device_list);
- mutex_unlock(&video->device_list_lock);
+ data->device_id = device_id;
+ data->video = video;
+ data->dev = device;
- acpi_video_device_add_fs(device);
+ acpi_video_get_device_flags(data);
+ acpi_video_device_find_cap(data);
+ acpi_video_get_brightness_levels(data, &max_brightness);
- return 0;
+ acpi_video_device_bind(video, data);
+
+ if (data->cap._BCL && data->cap._BCM && data->cap._BQC &&
+ max_brightness > 0) {
+ error = acpi_video_register_backlight(data, max_brightness);
+ if (error)
+ goto err_free_video_dev;
+ }
+
+ if (data->cap._DCS && data->cap._DSS) {
+ error = acpi_video_register_output(data);
+ if (error)
+ goto err_remove_backlight;
}
- return -ENOENT;
+ status = acpi_install_notify_handler(device->handle,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_device_notify,
+ data);
+ if (ACPI_FAILURE(status)) {
+ ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+ "Error installing notify handler\n"));
+ error = -EINVAL;
+ goto err_remove_output_dev;
+ }
+
+ mutex_lock(&video->device_list_lock);
+ list_add_tail(&data->entry, &video->video_device_list);
+ mutex_unlock(&video->device_list_lock);
+
+ error = acpi_video_device_add_fs(device);
+ if (error)
+ goto err_remove_from_list;
+
+ return 0;
+
+ err_remove_from_list:
+ mutex_lock(&video->device_list_lock);
+ list_del(&data->entry);
+ mutex_unlock(&video->device_list_lock);
+ acpi_remove_notify_handler(data->dev->handle,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_device_notify);
+ err_remove_output_dev:
+ video_output_unregister(data->output_dev);
+ err_remove_backlight:
+ backlight_device_unregister(data->backlight);
+ err_free_video_dev:
+ if (data->brightness) {
+ kfree(data->brightness->levels);
+ kfree(data->brightness);
+ }
+ kfree(data);
+
+ return 0;
}
/*
* Arg:
- * video : video bus device
+ * video : video bus device
*
* Return:
- * none
- *
- * Enumerate the video device list of the video bus,
+ * none
+ *
+ * Enumerate the video device list of the video bus,
* bind the ids with the corresponding video devices
* under the video bus.
*/
@@ -1477,13 +1495,13 @@ static void acpi_video_device_rebind(str
/*
* Arg:
- * video : video bus device
- * device : video output device under the video
- * bus
+ * video : video bus device
+ * device : video output device under the video
+ * bus
*
* Return:
- * none
- *
+ * none
+ *
* Bind the ids with the corresponding video devices
* under the video bus.
*/
@@ -1506,11 +1524,11 @@ acpi_video_device_bind(struct acpi_video
/*
* Arg:
- * video : video bus device
+ * video : video bus device
*
* Return:
- * < 0 : error
- *
+ * < 0 : error
+ *
* Call _DOD to enumerate all devices attached to display adapter
*
*/
@@ -1525,14 +1543,15 @@ static int acpi_video_device_enumerate(s
union acpi_object *dod = NULL;
union acpi_object *obj;
- status = acpi_evaluate_object(video->device->handle, "_DOD", NULL, &buffer);
+ status = acpi_evaluate_object(video->device->handle, "_DOD", NULL,
+ &buffer);
if (!ACPI_SUCCESS(status)) {
ACPI_EXCEPTION((AE_INFO, status, "Evaluating _DOD"));
return status;
}
dod = buffer.pointer;
- if (!dod || (dod->type != ACPI_TYPE_PACKAGE)) {
+ if (!dod || dod->type != ACPI_TYPE_PACKAGE) {
ACPI_EXCEPTION((AE_INFO, status, "Invalid _DOD data"));
status = -EFAULT;
goto out;
@@ -1578,12 +1597,12 @@ static int acpi_video_device_enumerate(s
/*
* Arg:
- * video : video bus device
- * event : notify event
+ * video : video bus device
+ * event : notify event
*
* Return:
- * < 0 : error
- *
+ * < 0 : error
+ *
* 1. Find out the current active output device.
* 2. Identify the next output device to switch to.
* 3. call _DSS to do actual switch.
@@ -1683,47 +1702,39 @@ static void
acpi_video_switch_brightness(struct acpi_video_device *device, int event)
{
unsigned long level_current, level_next;
+
acpi_video_device_lcd_get_level_current(device, &level_current);
level_next = acpi_video_get_next_level(device, level_current, event);
acpi_video_device_lcd_set_level(device, level_next);
}
-static int
+static void
acpi_video_bus_get_devices(struct acpi_video_bus *video,
struct acpi_device *device)
{
- int status = 0;
+ int error;
struct acpi_device *dev;
acpi_video_device_enumerate(video);
list_for_each_entry(dev, &device->children, node) {
-
- status = acpi_video_bus_get_one_device(dev, video);
- if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "Cant attach device"));
- continue;
- }
+ error = acpi_video_bus_get_one_device(dev, video);
+ if (error)
+ printk(KERN_ERR PREFIX "Can't attach device");
}
- return status;
}
static int acpi_video_bus_put_one_device(struct acpi_video_device *device)
{
- acpi_status status;
- struct acpi_video_bus *video;
+ struct acpi_video_bus *video = device->video;
-
- if (!device || !device->video)
+ if (!video)
return -ENOENT;
- video = device->video;
-
acpi_video_device_remove_fs(device->dev);
-
- status = acpi_remove_notify_handler(device->dev->handle,
- ACPI_DEVICE_NOTIFY,
- acpi_video_device_notify);
+ acpi_remove_notify_handler(device->dev->handle,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_device_notify);
backlight_device_unregister(device->backlight);
video_output_unregister(device->output_dev);
@@ -1772,12 +1783,11 @@ static int acpi_video_bus_stop_devices(s
static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_video_bus *video = data;
- struct acpi_device *device = NULL;
+ struct acpi_device *device;
struct input_dev *input;
int keycode;
-
- printk("video bus notify\n");
+ printk(KERN_DEBUG "video bus notify\n");
if (!video)
return;
@@ -1828,14 +1838,12 @@ static void acpi_video_bus_notify(acpi_h
input_sync(input);
input_report_key(input, keycode, 0);
input_sync(input);
-
- return;
}
static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_video_device *video_device = data;
- struct acpi_device *device = NULL;
+ struct acpi_device *device;
struct acpi_video_bus *bus;
struct input_dev *input;
int keycode;
@@ -1884,30 +1892,33 @@ static void acpi_video_device_notify(acp
input_sync(input);
input_report_key(input, keycode, 0);
input_sync(input);
-
- return;
}
-static int instance;
static int acpi_video_bus_add(struct acpi_device *device)
{
+ static atomic_t instance = ATOMIC_INIT(0);
+
acpi_status status;
struct acpi_video_bus *video;
struct input_dev *input;
int error;
+ int i;
video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
if (!video)
return -ENOMEM;
+ mutex_init(&video->device_list_lock);
+ INIT_LIST_HEAD(&video->video_device_list);
+ video->device = device;
+
/* a hack to fix the duplicate name "VID" problem on T61 */
if (!strcmp(device->pnp.bus_id, "VID")) {
- if (instance)
- device->pnp.bus_id[3] = '0' + instance;
- instance ++;
+ i = atomic_inc_return(&instance) - 1;
+ if (i)
+ device->pnp.bus_id[3] = '0' + i;
}
- video->device = device;
strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
acpi_driver_data(device) = video;
@@ -1921,9 +1932,6 @@ static int acpi_video_bus_add(struct acp
if (error)
goto err_free_video;
- mutex_init(&video->device_list_lock);
- INIT_LIST_HEAD(&video->video_device_list);
-
acpi_video_bus_get_devices(video, device);
acpi_video_bus_start_devices(video);
@@ -1993,25 +2001,20 @@ static int acpi_video_bus_add(struct acp
static int acpi_video_bus_remove(struct acpi_device *device, int type)
{
- acpi_status status = 0;
- struct acpi_video_bus *video = NULL;
-
+ struct acpi_video_bus *video = acpi_driver_data(device);
- if (!device || !acpi_driver_data(device))
+ if (!video)
return -EINVAL;
- video = acpi_driver_data(device);
-
acpi_video_bus_stop_devices(video);
-
- status = acpi_remove_notify_handler(video->device->handle,
- ACPI_DEVICE_NOTIFY,
- acpi_video_bus_notify);
-
+ acpi_remove_notify_handler(video->device->handle,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_bus_notify);
acpi_video_bus_put_devices(video);
acpi_video_bus_remove_fs(device);
input_unregister_device(video->input);
+
kfree(video->attached_array);
kfree(video);
@@ -2022,7 +2025,6 @@ static int __init acpi_video_init(void)
{
int result = 0;
-
/*
acpi_dbg_level = 0xFFFFFFFF;
acpi_dbg_layer = 0x08000000;
@@ -2044,12 +2046,8 @@ static int __init acpi_video_init(void)
static void __exit acpi_video_exit(void)
{
-
acpi_bus_unregister_driver(&acpi_video_bus);
-
remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir);
-
- return;
}
module_init(acpi_video_init);
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* [patch 8/8] ACPI: video - fix permissions on some proc entries
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (6 preceding siblings ...)
2007-11-05 16:43 ` [patch 7/8] ACPI: video - more cleanups Dmitry Torokhov
@ 2007-11-05 16:43 ` Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-14 7:17 ` [patch 0/8] ACPI Video various cleanups & fixes Zhang Rui
8 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-05 16:43 UTC (permalink / raw)
To: Len Brown, Rui Zhang; +Cc: linux-acpi
[-- Attachment #1: acpi-video-fix-proc-perms.patch --]
[-- Type: text/plain, Size: 1056 bytes --]
ACPI: video - fix permissions on some proc entries
POST and DOS are supposed to be writable but permissions
did not allow it.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/acpi/video.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -1276,7 +1276,7 @@ static int acpi_video_bus_add_fs(struct
entry->owner = THIS_MODULE;
/* 'POST' [R/W] */
- entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
+ entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IWUSR,
device_dir);
if (!entry)
goto err_remove_post_info;
@@ -1287,7 +1287,7 @@ static int acpi_video_bus_add_fs(struct
entry->owner = THIS_MODULE;
/* 'DOS' [R/W] */
- entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
+ entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IWUSR,
device_dir);
if (!entry)
goto err_remove_post;
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 0/8] ACPI Video various cleanups & fixes
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
` (7 preceding siblings ...)
2007-11-05 16:43 ` [patch 8/8] ACPI: video - fix permissions on some proc entries Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 15:04 ` Dmitry Torokhov
8 siblings, 1 reply; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> Hi Len, Rui,
>
> Here is a group of cleanups and patches to the acpi video driver.
> Originally I just wanted to fix sysfs placement of input devices
> created by the driver, but then I saw some more potential issues.
>
> The patches are against Linus's tree, please let me know if you
> want them redone against something else.
>
Hi, Dmitry,
Thanks for your patches and sorry for the delay.
The patches are good and I'm okay with most of them.
Detailed comments are inline.
Thanks,
Rui
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 1/8] ACPI: video - fit input device into sysfs tree
2007-11-05 16:43 ` [patch 1/8] ACPI: video - fit input device into sysfs tree Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 16:49 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - fit input device into sysfs tree
>
> Properly set up parent on input device registered by the video driver.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/video.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -1961,6 +1961,7 @@ static int acpi_video_bus_add(struct acp
> input->phys = video->phys;
> input->id.bustype = BUS_HOST;
> input->id.product = 0x06;
> + input->dev.parent = &device->dev;
> input->evbit[0] = BIT(EV_KEY);
> set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
> set_bit(KEY_VIDEO_NEXT, input->keybit);
> @@ -1990,7 +1991,7 @@ static int acpi_video_bus_add(struct acp
> video->flags.rom ? "yes" : "no",
> video->flags.post ? "yes" : "no");
>
> - end:
> + end:
> if (result)
> kfree(video);
>
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 2/8] ACPI: video - add missing input_free_device()
2007-11-05 16:43 ` [patch 2/8] ACPI: video - add missing input_free_device() Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:00 ` Len Brown
1 sibling, 0 replies; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - add missing input_free_device()
>
> If input_register_device() fails input_free_device() must
> be called to release memory allocated for the device.
> Also consolidate error handling in acpi_bus_video_add()
> and handle input_allocate_device() failures.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/video.c | 71
> +++++++++++++++++++++++++--------------------------
> 1 file changed, 35 insertions(+), 36 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -1897,14 +1897,10 @@ static void acpi_video_device_notify(acp
> static int instance;
> static int acpi_video_bus_add(struct acpi_device *device)
> {
> - int result = 0;
> - acpi_status status = 0;
> - struct acpi_video_bus *video = NULL;
> + acpi_status status;
> + struct acpi_video_bus *video;
> struct input_dev *input;
> -
> -
> - if (!device)
> - return -EINVAL;
> + int error;
>
> video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
> if (!video)
> @@ -1923,13 +1919,13 @@ static int acpi_video_bus_add(struct acp
> acpi_driver_data(device) = video;
>
> acpi_video_bus_find_cap(video);
> - result = acpi_video_bus_check(video);
> - if (result)
> - goto end;
> -
> - result = acpi_video_bus_add_fs(device);
> - if (result)
> - goto end;
> + error = acpi_video_bus_check(video);
> + if (error)
> + goto err_free_video;
> +
> + error = acpi_video_bus_add_fs(device);
> + if (error)
> + goto err_free_video;
>
> init_MUTEX(&video->sem);
> INIT_LIST_HEAD(&video->video_device_list);
> @@ -1943,16 +1939,15 @@ static int acpi_video_bus_add(struct acp
> if (ACPI_FAILURE(status)) {
> ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
> "Error installing notify handler
> \n"));
> - acpi_video_bus_stop_devices(video);
> - acpi_video_bus_put_devices(video);
> - kfree(video->attached_array);
> - acpi_video_bus_remove_fs(device);
> - result = -ENODEV;
> - goto end;
> + error = -ENODEV;
> + goto err_stop_video;
> }
>
> -
> video->input = input = input_allocate_device();
> + if (!input) {
> + error = -ENOMEM;
> + goto err_uninstall_notify;
> + }
>
> snprintf(video->phys, sizeof(video->phys),
> "%s/video/input0", acpi_device_hid(video->device));
> @@ -1972,18 +1967,10 @@ static int acpi_video_bus_add(struct acp
> set_bit(KEY_BRIGHTNESS_ZERO, input->keybit);
> set_bit(KEY_DISPLAY_OFF, input->keybit);
> set_bit(KEY_UNKNOWN, input->keybit);
> - result = input_register_device(input);
> - if (result) {
> - acpi_remove_notify_handler(video->device->handle,
> - ACPI_DEVICE_NOTIFY,
> -
> acpi_video_bus_notify);
> - acpi_video_bus_stop_devices(video);
> - acpi_video_bus_put_devices(video);
> - kfree(video->attached_array);
> - acpi_video_bus_remove_fs(device);
> - goto end;
> - }
>
> + error = input_register_device(input);
> + if (error)
> + goto err_free_input_dev;
>
> printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s
> post: %s)\n",
> ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
> @@ -1991,11 +1978,23 @@ static int acpi_video_bus_add(struct acp
> video->flags.rom ? "yes" : "no",
> video->flags.post ? "yes" : "no");
>
> - end:
> - if (result)
> - kfree(video);
> + return 0;
> +
> + err_free_input_dev:
> + input_free_device(input);
> + err_uninstall_notify:
> + acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> + acpi_video_bus_notify);
> + err_stop_video:
> + acpi_video_bus_stop_devices(video);
> + acpi_video_bus_put_devices(video);
> + kfree(video->attached_array);
> + acpi_video_bus_remove_fs(device);
> + err_free_video:
> + kfree(video);
> + acpi_driver_data(device) = NULL;
>
> - return result;
> + return error;
> }
>
> static int acpi_video_bus_remove(struct acpi_device *device, int
> type)
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe()
2007-11-05 16:43 ` [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe() Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:46 ` Len Brown
1 sibling, 0 replies; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - remove unsafe uses of list_for_each_safe()
>
> list_for_each_safe() only protects list from list alterations
> performed by the same thread. One still needs to implement
> proper locking when list is being accessed from several threads.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/video.c | 71
> ++++++++++++++++++++++++---------------------------
> 1 file changed, 34 insertions(+), 37 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -1462,12 +1462,14 @@ acpi_video_bus_get_one_device(struct acp
>
> static void acpi_video_device_rebind(struct acpi_video_bus *video)
> {
> - struct list_head *node, *next;
> - list_for_each_safe(node, next, &video->video_device_list) {
> - struct acpi_video_device *dev =
> - container_of(node, struct acpi_video_device,
> entry);
> + struct acpi_video_device *dev;
> +
> + down(&video->sem);
> +
> + list_for_each_entry(dev, &video->video_device_list, entry)
> acpi_video_device_bind(video, dev);
> - }
> +
> + up(&video->sem);
> }
>
> /*
> @@ -1592,30 +1594,33 @@ static int acpi_video_device_enumerate(s
>
> static int acpi_video_switch_output(struct acpi_video_bus *video, int
> event)
> {
> - struct list_head *node, *next;
> + struct list_head *node;
> struct acpi_video_device *dev = NULL;
> struct acpi_video_device *dev_next = NULL;
> struct acpi_video_device *dev_prev = NULL;
> unsigned long state;
> int status = 0;
>
> + down(&video->sem);
>
> - list_for_each_safe(node, next, &video->video_device_list) {
> + list_for_each(node, &video->video_device_list) {
> dev = container_of(node, struct acpi_video_device,
> entry);
> status = acpi_video_device_get_state(dev, &state);
> if (state & 0x2) {
> - dev_next =
> - container_of(node->next, struct
> acpi_video_device,
> - entry);
> - dev_prev =
> - container_of(node->prev, struct
> acpi_video_device,
> - entry);
> + dev_next = container_of(node->next,
> + struct acpi_video_device,
> entry);
> + dev_prev = container_of(node->prev,
> + struct acpi_video_device,
> entry);
> goto out;
> }
> }
> +
> dev_next = container_of(node->next, struct acpi_video_device,
> entry);
> dev_prev = container_of(node->prev, struct acpi_video_device,
> entry);
> - out:
> +
> + out:
> + up(&video->sem);
> +
> switch (event) {
> case ACPI_VIDEO_NOTIFY_CYCLE:
> case ACPI_VIDEO_NOTIFY_NEXT_OUTPUT:
> @@ -1691,24 +1696,17 @@ acpi_video_bus_get_devices(struct acpi_v
> struct acpi_device *device)
> {
> int status = 0;
> - struct list_head *node, *next;
> -
> + struct acpi_device *dev;
>
> acpi_video_device_enumerate(video);
>
> - list_for_each_safe(node, next, &device->children) {
> - struct acpi_device *dev =
> - list_entry(node, struct acpi_device, node);
> -
> - if (!dev)
> - continue;
> + list_for_each_entry(dev, &device->children, node) {
>
> status = acpi_video_bus_get_one_device(dev, video);
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status, "Cant attach
> device"));
> continue;
> }
> -
> }
> return status;
> }
> @@ -1724,9 +1722,6 @@ static int acpi_video_bus_put_one_device
>
> video = device->video;
>
> - down(&video->sem);
> - list_del(&device->entry);
> - up(&video->sem);
> acpi_video_device_remove_fs(device->dev);
>
> status = acpi_remove_notify_handler(device->dev->handle,
> @@ -1734,32 +1729,34 @@ static int acpi_video_bus_put_one_device
> acpi_video_device_notify);
> backlight_device_unregister(device->backlight);
> video_output_unregister(device->output_dev);
> +
> return 0;
> }
>
> static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
> {
> int status;
> - struct list_head *node, *next;
> + struct acpi_video_device *dev, *next;
>
> + down(&video->sem);
>
> - list_for_each_safe(node, next, &video->video_device_list) {
> - struct acpi_video_device *data =
> - list_entry(node, struct acpi_video_device, entry);
> - if (!data)
> - continue;
> + list_for_each_entry_safe(dev, next, &video->video_device_list,
> entry) {
>
> - status = acpi_video_bus_put_one_device(data);
> + status = acpi_video_bus_put_one_device(dev);
> if (ACPI_FAILURE(status))
> printk(KERN_WARNING PREFIX
> "hhuuhhuu bug in acpi video
> driver.\n");
>
> - if (data->brightness)
> - kfree(data->brightness->levels);
> - kfree(data->brightness);
> - kfree(data);
> + if (dev->brightness) {
> + kfree(dev->brightness->levels);
> + kfree(dev->brightness);
> + }
> + list_del(&dev->entry);
> + kfree(dev);
> }
>
> + up(&video->sem);
> +
> return 0;
> }
>
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 4/8] ACPI: video - convert semaphore to a mutex
2007-11-05 16:43 ` [patch 4/8] ACPI: video - convert semaphore to a mutex Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:18 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - convert semaphore to a mutex
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/video.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -29,6 +29,7 @@
> #include <linux/init.h>
> #include <linux/types.h>
> #include <linux/list.h>
> +#include <linux/mutex.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
> #include <linux/input.h>
> @@ -135,8 +136,8 @@ struct acpi_video_bus {
> u8 attached_count;
> struct acpi_video_bus_cap cap;
> struct acpi_video_bus_flags flags;
> - struct semaphore sem;
> struct list_head video_device_list;
> + struct mutex device_list_lock; /* protects video_device_list
> */
> struct proc_dir_entry *dir;
> struct input_dev *input;
> char phys[32]; /* for input device */
> @@ -1436,9 +1437,9 @@ acpi_video_bus_get_one_device(struct acp
> return -ENODEV;
> }
>
> - down(&video->sem);
> + mutex_lock(&video->device_list_lock);
> list_add_tail(&data->entry,
> &video->video_device_list);
> - up(&video->sem);
> + mutex_unlock(&video->device_list_lock);
>
> acpi_video_device_add_fs(device);
>
> @@ -1464,12 +1465,12 @@ static void acpi_video_device_rebind(str
> {
> struct acpi_video_device *dev;
>
> - down(&video->sem);
> + mutex_lock(&video->device_list_lock);
>
> list_for_each_entry(dev, &video->video_device_list, entry)
> acpi_video_device_bind(video, dev);
>
> - up(&video->sem);
> + mutex_unlock(&video->device_list_lock);
> }
>
> /*
> @@ -1601,7 +1602,7 @@ static int acpi_video_switch_output(stru
> unsigned long state;
> int status = 0;
>
> - down(&video->sem);
> + mutex_lock(&video->device_list_lock);
>
> list_for_each(node, &video->video_device_list) {
> dev = container_of(node, struct acpi_video_device,
> entry);
> @@ -1619,7 +1620,7 @@ static int acpi_video_switch_output(stru
> dev_prev = container_of(node->prev, struct acpi_video_device,
> entry);
>
> out:
> - up(&video->sem);
> + mutex_unlock(&video->device_list_lock);
>
> switch (event) {
> case ACPI_VIDEO_NOTIFY_CYCLE:
> @@ -1738,7 +1739,7 @@ static int acpi_video_bus_put_devices(st
> int status;
> struct acpi_video_device *dev, *next;
>
> - down(&video->sem);
> + mutex_lock(&video->device_list_lock);
>
> list_for_each_entry_safe(dev, next, &video->video_device_list,
> entry) {
>
> @@ -1755,7 +1756,7 @@ static int acpi_video_bus_put_devices(st
> kfree(dev);
> }
>
> - up(&video->sem);
> + mutex_unlock(&video->device_list_lock);
>
> return 0;
> }
> @@ -1924,7 +1925,7 @@ static int acpi_video_bus_add(struct acp
> if (error)
> goto err_free_video;
>
> - init_MUTEX(&video->sem);
> + mutex_init(&video->device_list_lock);
> INIT_LIST_HEAD(&video->video_device_list);
>
> acpi_video_bus_get_devices(video, device);
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 5/8] ACPI: video - simplify handling of attached devices
2007-11-05 16:43 ` [patch 5/8] ACPI: video - simplify handling of attached devices Dmitry Torokhov
@ 2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:34 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - simplify handling of attached devices
>
> The old code needlessly stored invalid entries in attached_array list.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/video.c | 58
> +++++++++++++++++++++++----------------------------
> 1 file changed, 27 insertions(+), 31 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -56,8 +56,6 @@
> #define ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS 0x88
> #define ACPI_VIDEO_NOTIFY_DISPLAY_OFF 0x89
>
> -#define ACPI_VIDEO_HEAD_INVALID (~0u - 1)
> -#define ACPI_VIDEO_HEAD_END (~0u)
> #define MAX_NAME_LEN 20
>
> #define ACPI_VIDEO_DISPLAY_CRT 1
> @@ -1359,11 +1357,15 @@ static int acpi_video_bus_remove_fs(stru
> static struct acpi_video_device_attrib*
> acpi_video_get_device_attr(struct acpi_video_bus *video, unsigned
> long device_id)
> {
> - int count;
> + struct acpi_video_enumerated_device *ids;
> + int i;
> +
> + for (i = 0; i < video->attached_count; i++) {
> + ids = &video->attached_array[i];
> + if ((ids->value.int_val & 0xffff) == device_id)
> + return &ids->value.attrib;
> + }
>
> - for(count = 0; count < video->attached_count; count++)
> - if((video->attached_array[count].value.int_val &
> 0xffff) == device_id)
> - return
> &(video->attached_array[count].value.attrib);
> return NULL;
> }
>
> @@ -1490,20 +1492,16 @@ static void
> acpi_video_device_bind(struct acpi_video_bus *video,
> struct acpi_video_device *device)
> {
> + struct acpi_video_enumerated_device *ids;
> int i;
>
> -#define IDS_VAL(i) video->attached_array[i].value.int_val
> -#define IDS_BIND(i) video->attached_array[i].bind_info
> -
> - for (i = 0; IDS_VAL(i) != ACPI_VIDEO_HEAD_INVALID &&
> - i < video->attached_count; i++) {
> - if (device->device_id == (IDS_VAL(i) & 0xffff)) {
> - IDS_BIND(i) = device;
> + for (i = 0; i < video->attached_count; i++) {
> + ids = &video->attached_array[i];
> + if (device->device_id == (ids->value.int_val &
> 0xffff)) {
> + ids->bind_info = device;
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "device_bind %
> d\n", i));
> }
> }
> -#undef IDS_VAL
> -#undef IDS_BIND
> }
>
> /*
> @@ -1522,7 +1520,7 @@ static int acpi_video_device_enumerate(s
> int status;
> int count;
> int i;
> - struct acpi_video_enumerated_device *active_device_list;
> + struct acpi_video_enumerated_device *active_list;
> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> union acpi_object *dod = NULL;
> union acpi_object *obj;
> @@ -1543,13 +1541,10 @@ static int acpi_video_device_enumerate(s
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d video heads in _DOD
> \n",
> dod->package.count));
>
> - active_device_list = kmalloc((1 +
> - dod->package.count) *
> - sizeof(struct
> -
> acpi_video_enumerated_device),
> - GFP_KERNEL);
> -
> - if (!active_device_list) {
> + active_list = kcalloc(1 + dod->package.count,
> + sizeof(struct
> acpi_video_enumerated_device),
> + GFP_KERNEL);
> + if (!active_list) {
> status = -ENOMEM;
> goto out;
> }
> @@ -1559,23 +1554,24 @@ static int acpi_video_device_enumerate(s
> obj = &dod->package.elements[i];
>
> if (obj->type != ACPI_TYPE_INTEGER) {
> - printk(KERN_ERR PREFIX "Invalid _DOD data\n");
> - active_device_list[i].value.int_val =
> - ACPI_VIDEO_HEAD_INVALID;
> + printk(KERN_ERR PREFIX
> + "Invalid _DOD data in element %d\n",
> i);
> + continue;
> }
> - active_device_list[i].value.int_val =
> obj->integer.value;
> - active_device_list[i].bind_info = NULL;
> +
> + active_list[count].value.int_val = obj->integer.value;
> + active_list[count].bind_info = NULL;
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "dod element[%d] = %d
> \n", i,
> (int)obj->integer.value));
> count++;
> }
> - active_device_list[count].value.int_val = ACPI_VIDEO_HEAD_END;
>
> kfree(video->attached_array);
>
> - video->attached_array = active_device_list;
> + video->attached_array = active_list;
> video->attached_count = count;
> - out:
> +
> + out:
> kfree(buffer.pointer);
> return status;
> }
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-05 16:43 ` [patch 6/8] ACPI: video - properly handle errors when registering proc elements Dmitry Torokhov
@ 2007-11-14 7:18 ` Zhang Rui
2007-11-14 13:36 ` Henrique de Moraes Holschuh
2007-11-14 17:38 ` Len Brown
0 siblings, 2 replies; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:18 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - properly handle errors when registering proc elements
>
> Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
> properly unwind proc creation after error.
Hi, Dmitry,
The patch is good.
But I'm afraid we don't need this patch while we are trying to remove
the proc I/F for all ACPI device drivers.
Thanks,
Rui
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> drivers/acpi/video.c | 230
> +++++++++++++++++++++++++--------------------------
> 1 file changed, 115 insertions(+), 115 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -967,87 +967,90 @@ acpi_video_device_EDID_open_fs(struct in
>
> static int acpi_video_device_add_fs(struct acpi_device *device)
> {
> - struct proc_dir_entry *entry = NULL;
> + struct proc_dir_entry *entry, *device_dir;
> struct acpi_video_device *vid_dev;
>
> -
> - if (!device)
> - return -ENODEV;
> -
> vid_dev = acpi_driver_data(device);
> if (!vid_dev)
> return -ENODEV;
>
> - if (!acpi_device_dir(device)) {
> - acpi_device_dir(device) =
> proc_mkdir(acpi_device_bid(device),
> -
> vid_dev->video->dir);
> - if (!acpi_device_dir(device))
> - return -ENODEV;
> - acpi_device_dir(device)->owner = THIS_MODULE;
> - }
> + device_dir = proc_mkdir(acpi_device_bid(device),
> + vid_dev->video->dir);
> + if (!device_dir)
> + return -ENOMEM;
> +
> + device_dir->owner = THIS_MODULE;
>
> /* 'info' [R] */
> - entry = create_proc_entry("info", S_IRUGO,
> acpi_device_dir(device));
> + entry = create_proc_entry("info", S_IRUGO, device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - entry->proc_fops = &acpi_video_device_info_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_dir;
> +
> + entry->proc_fops = &acpi_video_device_info_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'state' [R/W] */
> - entry =
> - create_proc_entry("state", S_IFREG | S_IRUGO | S_IWUSR,
> - acpi_device_dir(device));
> + entry = create_proc_entry("state", S_IFREG | S_IRUGO |
> S_IWUSR,
> + device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - acpi_video_device_state_fops.write =
> acpi_video_device_write_state;
> - entry->proc_fops = &acpi_video_device_state_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_info;
> +
> + acpi_video_device_state_fops.write =
> acpi_video_device_write_state;
> + entry->proc_fops = &acpi_video_device_state_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'brightness' [R/W] */
> - entry =
> - create_proc_entry("brightness", S_IFREG | S_IRUGO |
> S_IWUSR,
> - acpi_device_dir(device));
> + entry = create_proc_entry("brightness", S_IFREG | S_IRUGO |
> S_IWUSR,
> + device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - acpi_video_device_brightness_fops.write =
> acpi_video_device_write_brightness;
> - entry->proc_fops = &acpi_video_device_brightness_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_state;
> +
> + acpi_video_device_brightness_fops.write =
> + acpi_video_device_write_brightness;
> + entry->proc_fops = &acpi_video_device_brightness_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'EDID' [R] */
> - entry = create_proc_entry("EDID", S_IRUGO,
> acpi_device_dir(device));
> + entry = create_proc_entry("EDID", S_IRUGO, device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - entry->proc_fops = &acpi_video_device_EDID_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_brightness;
>
> + entry->proc_fops = &acpi_video_device_EDID_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
> +
> + acpi_device_dir(device) = device_dir;
> return 0;
> +
> + err_remove_brightness:
> + remove_proc_entry("brightness", device_dir);
> + err_remove_state:
> + remove_proc_entry("state", device_dir);
> + err_remove_info:
> + remove_proc_entry("info", device_dir);
> + err_remove_dir:
> + remove_proc_entry(acpi_device_bid(device),
> vid_dev->video->dir);
> + return -ENOMEM;
> }
>
> static int acpi_video_device_remove_fs(struct acpi_device *device)
> {
> struct acpi_video_device *vid_dev;
> + struct proc_dir_entry *device_dir;
>
> vid_dev = acpi_driver_data(device);
> if (!vid_dev || !vid_dev->video || !vid_dev->video->dir)
> return -ENODEV;
>
> - if (acpi_device_dir(device)) {
> - remove_proc_entry("info", acpi_device_dir(device));
> - remove_proc_entry("state", acpi_device_dir(device));
> - remove_proc_entry("brightness",
> acpi_device_dir(device));
> - remove_proc_entry("EDID", acpi_device_dir(device));
> + device_dir = acpi_device_dir(device);
> + if (device_dir) {
> + remove_proc_entry("info", device_dir);
> + remove_proc_entry("state", device_dir);
> + remove_proc_entry("brightness", device_dir);
> + remove_proc_entry("EDID", device_dir);
> remove_proc_entry(acpi_device_bid(device),
> vid_dev->video->dir);
> acpi_device_dir(device) = NULL;
> }
> @@ -1254,94 +1257,91 @@ acpi_video_bus_write_DOS(struct file *fi
>
> static int acpi_video_bus_add_fs(struct acpi_device *device)
> {
> - struct proc_dir_entry *entry = NULL;
> - struct acpi_video_bus *video;
> + struct acpi_video_bus *video = acpi_driver_data(device);
> + struct proc_dir_entry *device_dir;
> + struct proc_dir_entry *entry;
>
> + device_dir = proc_mkdir(acpi_device_bid(device),
> acpi_video_dir);
> + if (!device_dir)
> + return -ENOMEM;
>
> - video = acpi_driver_data(device);
> -
> - if (!acpi_device_dir(device)) {
> - acpi_device_dir(device) =
> proc_mkdir(acpi_device_bid(device),
> - acpi_video_dir);
> - if (!acpi_device_dir(device))
> - return -ENODEV;
> - video->dir = acpi_device_dir(device);
> - acpi_device_dir(device)->owner = THIS_MODULE;
> - }
> + device_dir->owner = THIS_MODULE;
>
> /* 'info' [R] */
> - entry = create_proc_entry("info", S_IRUGO,
> acpi_device_dir(device));
> + entry = create_proc_entry("info", S_IRUGO, device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - entry->proc_fops = &acpi_video_bus_info_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_dir;
> +
> + entry->proc_fops = &acpi_video_bus_info_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'ROM' [R] */
> - entry = create_proc_entry("ROM", S_IRUGO,
> acpi_device_dir(device));
> + entry = create_proc_entry("ROM", S_IRUGO, device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - entry->proc_fops = &acpi_video_bus_ROM_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_info;
> +
> + entry->proc_fops = &acpi_video_bus_ROM_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'POST_info' [R] */
> - entry =
> - create_proc_entry("POST_info", S_IRUGO,
> acpi_device_dir(device));
> + entry = create_proc_entry("POST_info", S_IRUGO, device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - entry->proc_fops = &acpi_video_bus_POST_info_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_rom;
> +
> + entry->proc_fops = &acpi_video_bus_POST_info_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'POST' [R/W] */
> - entry =
> - create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
> - acpi_device_dir(device));
> + entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
> + device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - acpi_video_bus_POST_fops.write =
> acpi_video_bus_write_POST;
> - entry->proc_fops = &acpi_video_bus_POST_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_post_info;
> +
> + acpi_video_bus_POST_fops.write = acpi_video_bus_write_POST;
> + entry->proc_fops = &acpi_video_bus_POST_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> /* 'DOS' [R/W] */
> - entry =
> - create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
> - acpi_device_dir(device));
> + entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
> + device_dir);
> if (!entry)
> - return -ENODEV;
> - else {
> - acpi_video_bus_DOS_fops.write =
> acpi_video_bus_write_DOS;
> - entry->proc_fops = &acpi_video_bus_DOS_fops;
> - entry->data = acpi_driver_data(device);
> - entry->owner = THIS_MODULE;
> - }
> + goto err_remove_post;
> +
> + acpi_video_bus_DOS_fops.write = acpi_video_bus_write_DOS;
> + entry->proc_fops = &acpi_video_bus_DOS_fops;
> + entry->data = acpi_driver_data(device);
> + entry->owner = THIS_MODULE;
>
> + video->dir = acpi_device_dir(device) = device_dir;
> return 0;
> +
> + err_remove_post:
> + remove_proc_entry("POST", device_dir);
> + err_remove_post_info:
> + remove_proc_entry("POST_info", device_dir);
> + err_remove_rom:
> + remove_proc_entry("ROM", device_dir);
> + err_remove_info:
> + remove_proc_entry("info", device_dir);
> + err_remove_dir:
> + remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
> + return -ENOMEM;
> }
>
> static int acpi_video_bus_remove_fs(struct acpi_device *device)
> {
> - struct acpi_video_bus *video;
> -
> -
> - video = acpi_driver_data(device);
> + struct proc_dir_entry *device_dir = acpi_device_dir(device);
>
> - if (acpi_device_dir(device)) {
> - remove_proc_entry("info", acpi_device_dir(device));
> - remove_proc_entry("ROM", acpi_device_dir(device));
> - remove_proc_entry("POST_info",
> acpi_device_dir(device));
> - remove_proc_entry("POST", acpi_device_dir(device));
> - remove_proc_entry("DOS", acpi_device_dir(device));
> + if (device_dir) {
> + remove_proc_entry("info", device_dir);
> + remove_proc_entry("ROM", device_dir);
> + remove_proc_entry("POST_info", device_dir);
> + remove_proc_entry("POST", device_dir);
> + remove_proc_entry("DOS", device_dir);
> remove_proc_entry(acpi_device_bid(device),
> acpi_video_dir);
> acpi_device_dir(device) = NULL;
> }
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 7/8] ACPI: video - more cleanups
2007-11-05 16:43 ` [patch 7/8] ACPI: video - more cleanups Dmitry Torokhov
@ 2007-11-14 7:18 ` Zhang Rui
2007-11-14 17:42 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:18 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - more cleanups
>
> Remove unneeded checks and initializations, implement proper
> unwinding after errors in initialization code, get rid of
> unneeded casts, adjust formatting.
A big patch with a large number of minor cleanups/fixes.
Thanks for your work, Dmitry.
I'm okay with this one except the comment below.
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> @@ -1772,12 +1783,11 @@ static int acpi_video_bus_stop_devices(s
> static void acpi_video_bus_notify(acpi_handle handle, u32 event, void
> *data)
> {
> struct acpi_video_bus *video = data;
> - struct acpi_device *device = NULL;
> + struct acpi_device *device;
> struct input_dev *input;
> int keycode;
>
> -
> - printk("video bus notify\n");
> + printk(KERN_DEBUG "video bus notify\n");
This debug message should be removed.
Thanks,
Rui
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 8/8] ACPI: video - fix permissions on some proc entries
2007-11-05 16:43 ` [patch 8/8] ACPI: video - fix permissions on some proc entries Dmitry Torokhov
@ 2007-11-14 7:18 ` Zhang Rui
0 siblings, 0 replies; 30+ messages in thread
From: Zhang Rui @ 2007-11-14 7:18 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Brown, Len, linux-acpi
On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> ACPI: video - fix permissions on some proc entries
>
> POST and DOS are supposed to be writable but permissions
> did not allow it.
The same reason as patch 6. :)
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> drivers/acpi/video.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -1276,7 +1276,7 @@ static int acpi_video_bus_add_fs(struct
> entry->owner = THIS_MODULE;
>
> /* 'POST' [R/W] */
> - entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IRUSR,
> + entry = create_proc_entry("POST", S_IFREG | S_IRUGO | S_IWUSR,
> device_dir);
> if (!entry)
> goto err_remove_post_info;
> @@ -1287,7 +1287,7 @@ static int acpi_video_bus_add_fs(struct
> entry->owner = THIS_MODULE;
>
> /* 'DOS' [R/W] */
> - entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IRUSR,
> + entry = create_proc_entry("DOS", S_IFREG | S_IRUGO | S_IWUSR,
> device_dir);
> if (!entry)
> goto err_remove_post;
>
> --
> Dmitry
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-14 7:18 ` Zhang Rui
@ 2007-11-14 13:36 ` Henrique de Moraes Holschuh
2007-11-14 15:06 ` Dmitry Torokhov
2007-11-14 17:38 ` Len Brown
1 sibling, 1 reply; 30+ messages in thread
From: Henrique de Moraes Holschuh @ 2007-11-14 13:36 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, Brown, Len, linux-acpi
On Wed, 14 Nov 2007, Zhang Rui wrote:
> On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > ACPI: video - properly handle errors when registering proc elements
> >
> > Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
> > properly unwind proc creation after error.
> Hi, Dmitry,
>
> The patch is good.
> But I'm afraid we don't need this patch while we are trying to remove
> the proc I/F for all ACPI device drivers.
IMHO, until procfs is gone for good, it is best to make sure it works
perfectly.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 0/8] ACPI Video various cleanups & fixes
2007-11-14 7:17 ` [patch 0/8] ACPI Video various cleanups & fixes Zhang Rui
@ 2007-11-14 15:04 ` Dmitry Torokhov
0 siblings, 0 replies; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-14 15:04 UTC (permalink / raw)
To: Zhang Rui, Brown, Len; +Cc: linux-acpi
On 11/14/07, Zhang Rui <rui.zhang@intel.com> wrote:
> On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > Hi Len, Rui,
> >
> > Here is a group of cleanups and patches to the acpi video driver.
> > Originally I just wanted to fix sysfs placement of input devices
> > created by the driver, but then I saw some more potential issues.
> >
> > The patches are against Linus's tree, please let me know if you
> > want them redone against something else.
> >
> Hi, Dmitry,
>
> Thanks for your patches and sorry for the delay.
>
> The patches are good and I'm okay with most of them.
> Detailed comments are inline.
Thank you very much for reviewing them.
Len, it would be nice if the very first patch (sysfs one) made in 2.6.24.
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-14 13:36 ` Henrique de Moraes Holschuh
@ 2007-11-14 15:06 ` Dmitry Torokhov
2007-11-14 17:22 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Dmitry Torokhov @ 2007-11-14 15:06 UTC (permalink / raw)
To: Henrique de Moraes Holschuh; +Cc: Zhang Rui, Brown, Len, linux-acpi
On 11/14/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> On Wed, 14 Nov 2007, Zhang Rui wrote:
> > On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > > ACPI: video - properly handle errors when registering proc elements
> > >
> > > Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
> > > properly unwind proc creation after error.
> > Hi, Dmitry,
> >
> > The patch is good.
> > But I'm afraid we don't need this patch while we are trying to remove
> > the proc I/F for all ACPI device drivers.
>
> IMHO, until procfs is gone for good, it is best to make sure it works
> perfectly.
>
That is my position as well. The removal is scheduled for July 2008
which is 9 months out.
--
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 1/8] ACPI: video - fit input device into sysfs tree
2007-11-14 7:17 ` Zhang Rui
@ 2007-11-14 16:49 ` Len Brown
0 siblings, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 16:49 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, Brown, Len, linux-acpi
On Wednesday 14 November 2007 02:17, Zhang Rui wrote:
> Acked-by: Zhang Rui <rui.zhang@intel.com>
applied.
thanks,
-len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 2/8] ACPI: video - add missing input_free_device()
2007-11-05 16:43 ` [patch 2/8] ACPI: video - add missing input_free_device() Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
@ 2007-11-14 17:00 ` Len Brown
1 sibling, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:00 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Rui Zhang, linux-acpi
On Monday 05 November 2007 11:43, Dmitry Torokhov wrote:
> ACPI: video - add missing input_free_device()
applied.
thanks,
-len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 4/8] ACPI: video - convert semaphore to a mutex
2007-11-14 7:17 ` Zhang Rui
@ 2007-11-14 17:18 ` Len Brown
0 siblings, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:18 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, linux-acpi
On Wednesday 14 November 2007 02:17, Zhang Rui wrote:
> On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > ACPI: video - convert semaphore to a mutex
> >
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> Acked-by: Zhang Rui <rui.zhang@intel.com>
applied.
thanks,
-Len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-14 15:06 ` Dmitry Torokhov
@ 2007-11-14 17:22 ` Len Brown
0 siblings, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Henrique de Moraes Holschuh, Zhang Rui, linux-acpi
On Wednesday 14 November 2007 10:06, Dmitry Torokhov wrote:
> On 11/14/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> > On Wed, 14 Nov 2007, Zhang Rui wrote:
> > > On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > > > ACPI: video - properly handle errors when registering proc elements
> > > >
> > > > Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
> > > > properly unwind proc creation after error.
> > > Hi, Dmitry,
> > >
> > > The patch is good.
> > > But I'm afraid we don't need this patch while we are trying to remove
> > > the proc I/F for all ACPI device drivers.
> >
> > IMHO, until procfs is gone for good, it is best to make sure it works
> > perfectly.
> >
>
> That is my position as well. The removal is scheduled for July 2008
> which is 9 months out.
Agreed.
While we don't want to add (and support) any new /proc features,
unwinding from errors properly with the existing features
seems like a good idea.
thanks,
-Len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 5/8] ACPI: video - simplify handling of attached devices
2007-11-14 7:17 ` Zhang Rui
@ 2007-11-14 17:34 ` Len Brown
0 siblings, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:34 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, linux-acpi
On Wednesday 14 November 2007 02:17, Zhang Rui wrote:
> Acked-by: Zhang Rui <rui.zhang@intel.com>
applied to test branch.
thanks,
-Len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 6/8] ACPI: video - properly handle errors when registering proc elements
2007-11-14 7:18 ` Zhang Rui
2007-11-14 13:36 ` Henrique de Moraes Holschuh
@ 2007-11-14 17:38 ` Len Brown
1 sibling, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:38 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, linux-acpi
On Wednesday 14 November 2007 02:18, Zhang Rui wrote:
> On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > ACPI: video - properly handle errors when registering proc elements
> >
> > Have acpi_video_device_add_fs() and acpi_video_bus_add_fs()
> > properly unwind proc creation after error.
> Hi, Dmitry,
>
> The patch is good.
> But I'm afraid we don't need this patch while we are trying to remove
> the proc I/F for all ACPI device drivers.
applied to test branch & queued for 2.6.25. I'm okay with fixing it, but
it doesn't seem trivial enough and urgent enough for post 24-rc2.
thanks,
-len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 7/8] ACPI: video - more cleanups
2007-11-14 7:18 ` Zhang Rui
@ 2007-11-14 17:42 ` Len Brown
2007-11-14 17:47 ` Len Brown
0 siblings, 1 reply; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:42 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, linux-acpi
On Wednesday 14 November 2007 02:18, Zhang Rui wrote:
> On Tue, 2007-11-06 at 00:43 +0800, Dmitry Torokhov wrote:
> > ACPI: video - more cleanups
> >
> > Remove unneeded checks and initializations, implement proper
> > unwinding after errors in initialization code, get rid of
> > unneeded casts, adjust formatting.
> A big patch with a large number of minor cleanups/fixes.
> Thanks for your work, Dmitry.
> I'm okay with this one except the comment below.
>
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>
> > @@ -1772,12 +1783,11 @@ static int acpi_video_bus_stop_devices(s
> > static void acpi_video_bus_notify(acpi_handle handle, u32 event, void
> > *data)
> > {
> > struct acpi_video_bus *video = data;
> > - struct acpi_device *device = NULL;
> > + struct acpi_device *device;
> > struct input_dev *input;
> > int keycode;
> >
> > -
> > - printk("video bus notify\n");
> > + printk(KERN_DEBUG "video bus notify\n");
> This debug message should be removed.
wups, yes I remember that one...
I'll delete that in .24 and queue the rest of Dmitry's cleanup for .25.
thanks,
-Len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe()
2007-11-05 16:43 ` [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe() Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
@ 2007-11-14 17:46 ` Len Brown
1 sibling, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:46 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Rui Zhang, linux-acpi
On Monday 05 November 2007 11:43, Dmitry Torokhov wrote:
> ACPI: video - remove unsafe uses of list_for_each_safe()
>
> list_for_each_safe() only protects list from list alterations
> performed by the same thread. One still needs to implement
> proper locking when list is being accessed from several threads.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> drivers/acpi/video.c | 71 ++++++++++++++++++++++++---------------------------
> 1 file changed, 34 insertions(+), 37 deletions(-)
>
> Index: work/drivers/acpi/video.c
> ===================================================================
> --- work.orig/drivers/acpi/video.c
> +++ work/drivers/acpi/video.c
> @@ -1462,12 +1462,14 @@ acpi_video_bus_get_one_device(struct acp
>
> static void acpi_video_device_rebind(struct acpi_video_bus *video)
> {
> - struct list_head *node, *next;
> - list_for_each_safe(node, next, &video->video_device_list) {
yeah, these were mis-used -- they're just for the case
where we delete the node inside the loop and don't
want to test the node->next.
applied.
thanks,
-Len
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch 7/8] ACPI: video - more cleanups
2007-11-14 17:42 ` Len Brown
@ 2007-11-14 17:47 ` Len Brown
0 siblings, 0 replies; 30+ messages in thread
From: Len Brown @ 2007-11-14 17:47 UTC (permalink / raw)
To: Zhang Rui; +Cc: Dmitry Torokhov, linux-acpi
> > > - printk("video bus notify\n");
> > > + printk(KERN_DEBUG "video bus notify\n");
> > This debug message should be removed.
>
> wups, yes I remember that one...
> I'll delete that in .24 and queue the rest of Dmitry's cleanup for .25.
>
Hmmm, looks like another stray printk down below -- should I nuke that too?
static int acpi_video_bus_POST_info_seq_show(struct seq_file *seq, void *offset)
{
struct acpi_video_bus *video = seq->private;
unsigned long options;
int status;
if (!video)
goto end;
status = acpi_video_bus_POST_options(video, &options);
if (ACPI_SUCCESS(status)) {
if (!(options & 1)) {
printk(KERN_WARNING PREFIX
"The motherboard VGA device is not listed as a possible POST device.\n");
printk(KERN_WARNING PREFIX
"This indicates a BIOS bug. Please contact the manufacturer.\n");
}
-->>> printk("%lx\n", options);
seq_printf(seq, "can POST: <integrated video>");
if (options & 2)
seq_printf(seq, " <PCI video>");
if (options & 4)
seq_printf(seq, " <AGP video>");
seq_putc(seq, '\n');
} else
seq_printf(seq, "<not supported>\n");
end:
return 0;
}
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2007-11-14 17:47 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-05 16:43 [patch 0/8] ACPI Video various cleanups & fixes Dmitry Torokhov
2007-11-05 16:43 ` [patch 1/8] ACPI: video - fit input device into sysfs tree Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 16:49 ` Len Brown
2007-11-05 16:43 ` [patch 2/8] ACPI: video - add missing input_free_device() Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:00 ` Len Brown
2007-11-05 16:43 ` [patch 3/8] ACPI: video - remove unsafe uses of list_for_each_safe() Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:46 ` Len Brown
2007-11-05 16:43 ` [patch 4/8] ACPI: video - convert semaphore to a mutex Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:18 ` Len Brown
2007-11-05 16:43 ` [patch 5/8] ACPI: video - simplify handling of attached devices Dmitry Torokhov
2007-11-14 7:17 ` Zhang Rui
2007-11-14 17:34 ` Len Brown
2007-11-05 16:43 ` [patch 6/8] ACPI: video - properly handle errors when registering proc elements Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-14 13:36 ` Henrique de Moraes Holschuh
2007-11-14 15:06 ` Dmitry Torokhov
2007-11-14 17:22 ` Len Brown
2007-11-14 17:38 ` Len Brown
2007-11-05 16:43 ` [patch 7/8] ACPI: video - more cleanups Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-14 17:42 ` Len Brown
2007-11-14 17:47 ` Len Brown
2007-11-05 16:43 ` [patch 8/8] ACPI: video - fix permissions on some proc entries Dmitry Torokhov
2007-11-14 7:18 ` Zhang Rui
2007-11-14 7:17 ` [patch 0/8] ACPI Video various cleanups & fixes Zhang Rui
2007-11-14 15:04 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox