Re: [PATCH] pnpacpi: Better format error message

Re: [PATCH] pnpacpi: Better format error message

[Bjorn Helgaas]

On Tue, Mar 01, 2011 at 03:24:50PM +0100, Thomas Renninger wrote:
> On a system with empty _CRS method I see:
> pnp 00:0c: can't evaluate _CRS: 12311
> which is 0x3017, which would mean: AE_AML_INVALID_RESOURCE_TYPE
> 
> This patch would directly show: AE_AML_INVALID_RESOURCE_TYPE

The patch seems fine to me, but I question whether we need these
printks at all.  It seems pointless to have an empty _CRS method,
but it does seem legal, or at least debatable, so I'm not sure
that the message is telling us anything useful.

Bjorn

> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: bjorn@helgaas.com
> CC: lenb@kernel.org
> CC: robert.moore@intel.com
> 
> ---
> Corrected email address of Bjorn.
> 
>  drivers/pnp/pnpacpi/rsparser.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6.37-master/drivers/pnp/pnpacpi/rsparser.c
> ===================================================================
> --- linux-2.6.37-master.orig/drivers/pnp/pnpacpi/rsparser.c
> +++ linux-2.6.37-master/drivers/pnp/pnpacpi/rsparser.c
> @@ -498,7 +498,8 @@ int pnpacpi_parse_allocated_resource(str
>  
>  	if (ACPI_FAILURE(status)) {
>  		if (status != AE_NOT_FOUND)
> -			dev_err(&dev->dev, "can't evaluate _CRS: %d", status);
> +			dev_err(&dev->dev, "can't evaluate _CRS: %s",
> +				acpi_format_exception(status));
>  		return -EPERM;
>  	}
>  	return 0;
> @@ -809,7 +810,8 @@ int __init pnpacpi_parse_resource_option
>  
>  	if (ACPI_FAILURE(status)) {
>  		if (status != AE_NOT_FOUND)
> -			dev_err(&dev->dev, "can't evaluate _PRS: %d", status);
> +			dev_err(&dev->dev, "can't evaluate _PRS: %s",
> +				acpi_format_exception(status));
>  		return -EPERM;
>  	}
>  	return 0;
> @@ -876,7 +878,8 @@ int pnpacpi_build_resource_template(stru
>  	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>  				     pnpacpi_count_resources, &res_cnt);
>  	if (ACPI_FAILURE(status)) {
> -		dev_err(&dev->dev, "can't evaluate _CRS: %d\n", status);
> +		dev_err(&dev->dev, "can't evaluate _CRS: %s\n",
> +			acpi_format_exception(status));
>  		return -EINVAL;
>  	}
>  	if (!res_cnt)
> @@ -891,7 +894,8 @@ int pnpacpi_build_resource_template(stru
>  				     pnpacpi_type_resources, &resource);
>  	if (ACPI_FAILURE(status)) {
>  		kfree(buffer->pointer);
> -		dev_err(&dev->dev, "can't evaluate _CRS: %d\n", status);
> +		dev_err(&dev->dev, "can't evaluate _CRS: %s\n",
> +			acpi_format_exception(status));
>  		return -EINVAL;
>  	}
>  	/* resource will pointer the end resource now */

RE: [PATCH] pnpacpi: Better format error message

[Moore, Robert]

- there seem to be another problem in the acpica resource handling
depths if _CRS functions do not return anything.


Strictly speaking it's illegal for _CRS to not return anything. Practically, perhaps we should repair this case to a "NULL" resource descriptor which perhaps could be defined as a resource descriptor with a single End Tag descriptor.



From: Thomas Renninger [mailto:trenn@suse.de] 
Sent: Tuesday, March 01, 2011 8:44 AM
To: Bjorn Helgaas
Cc: lenb@kernel.org; Moore, Robert; linux-acpi@vger.kernel.org
Subject: Re: [PATCH] pnpacpi: Better format error message

On Tuesday, March 01, 2011 05:25:04 PM Bjorn Helgaas wrote:
> On Tue, Mar 01, 2011 at 03:24:50PM +0100, Thomas Renninger wrote:
> > On a system with empty _CRS method I see:
> > pnp 00:0c: can't evaluate _CRS: 12311
> > which is 0x3017, which would mean: AE_AML_INVALID_RESOURCE_TYPE
> > 
> > This patch would directly show: AE_AML_INVALID_RESOURCE_TYPE
> 
> The patch seems fine to me, but I question whether we need these
> printks at all. It seems pointless to have an empty _CRS method,
> but it does seem legal, or at least debatable, so I'm not sure
> that the message is telling us anything useful.
Theoretically the callback function passed to:
acpi_walk_resources(handle, METHOD_NAME__CRS
should get called with an empty resource string or not at all
in case _CRS can return nothing.
But in this case acpi_walk_resources fails which is worth
reporting. I could imagine other _CRS functions are not
processed anymore when acpi_walk_resources bails out with
AE_AML_INVALID_RESOURCE_TYPE on a specific _CRS function, don't know.

Summary:
- acpi_walk_resources failures should be reported (and this patch
is to do that more nicely)
- there seem to be another problem in the acpica resource handling
depths if _CRS functions do not return anything.

Thomas


> 
> Bjorn
> 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: bjorn@helgaas.com
> > CC: lenb@kernel.org
> > CC: robert.moore@intel.com
> > 
> > ---
> > Corrected email address of Bjorn.
> > 
> > drivers/pnp/pnpacpi/rsparser.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > Index: linux-2.6.37-master/drivers/pnp/pnpacpi/rsparser.c
> > ===================================================================
> > --- linux-2.6.37-master.orig/drivers/pnp/pnpacpi/rsparser.c
> > +++ linux-2.6.37-master/drivers/pnp/pnpacpi/rsparser.c
> > @@ -498,7 +498,8 @@ int pnpacpi_parse_allocated_resource(str
> > 
> > if (ACPI_FAILURE(status)) {
> > if (status != AE_NOT_FOUND)
> > - dev_err(&dev->dev, "can't evaluate _CRS: %d", status);
> > + dev_err(&dev->dev, "can't evaluate _CRS: %s",
> > + acpi_format_exception(status));
> > return -EPERM;
> > }
> > return 0;
> > @@ -809,7 +810,8 @@ int __init pnpacpi_parse_resource_option
> > 
> > if (ACPI_FAILURE(status)) {
> > if (status != AE_NOT_FOUND)
> > - dev_err(&dev->dev, "can't evaluate _PRS: %d", status);
> > + dev_err(&dev->dev, "can't evaluate _PRS: %s",
> > + acpi_format_exception(status));
> > return -EPERM;
> > }
> > return 0;
> > @@ -876,7 +878,8 @@ int pnpacpi_build_resource_template(stru
> > status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> > pnpacpi_count_resources, &res_cnt);
> > if (ACPI_FAILURE(status)) {
> > - dev_err(&dev->dev, "can't evaluate _CRS: %d\n", status);
> > + dev_err(&dev->dev, "can't evaluate _CRS: %s\n",
> > + acpi_format_exception(status));
> > return -EINVAL;
> > }
> > if (!res_cnt)
> > @@ -891,7 +894,8 @@ int pnpacpi_build_resource_template(stru
> > pnpacpi_type_resources, &resource);
> > if (ACPI_FAILURE(status)) {
> > kfree(buffer->pointer);
> > - dev_err(&dev->dev, "can't evaluate _CRS: %d\n", status);
> > + dev_err(&dev->dev, "can't evaluate _CRS: %s\n",
> > + acpi_format_exception(status));
> > return -EINVAL;
> > }
> > /* resource will pointer the end resource now */
> 

Re: [PATCH] pnpacpi: Better format error message

[Thomas Renninger]

On Tuesday 01 March 2011 18:03:23 Moore, Robert wrote:
> - there seem to be another problem in the acpica resource handling
> depths if _CRS functions do not return anything.
> 
> 
> Strictly speaking it's illegal for _CRS to not return anything.
> Practically, perhaps we should repair this case to a "NULL" resource
> descriptor which perhaps could be defined as a resource descriptor
> with a single End Tag descriptor.
Be aware that this was an early BIOS.
I am fine if there is an error message if this is not allowed.
It's the first time I saw such a message and an empty resource func.


    Thomas

RE: [PATCH] pnpacpi: Better format error message

[Moore, Robert]

> -----Original Message-----
> From: Thomas Renninger [mailto:trenn@suse.de]
> Sent: Tuesday, March 01, 2011 1:08 PM
> To: Moore, Robert
> Cc: Bjorn Helgaas; lenb@kernel.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH] pnpacpi: Better format error message
> 
> On Tuesday 01 March 2011 18:03:23 Moore, Robert wrote:
> > - there seem to be another problem in the acpica resource handling
> > depths if _CRS functions do not return anything.
> >
> >
> > Strictly speaking it's illegal for _CRS to not return anything.
> > Practically, perhaps we should repair this case to a "NULL" resource
> > descriptor which perhaps could be defined as a resource descriptor
> > with a single End Tag descriptor.
> Be aware that this was an early BIOS.
> I am fine if there is an error message if this is not allowed.
> It's the first time I saw such a message and an empty resource func.
> 
>     Thomas

The goal of the repair code is of course to simplify the drivers. What we'd like to be able to do is guarantee that if an AE_OK is returned by the evaluation of a predefined ACPI name, the returned object is of the correct type. It seems to me that a "resource descriptor" is a special type of buffer, so we can add support for that in the repair code. 

In the _CRS case above, we could return a resource descriptor with simply an EndTag -- the driver can of course squawk about this if it desires.

On the other hand, we could just punt on the conversion and just return an OBJECT_TYPE error.