* [PATCH] acpi: video: correct acpi_video_bus_add error processing
@ 2013-04-25 2:47 Aaron Lu
2013-04-25 11:03 ` Rafael J. Wysocki
0 siblings, 1 reply; 2+ messages in thread
From: Aaron Lu @ 2013-04-25 2:47 UTC (permalink / raw)
To: Rafael J. Wysocki, i-tek; +Cc: Igor Murzov, ACPI Devel Mailing List, Zhang Rui
acpi_video_bus_get_devices may fail due to some video output device
doesn't have the _ADR method, and in this case, the error processing is
to simply free the video structure in acpi_video_bus_add, while leaving
those already registered video output devices in the wild, which means
for some video output device, we have already registered a backlight
interface and installed a notification handler for it. So it can happen
when user is using this system, on hotkey pressing, the notification
handler will send a keycode through a non-existing input device, causing
kernel freeze.
To solve this problem, we free all those already registered video output
devices once something goes wrong in acpi_video_bus_get_devices so that
no wild backlight interfaces and notification handlers exist.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=51731
Reported-bisected-and-tested-by: <i-tek@web.de>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
drivers/acpi/video.c | 143 ++++++++++++++++++++++++---------------------------
1 file changed, 66 insertions(+), 77 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index ed192e5..c3932d0 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -167,7 +167,8 @@ struct acpi_video_device_flags {
u8 dvi:1;
u8 bios:1;
u8 unknown:1;
- u8 reserved:2;
+ u8 notify:1;
+ u8 reserved:1;
};
struct acpi_video_device_cap {
@@ -1074,53 +1075,51 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
struct acpi_video_device *data;
struct acpi_video_device_attrib* attribute;
- if (!device || !video)
- return -EINVAL;
-
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;
-
- strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
- strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
- device->driver_data = data;
-
- data->device_id = device_id;
- data->video = video;
- data->dev = device;
+ /* Some device omits _ADR, we skip them instead of fail */
+ if (ACPI_FAILURE(status))
+ return 0;
- attribute = acpi_video_get_device_attr(video, device_id);
+ data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
- 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 {
- /* Check for legacy IDs */
- device_type = acpi_video_get_device_type(video,
- device_id);
- /* Ignore bits 16 and 18-20 */
- switch (device_type & 0xffe2ffff) {
+ strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
+ strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
+ device->driver_data = 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 {
+ /* Check for legacy IDs */
+ device_type = acpi_video_get_device_type(video, device_id);
+ /* Ignore bits 16 and 18-20 */
+ switch (device_type & 0xffe2ffff) {
case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
data->flags.crt = 1;
break;
@@ -1132,34 +1131,24 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
break;
default:
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)) {
- printk(KERN_ERR PREFIX
- "Error installing notify handler\n");
- if(data->brightness)
- kfree(data->brightness->levels);
- kfree(data->brightness);
- kfree(data);
- return -ENODEV;
- }
+ acpi_video_device_bind(video, data);
+ acpi_video_device_find_cap(data);
- mutex_lock(&video->device_list_lock);
- list_add_tail(&data->entry, &video->video_device_list);
- mutex_unlock(&video->device_list_lock);
+ status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+ acpi_video_device_notify, data);
+ if (ACPI_FAILURE(status))
+ dev_err(&device->dev, "Error installing notify handler\n");
+ else
+ data->flags.notify = 1;
- return 0;
- }
+ mutex_lock(&video->device_list_lock);
+ list_add_tail(&data->entry, &video->video_device_list);
+ mutex_unlock(&video->device_list_lock);
- return -ENOENT;
+ return status;
}
/*
@@ -1452,9 +1441,8 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
status = acpi_video_bus_get_one_device(dev, video);
if (status) {
- printk(KERN_WARNING PREFIX
- "Can't attach device\n");
- continue;
+ dev_err(&dev->dev, "Can't attach device\n");
+ break;
}
}
return status;
@@ -1467,13 +1455,14 @@ static int acpi_video_bus_put_one_device(struct acpi_video_device *device)
if (!device || !device->video)
return -ENOENT;
- status = acpi_remove_notify_handler(device->dev->handle,
- ACPI_DEVICE_NOTIFY,
- acpi_video_device_notify);
- if (ACPI_FAILURE(status)) {
- printk(KERN_WARNING PREFIX
- "Can't remove video notify handler\n");
+ if (device->flags.notify) {
+ status = acpi_remove_notify_handler(device->dev->handle,
+ ACPI_DEVICE_NOTIFY, acpi_video_device_notify);
+ if (ACPI_FAILURE(status))
+ dev_err(&device->dev->dev,
+ "Can't remove video notify handler\n");
}
+
if (device->backlight) {
backlight_device_unregister(device->backlight);
device->backlight = NULL;
@@ -1755,7 +1744,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
error = acpi_video_bus_get_devices(video, device);
if (error)
- goto err_free_video;
+ goto err_put_video;
video->input = input = input_allocate_device();
if (!input) {
--
1.8.2.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] acpi: video: correct acpi_video_bus_add error processing
2013-04-25 2:47 [PATCH] acpi: video: correct acpi_video_bus_add error processing Aaron Lu
@ 2013-04-25 11:03 ` Rafael J. Wysocki
0 siblings, 0 replies; 2+ messages in thread
From: Rafael J. Wysocki @ 2013-04-25 11:03 UTC (permalink / raw)
To: Aaron Lu; +Cc: i-tek, Igor Murzov, ACPI Devel Mailing List, Zhang Rui
On Thursday, April 25, 2013 10:47:49 AM Aaron Lu wrote:
> acpi_video_bus_get_devices may fail due to some video output device
> doesn't have the _ADR method, and in this case, the error processing is
> to simply free the video structure in acpi_video_bus_add, while leaving
> those already registered video output devices in the wild, which means
> for some video output device, we have already registered a backlight
> interface and installed a notification handler for it. So it can happen
> when user is using this system, on hotkey pressing, the notification
> handler will send a keycode through a non-existing input device, causing
> kernel freeze.
>
> To solve this problem, we free all those already registered video output
> devices once something goes wrong in acpi_video_bus_get_devices so that
> no wild backlight interfaces and notification handlers exist.
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=51731
> Reported-bisected-and-tested-by: <i-tek@web.de>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Applied.
Thanks,
Rafael
> ---
> drivers/acpi/video.c | 143 ++++++++++++++++++++++++---------------------------
> 1 file changed, 66 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index ed192e5..c3932d0 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -167,7 +167,8 @@ struct acpi_video_device_flags {
> u8 dvi:1;
> u8 bios:1;
> u8 unknown:1;
> - u8 reserved:2;
> + u8 notify:1;
> + u8 reserved:1;
> };
>
> struct acpi_video_device_cap {
> @@ -1074,53 +1075,51 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
> struct acpi_video_device *data;
> struct acpi_video_device_attrib* attribute;
>
> - if (!device || !video)
> - return -EINVAL;
> -
> 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;
> -
> - strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
> - strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> - device->driver_data = data;
> -
> - data->device_id = device_id;
> - data->video = video;
> - data->dev = device;
> + /* Some device omits _ADR, we skip them instead of fail */
> + if (ACPI_FAILURE(status))
> + return 0;
>
> - attribute = acpi_video_get_device_attr(video, device_id);
> + data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>
> - 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 {
> - /* Check for legacy IDs */
> - device_type = acpi_video_get_device_type(video,
> - device_id);
> - /* Ignore bits 16 and 18-20 */
> - switch (device_type & 0xffe2ffff) {
> + strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
> + strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> + device->driver_data = 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 {
> + /* Check for legacy IDs */
> + device_type = acpi_video_get_device_type(video, device_id);
> + /* Ignore bits 16 and 18-20 */
> + switch (device_type & 0xffe2ffff) {
> case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
> data->flags.crt = 1;
> break;
> @@ -1132,34 +1131,24 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
> break;
> default:
> 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)) {
> - printk(KERN_ERR PREFIX
> - "Error installing notify handler\n");
> - if(data->brightness)
> - kfree(data->brightness->levels);
> - kfree(data->brightness);
> - kfree(data);
> - return -ENODEV;
> - }
> + acpi_video_device_bind(video, data);
> + acpi_video_device_find_cap(data);
>
> - mutex_lock(&video->device_list_lock);
> - list_add_tail(&data->entry, &video->video_device_list);
> - mutex_unlock(&video->device_list_lock);
> + status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> + acpi_video_device_notify, data);
> + if (ACPI_FAILURE(status))
> + dev_err(&device->dev, "Error installing notify handler\n");
> + else
> + data->flags.notify = 1;
>
> - return 0;
> - }
> + mutex_lock(&video->device_list_lock);
> + list_add_tail(&data->entry, &video->video_device_list);
> + mutex_unlock(&video->device_list_lock);
>
> - return -ENOENT;
> + return status;
> }
>
> /*
> @@ -1452,9 +1441,8 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
>
> status = acpi_video_bus_get_one_device(dev, video);
> if (status) {
> - printk(KERN_WARNING PREFIX
> - "Can't attach device\n");
> - continue;
> + dev_err(&dev->dev, "Can't attach device\n");
> + break;
> }
> }
> return status;
> @@ -1467,13 +1455,14 @@ static int acpi_video_bus_put_one_device(struct acpi_video_device *device)
> if (!device || !device->video)
> return -ENOENT;
>
> - status = acpi_remove_notify_handler(device->dev->handle,
> - ACPI_DEVICE_NOTIFY,
> - acpi_video_device_notify);
> - if (ACPI_FAILURE(status)) {
> - printk(KERN_WARNING PREFIX
> - "Can't remove video notify handler\n");
> + if (device->flags.notify) {
> + status = acpi_remove_notify_handler(device->dev->handle,
> + ACPI_DEVICE_NOTIFY, acpi_video_device_notify);
> + if (ACPI_FAILURE(status))
> + dev_err(&device->dev->dev,
> + "Can't remove video notify handler\n");
> }
> +
> if (device->backlight) {
> backlight_device_unregister(device->backlight);
> device->backlight = NULL;
> @@ -1755,7 +1744,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
>
> error = acpi_video_bus_get_devices(video, device);
> if (error)
> - goto err_free_video;
> + goto err_put_video;
>
> video->input = input = input_allocate_device();
> if (!input) {
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-04-25 10:54 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-25 2:47 [PATCH] acpi: video: correct acpi_video_bus_add error processing Aaron Lu
2013-04-25 11:03 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox