* Re: [PATCH 1/2] ACPI video: Harden video bus adding. [not found] <20120229035142.31016e6f@garik> @ 2012-02-29 0:12 ` Dmitry Torokhov 2012-02-29 0:44 ` Igor Murzov [not found] ` <20120229035518.306c57a8@garik> 1 sibling, 1 reply; 4+ messages in thread From: Dmitry Torokhov @ 2012-02-29 0:12 UTC (permalink / raw) To: Igor Murzov Cc: Zhang Rui, Len Brown, linux-acpi, linux-kernel, Igor Murzov, Sergey V. Hi Igor, On Wed, Feb 29, 2012 at 03:51:42AM +0400, Igor Murzov wrote: > From 127b9fc18ee646d92039c45d7fa3680a107928a2 Mon Sep 17 00:00:00 2001 > From: Igor Murzov <e-mail@date.by> > 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 <e-mail@date.by> > --- > drivers/acpi/video.c | 28 +++++++++++++++++++--------- > 1 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index eaef02a..1bc6370 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -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 }; > > > 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. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] ACPI video: Harden video bus adding. 2012-02-29 0:12 ` [PATCH 1/2] ACPI video: Harden video bus adding Dmitry Torokhov @ 2012-02-29 0:44 ` Igor Murzov 0 siblings, 0 replies; 4+ messages in thread From: Igor Murzov @ 2012-02-29 0:44 UTC (permalink / raw) To: Dmitry Torokhov Cc: Zhang Rui, Len Brown, linux-acpi, linux-kernel, Igor Murzov, Sergey V. On Tue, 28 Feb 2012 16:12:59 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Igor, > > On Wed, Feb 29, 2012 at 03:51:42AM +0400, Igor Murzov wrote: > > From 127b9fc18ee646d92039c45d7fa3680a107928a2 Mon Sep 17 00:00:00 2001 > > From: Igor Murzov <e-mail@date.by> > > 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 <e-mail@date.by> > > --- > > drivers/acpi/video.c | 28 +++++++++++++++++++--------- > > 1 files changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index eaef02a..1bc6370 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -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 }; > > > > > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20120229035518.306c57a8@garik>]
* Re: [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered [not found] ` <20120229035518.306c57a8@garik> @ 2012-02-29 0:39 ` Dmitry Torokhov 2012-02-29 1:17 ` Dmitry Torokhov 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Torokhov @ 2012-02-29 0:39 UTC (permalink / raw) To: Igor Murzov Cc: Zhang Rui, Len Brown, linux-acpi, linux-kernel, Igor Murzov, Sergey V. On Wed, Feb 29, 2012 at 03:55:18AM +0400, Igor Murzov wrote: > From 202f25d44b1cf871697c88a6a3a8d854674db4d8 Mon Sep 17 00:00:00 2001 > From: Igor Murzov <e-mail@date.by> > 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(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index 1bc6370..b9e114c 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -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) { > @@ -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. > 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: > -- > 1.7.5.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered 2012-02-29 0:39 ` [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered Dmitry Torokhov @ 2012-02-29 1:17 ` Dmitry Torokhov 0 siblings, 0 replies; 4+ messages in thread From: Dmitry Torokhov @ 2012-02-29 1:17 UTC (permalink / raw) To: Igor Murzov Cc: Zhang Rui, Len Brown, linux-acpi, linux-kernel, Igor Murzov, Sergey V. On Tue, Feb 28, 2012 at 04:39:15PM -0800, Dmitry Torokhov wrote: > On Wed, Feb 29, 2012 at 03:55:18AM +0400, Igor Murzov wrote: > > From 202f25d44b1cf871697c88a6a3a8d854674db4d8 Mon Sep 17 00:00:00 2001 > > From: Igor Murzov <e-mail@date.by> > > 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(-) > > > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index 1bc6370..b9e114c 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -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) { > > @@ -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. 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. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-29 1:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20120229035142.31016e6f@garik>
2012-02-29 0:12 ` [PATCH 1/2] ACPI video: Harden video bus adding Dmitry Torokhov
2012-02-29 0:44 ` Igor Murzov
[not found] ` <20120229035518.306c57a8@garik>
2012-02-29 0:39 ` [RFC][PATCH 2/2] ACPI video: Don't start video device until its associated input device has been reigstered Dmitry Torokhov
2012-02-29 1:17 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox