[PATCH] ACPI video: Ignore AE_AML_PACKAGE_LIMIT errors after _DOD evaluation.

[PATCH] ACPI video: Ignore AE_AML_PACKAGE_LIMIT errors after _DOD evaluation.

[Igor Murzov]

This should fix brightness controls on some laptops.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861

Signed-off-by: Igor Murzov <e-mail@date.by>
---
 drivers/acpi/video.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..01bb58d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
 	struct acpi_device *dev;
 
 	status = acpi_video_device_enumerate(video);
-	if (status)
-		return status;
+	if (status) {
+		if (status == AE_AML_PACKAGE_LIMIT)
+			status = 0; /* Ignore this error */
+		else
+			return status;
+	}
 
 	list_for_each_entry(dev, &device->children, node) {
 
-- 
1.7.12

Re: [PATCH] ACPI video: Ignore AE_AML_PACKAGE_LIMIT errors after _DOD evaluation.

[Rafael J. Wysocki]

On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote:
> This should fix brightness controls on some laptops.
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861
> 
> Signed-off-by: Igor Murzov <e-mail@date.by>

Put more detials into the changelog, please.  The BZ entry linked above
may or may not be accessible when the changelog is read by someone.

> ---
>  drivers/acpi/video.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 1e0a9e1..01bb58d 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
>  	struct acpi_device *dev;
>  
>  	status = acpi_video_device_enumerate(video);
> -	if (status)
> -		return status;
> +	if (status) {
> +		if (status == AE_AML_PACKAGE_LIMIT)
> +			status = 0; /* Ignore this error */
> +		else
> +			return status;
> +	}

First off, please add a comment explaining _why_ we're ignoring the error.

Second, what about adding just:

if (status == AE_AML_PACKAGE_LIMIT)
	status = 0;

before the "if (status)" check?

>  
>  	list_for_each_entry(dev, &device->children, node) {

Thanks,
Rafael


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

Re: [PATCH] ACPI video: Ignore AE_AML_PACKAGE_LIMIT errors after _DOD evaluation.

[Igor Murzov]

On Wed, 10 Oct 2012 00:58:10 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote:
> > This should fix brightness controls on some laptops.
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861
> > 
> > Signed-off-by: Igor Murzov <e-mail@date.by>
> 
> Put more detials into the changelog, please.  The BZ entry linked above
> may or may not be accessible when the changelog is read by someone.
> 
> > ---
> >  drivers/acpi/video.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 1e0a9e1..01bb58d 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
> >  	struct acpi_device *dev;
> >  
> >  	status = acpi_video_device_enumerate(video);
> > -	if (status)
> > -		return status;
> > +	if (status) {
> > +		if (status == AE_AML_PACKAGE_LIMIT)
> > +			status = 0; /* Ignore this error */
> > +		else
> > +			return status;
> > +	}
> 
> First off, please add a comment explaining _why_ we're ignoring the error.

The problem is that i'm not sure if it's ok to ignore
AE_AML_PACKAGE_LIMIT here. Stefan Wilkens in bugzilla
claims that video module works fine on his Compaq 6720s
in spite of the fact that acpi_video_device_enumerate()
is not able to find any video device on his laptop.
This fix is just the most obvious one, not necessarily
the proper one.


> Second, what about adding just:
> 
> if (status == AE_AML_PACKAGE_LIMIT)
> 	status = 0;
> 
> before the "if (status)" check?

Ok

-- Igor

> >  
> >  	list_for_each_entry(dev, &device->children, node) {
> 
> Thanks,
> Rafael

Re: [PATCH] ACPI video: Ignore AE_AML_PACKAGE_LIMIT errors after _DOD evaluation.

[Rafael J. Wysocki]

On Wednesday 10 of October 2012 04:43:40 Igor Murzov wrote:
> On Wed, 10 Oct 2012 00:58:10 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote:
> > > This should fix brightness controls on some laptops.
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861
> > > 
> > > Signed-off-by: Igor Murzov <e-mail@date.by>
> > 
> > Put more detials into the changelog, please.  The BZ entry linked above
> > may or may not be accessible when the changelog is read by someone.
> > 
> > > ---
> > >  drivers/acpi/video.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > > index 1e0a9e1..01bb58d 100644
> > > --- a/drivers/acpi/video.c
> > > +++ b/drivers/acpi/video.c
> > > @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
> > >  	struct acpi_device *dev;
> > >  
> > >  	status = acpi_video_device_enumerate(video);
> > > -	if (status)
> > > -		return status;
> > > +	if (status) {
> > > +		if (status == AE_AML_PACKAGE_LIMIT)
> > > +			status = 0; /* Ignore this error */
> > > +		else
> > > +			return status;
> > > +	}
> > 
> > First off, please add a comment explaining _why_ we're ignoring the error.
> 
> The problem is that i'm not sure if it's ok to ignore
> AE_AML_PACKAGE_LIMIT here.

This should be written in a comment, then. :-)

> Stefan Wilkens in bugzilla
> claims that video module works fine on his Compaq 6720s
> in spite of the fact that acpi_video_device_enumerate()
> is not able to find any video device on his laptop.
> This fix is just the most obvious one, not necessarily
> the proper one.

So say something like "there are systems known to return AE_AML_PACKAGE_LIMIT
here, apparently for no valid reason" in a comment.  And give a reference to
the BZ entry in the changelog.

There are too many places where we have put workarounds like this without
documenting them properly and now we have problems when we need to change
something.

Thanks,
Rafael


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