[PATCH 1/4] acpi_video: Revert "ACPI / video: driver must be registered before checking for keypresses"

[PATCH 1/4] acpi_video: Revert "ACPI / video: driver must be registered before checking for keypresses"

[Hans de Goede]

On systems with an intel video opcode region, the completion used in the
patch this commit reverts will only complete if the i915 driver loads.
If for some reason the i915 driver never loads calls to
acpi_video_handles_brightness_key_presses() may be delayed indefinitely.

This reverts commit aecbd9b1bff6 ("ACPI / video: driver must be registered
before checking for keypresses") fixing this.

This reintroduces a potential NULL pointer deref due to using an
uninitalized mutex, this is fixed differently in a follow-up patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 15863a8..d95aaa5 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -90,8 +90,8 @@ module_param(device_id_scheme, bool, 0444);
 static bool only_lcd = false;
 module_param(only_lcd, bool, 0444);
 
-static DECLARE_COMPLETION(register_done);
-static DEFINE_MUTEX(register_done_mutex);
+static int register_count;
+static DEFINE_MUTEX(register_count_mutex);
 static struct mutex video_list_lock;
 static struct list_head video_bus_head;
 static int acpi_video_bus_add(struct acpi_device *device);
@@ -2058,8 +2058,8 @@ int acpi_video_register(void)
 {
 	int ret = 0;
 
-	mutex_lock(&register_done_mutex);
-	if (completion_done(&register_done)) {
+	mutex_lock(&register_count_mutex);
+	if (register_count) {
 		/*
 		 * if the function of acpi_video_register is already called,
 		 * don't register the acpi_vide_bus again and return no error.
@@ -2080,22 +2080,22 @@ int acpi_video_register(void)
 	 * When the acpi_video_bus is loaded successfully, increase
 	 * the counter reference.
 	 */
-	complete(&register_done);
+	register_count = 1;
 
 leave:
-	mutex_unlock(&register_done_mutex);
+	mutex_unlock(&register_count_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(acpi_video_register);
 
 void acpi_video_unregister(void)
 {
-	mutex_lock(&register_done_mutex);
-	if (completion_done(&register_done)) {
+	mutex_lock(&register_count_mutex);
+	if (register_count) {
 		acpi_bus_unregister_driver(&acpi_video_bus);
-		reinit_completion(&register_done);
+		register_count = 0;
 	}
-	mutex_unlock(&register_done_mutex);
+	mutex_unlock(&register_count_mutex);
 }
 EXPORT_SYMBOL(acpi_video_unregister);
 
@@ -2103,21 +2103,20 @@ void acpi_video_unregister_backlight(void)
 {
 	struct acpi_video_bus *video;
 
-	mutex_lock(&register_done_mutex);
-	if (completion_done(&register_done)) {
+	mutex_lock(&register_count_mutex);
+	if (register_count) {
 		mutex_lock(&video_list_lock);
 		list_for_each_entry(video, &video_bus_head, entry)
 			acpi_video_bus_unregister_backlight(video);
 		mutex_unlock(&video_list_lock);
 	}
-	mutex_unlock(&register_done_mutex);
+	mutex_unlock(&register_count_mutex);
 }
 
 bool acpi_video_handles_brightness_key_presses(void)
 {
 	bool have_video_busses;
 
-	wait_for_completion(&register_done);
 	mutex_lock(&video_list_lock);
 	have_video_busses = !list_empty(&video_bus_head);
 	mutex_unlock(&video_list_lock);
-- 
2.5.0

[PATCH 2/4] acpi_video: Fix using an uninitialized mutex / list_head in acpi_video_handles_brightness_key_presses()

[Hans de Goede]

If acpi_video_handles_brightness_key_presses() was called before
acpi_video_register(), it would use the video_list mutex / list_head
uninitialized.

This patch fixes this by using DEFINE_MUTEX / LIST_HEAD when declaring
these, instead of initializing them runtime from acpi_video_register().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index d95aaa5..408b014 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -92,8 +92,8 @@ module_param(only_lcd, bool, 0444);
 
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);
-static struct mutex video_list_lock;
-static struct list_head video_bus_head;
+static DEFINE_MUTEX(video_list_lock);
+static LIST_HEAD(video_bus_head);
 static int acpi_video_bus_add(struct acpi_device *device);
 static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
@@ -2067,9 +2067,6 @@ int acpi_video_register(void)
 		goto leave;
 	}
 
-	mutex_init(&video_list_lock);
-	INIT_LIST_HEAD(&video_bus_head);
-
 	dmi_check_system(video_dmi_table);
 
 	ret = acpi_bus_register_driver(&acpi_video_bus);
-- 
2.5.0

[PATCH 3/4] acpi_video: Document acpi_video_handles_brightness_key_presses() a bit

[Hans de Goede]

Document that acpi_video_handles_brightness_key_presses()'s return value
may change over time and should not be cached.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/acpi/video.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/acpi/video.h b/include/acpi/video.h
index f11d342..5ca2f2c 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -32,6 +32,10 @@ extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 extern enum acpi_backlight_type acpi_video_get_backlight_type(void);
 extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type);
+/*
+ * Note: The value returned by acpi_video_handles_brightness_key_presses()
+ * may change over time and should not be cached.
+ */
 extern bool acpi_video_handles_brightness_key_presses(void);
 #else
 static inline int acpi_video_register(void) { return 0; }
-- 
2.5.0

[PATCH 4/4] Revert "thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()"

[Hans de Goede]

acpi_video_handles_brightness_key_presses()'s may return false if the i915
driver is not loaded yet when thinkpad_acpi loads, and then return true
after the i915 driver has loaded. This means that thinkpad_acpi cannot
use it as is since thinkpad_acpi caches the return value.

This reverts commit 7714687a2b2d ("thinkpad_acpi: Use
acpi_video_handles_brightness_key_presses()").

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f453d5d..0bed473 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3488,7 +3488,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	/* Do not issue duplicate brightness change events to
 	 * userspace. tpacpi_detect_brightness_capabilities() must have
 	 * been called before this point  */
-	if (acpi_video_handles_brightness_key_presses()) {
+	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
 		pr_info("This ThinkPad has standard ACPI backlight "
 			"brightness control, supported by the ACPI "
 			"video driver\n");
-- 
2.5.0

Re: [PATCH 4/4] Revert "thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()"

[Darren Hart]

On Thu, Jan 14, 2016 at 09:41:48AM +0100, Hans de Goede wrote:
> acpi_video_handles_brightness_key_presses()'s may return false if the i915
> driver is not loaded yet when thinkpad_acpi loads, and then return true
> after the i915 driver has loaded. This means that thinkpad_acpi cannot
> use it as is since thinkpad_acpi caches the return value.
> 
> This reverts commit 7714687a2b2d ("thinkpad_acpi: Use
> acpi_video_handles_brightness_key_presses()").
> 

Rafael, I presume this would go through your tree?

No objection from me. Henrique?

Acked-by: Darren Hart <dvhart@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index f453d5d..0bed473 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3488,7 +3488,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  	/* Do not issue duplicate brightness change events to
>  	 * userspace. tpacpi_detect_brightness_capabilities() must have
>  	 * been called before this point  */
> -	if (acpi_video_handles_brightness_key_presses()) {
> +	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
>  		pr_info("This ThinkPad has standard ACPI backlight "
>  			"brightness control, supported by the ACPI "
>  			"video driver\n");
> -- 
> 2.5.0
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 4/4] Revert "thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()"

[Henrique de Moraes Holschuh]

On Thu, Jan 14, 2016, at 21:18, Darren Hart wrote:
> On Thu, Jan 14, 2016 at 09:41:48AM +0100, Hans de Goede wrote:
> > acpi_video_handles_brightness_key_presses()'s may return false if the i915
> > driver is not loaded yet when thinkpad_acpi loads, and then return true
> > after the i915 driver has loaded. This means that thinkpad_acpi cannot
> > use it as is since thinkpad_acpi caches the return value.
> > 
> > This reverts commit 7714687a2b2d ("thinkpad_acpi: Use
> > acpi_video_handles_brightness_key_presses()").
> > 
> 
> Rafael, I presume this would go through your tree?
> 
> No objection from me. Henrique?

Acked-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>

> 
> Acked-by: Darren Hart <dvhart-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

-- 
  "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

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140

Re: [PATCH 4/4] Revert "thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()"

[Rafael J. Wysocki]

On Thursday, January 14, 2016 03:18:56 PM Darren Hart wrote:
> On Thu, Jan 14, 2016 at 09:41:48AM +0100, Hans de Goede wrote:
> > acpi_video_handles_brightness_key_presses()'s may return false if the i915
> > driver is not loaded yet when thinkpad_acpi loads, and then return true
> > after the i915 driver has loaded. This means that thinkpad_acpi cannot
> > use it as is since thinkpad_acpi caches the return value.
> > 
> > This reverts commit 7714687a2b2d ("thinkpad_acpi: Use
> > acpi_video_handles_brightness_key_presses()").
> > 
> 
> Rafael, I presume this would go through your tree?

Yes, I'll take care of this, thanks!

> No objection from me. Henrique?
> 
> Acked-by: Darren Hart <dvhart@linux.intel.com>
> 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/platform/x86/thinkpad_acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index f453d5d..0bed473 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -3488,7 +3488,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
> >  	/* Do not issue duplicate brightness change events to
> >  	 * userspace. tpacpi_detect_brightness_capabilities() must have
> >  	 * been called before this point  */
> > -	if (acpi_video_handles_brightness_key_presses()) {
> > +	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
> >  		pr_info("This ThinkPad has standard ACPI backlight "
> >  			"brightness control, supported by the ACPI "
> >  			"video driver\n");
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.