Skip to contentSkip to conversation

ACPI video: Harden video bus adding.

PATCH Dmitry Torokhov 2 participants 4 messages Feb 29, 2012 2 messages not in the archive ACPI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20120229035142.31016e6f@garik

Dmitry Torokhov

author
#1 lore raw

Hi Igor,

Quoted 33 lines · Igor Murzov

From 127b9fc18ee646d92039c45d7fa3680a107928a2 Mon Sep 17 00:00:00 2001
From: Igor Murzov <[email protected]>
Date: Mon, 27 Feb 2012 21:50:17 +0400
Subject: [PATCH 1/2] ACPI video: Harden video bus adding.

It is always better to check return values.

Signed-off-by Igor Murzov <[email protected]>
---
 drivers/acpi/video.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
drivers/acpi/video.c +2 -2
index eaef02a..1bc6370 100644
@@ -548,24 +548,25 @@ acpi_video_device_EDID(struct acpi_video_device *device,
  *		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.
+ * 		-EINVAL	wrong arg.
  */
 
 static int
 acpi_video_bus_DOS(struct acpi_video_bus *video, int bios_flag, int lcd_flag)
 {
-	u64 status = 0;
+	int status = 0;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };

drivers/acpi/video.c acpi_video_device_EDID(struct acpi_video_device *device, · line 569

Quoted 10 lines
drivers/acpi/video.c +3 -2
 	if (bios_flag < 0 || bios_flag > 3 || lcd_flag < 0 || lcd_flag > 1) {
-		status = -1;
+		status = -EINVAL;
 		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);
+	status = acpi_evaluate_object(video->device->handle, "_DOS",
+	                              &args, NULL);

Here (as well as in the rest of your patch) you are mixing up errors from the standard Unix -E* namespace and ACPI AE_* namespace. You need to settle on one and be consistent (and also make sure that upper layers get wha they expect, namely -E* namespace).

Thanks.

Signature · 1 line
Dmitry
#2 lore raw
Quoted 36 lines · On Tue, 28 Feb 2012 16:12:59 -0800 Dmitry Torokhov <[email protected]>…

Hi Igor,

Quoted 33 lines · Igor Murzov

From 127b9fc18ee646d92039c45d7fa3680a107928a2 Mon Sep 17 00:00:00 2001
From: Igor Murzov <[email protected]>
Date: Mon, 27 Feb 2012 21:50:17 +0400
Subject: [PATCH 1/2] ACPI video: Harden video bus adding.

It is always better to check return values.

Signed-off-by Igor Murzov <[email protected]>
---
 drivers/acpi/video.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
drivers/acpi/video.c +2 -2
index eaef02a..1bc6370 100644
@@ -548,24 +548,25 @@ acpi_video_device_EDID(struct acpi_video_device *device,
  *		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.
+ * 		-EINVAL	wrong arg.
  */
 
 static int
 acpi_video_bus_DOS(struct acpi_video_bus *video, int bios_flag, int lcd_flag)
 {
-	u64 status = 0;
+	int status = 0;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
Quoted 15 lines

drivers/acpi/video.c acpi_video_device_EDID(struct acpi_video_device *device, · line 569

Quoted 10 lines
drivers/acpi/video.c +3 -2
 	if (bios_flag < 0 || bios_flag > 3 || lcd_flag < 0 || lcd_flag > 1) {
-		status = -1;
+		status = -EINVAL;
 		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);
+	status = acpi_evaluate_object(video->device->handle, "_DOS",
+	                              &args, NULL);

Here (as well as in the rest of your patch) you are mixing up errors from the standard Unix -E* namespace and ACPI AE_* namespace. You need to settle on one and be consistent (and also make sure that upper layers get wha they expect, namely -E* namespace).

Thanks for the reply. Will fix this and resend.

-- Igor

A message that is not in the archive (sent off-list or not yet received): 20120229035518.306c57a8@garik

author
#4 lore raw

Re: [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered

Quoted 26 lines · Igor Murzov

From 202f25d44b1cf871697c88a6a3a8d854674db4d8 Mon Sep 17 00:00:00 2001
From: Igor Murzov <[email protected]>
Date: Mon, 27 Feb 2012 21:57:59 +0400
Subject: [PATCH 2/2] ACPI video: Don't start video device until its
associated input device has been reigstered

---
 drivers/acpi/video.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/video.c +0 -3
index 1bc6370..b9e114c 100644
@@ -1659,9 +1659,6 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	error = acpi_video_bus_get_devices(video, device);
 	if (error)
 		goto err_free_video;
-	error = acpi_video_bus_start_devices(video);
-	if (error)
-		goto err_put_video;
 
 	video->input = input = input_allocate_device();
 	if (!input) {

drivers/acpi/video.c static int acpi_video_bus_add(struct acpi_device *device) · line 1706

Quoted 7 lines
drivers/acpi/video.c +4 -0
@@ -1703,13 +1700,18 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	if (error)
 		goto err_free_input_dev;
 
+	error = acpi_video_bus_start_devices(video);
+	if (error)
+		goto err_unregister_pm_notifier;
+

Registered input device should be unregistered with input_unregister_device(), not input_free_device(). Your change breaks this.

Also, notify() will not be called until acpi_video_bus_add() completes so current code should be perfectly fine.

Quoted 15 lines
drivers/acpi/video.c +2 -1
 	return 0;
 
+ err_unregister_pm_notifier:
+	unregister_pm_notifier(&video->pm_nb);
  err_free_input_dev:
 	input_free_device(input);
  err_stop_video:
 	acpi_video_bus_stop_devices(video);
- err_put_video:
 	acpi_video_bus_put_devices(video);
 	kfree(video->attached_array);
  err_free_video:
Signature · 1 line
1.7.5.1

Thanks.

Signature · 1 line
Dmitry
author
#5 lore raw

Re: [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered

Quoted 39 lines · Dmitry Torokhov
Quoted 26 lines · Igor Murzov

From 202f25d44b1cf871697c88a6a3a8d854674db4d8 Mon Sep 17 00:00:00 2001
From: Igor Murzov <[email protected]>
Date: Mon, 27 Feb 2012 21:57:59 +0400
Subject: [PATCH 2/2] ACPI video: Don't start video device until its
associated input device has been reigstered

---
 drivers/acpi/video.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/video.c +0 -3
index 1bc6370..b9e114c 100644
@@ -1659,9 +1659,6 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	error = acpi_video_bus_get_devices(video, device);
 	if (error)
 		goto err_free_video;
-	error = acpi_video_bus_start_devices(video);
-	if (error)
-		goto err_put_video;
 
 	video->input = input = input_allocate_device();
 	if (!input) {

drivers/acpi/video.c static int acpi_video_bus_add(struct acpi_device *device) · line 1706

Quoted 7 lines
drivers/acpi/video.c +4 -0
@@ -1703,13 +1700,18 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	if (error)
 		goto err_free_input_dev;
 
+	error = acpi_video_bus_start_devices(video);
+	if (error)
+		goto err_unregister_pm_notifier;
+

Registered input device should be unregistered with input_unregister_device(), not input_free_device(). Your change breaks this.

Quoted 2 lines

Also, notify() will not be called until acpi_video_bus_add() completes so current code should be perfectly fine.

Hm, I take the last paragraph back - in addition to bus notifier we do install device notifier explicitly so it might fire up early. The easiest fox would be to move acpi_video_bus_start_devices after input_allocate_device() but before input_refister_device() - unregistered input devoces can handle input_event() calls just fine.

Signature · 1 line
Dmitry