Patchwork Reduce ACPI resource conflict message to KERN_INFO, printf cleanup

login
register
mail settings
Submitter Chase Douglas
Date March 9, 2010, 2:41 p.m.
Message ID <1268145670-10094-2-git-send-email-chase.douglas@canonical.com>
Download mbox | patch
Permalink /patch/47164/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

Chase Douglas - March 9, 2010, 2:41 p.m.
By default, ACPI resource conflict messages are logged at level
KERN_ERR. This is a rather high level for a message that is more a
warning than an indication of a real kernel error. Also, KERN_ERR level
messages can appear over some boot splash screens, and this message is
not serious enough to warrant such treatment. Thus, the log level has
been reduced to KERN_INFO.

Also, cleanup message to use %pR resource printing format.

BugLink: http://bugs.launchpad.net/bugs/440470

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/acpi/osl.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)
Amit Kucheria - March 9, 2010, 5:29 p.m.
On 10 Mar 09, Chase Douglas wrote:
> By default, ACPI resource conflict messages are logged at level
> KERN_ERR. This is a rather high level for a message that is more a
> warning than an indication of a real kernel error. Also, KERN_ERR level
> messages can appear over some boot splash screens, and this message is
> not serious enough to warrant such treatment. Thus, the log level has
> been reduced to KERN_INFO.
> 
> Also, cleanup message to use %pR resource printing format.

This was rather interesting. So %pR replaces the entire [0x%llx-0x%llx] mess?
Cool.

> BugLink: http://bugs.launchpad.net/bugs/440470
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/acpi/osl.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 7c1c59e..f89f141 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1151,16 +1151,10 @@ int acpi_check_resource_conflict(struct resource *res)
>  
>  	if (clash) {
>  		if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) {
> -			printk("%sACPI: %s resource %s [0x%llx-0x%llx]"
> -			       " conflicts with ACPI region %s"
> -			       " [0x%llx-0x%llx]\n",
> -			       acpi_enforce_resources == ENFORCE_RESOURCES_LAX
> -			       ? KERN_WARNING : KERN_ERR,
> -			       ioport ? "I/O" : "Memory", res->name,
> -			       (long long) res->start, (long long) res->end,
> -			       res_list_elem->name,
> -			       (long long) res_list_elem->start,
> -			       (long long) res_list_elem->end);
> +			printk(KERN_INFO "ACPI: resource %s %pR"
> +			       " conflicts with ACPI region %s %pR\n",
> +			       res->name, res, res_list_elem->name,
> +			       res_list_elem);
>  			if (acpi_enforce_resources == ENFORCE_RESOURCES_LAX)
>  				printk(KERN_NOTICE "ACPI: This conflict may"
>  				       " cause random problems and system"
> -- 
> 1.6.3.3
> 

The very next line is:
		if (acpi_enforce_resources == ENFORCE_RESOURCES_STRICT)
					return -EBUSY;

And, acpi_enforce_resources is initialised to ENFORCE_RESOURCES_STRICT by
default. So upstream considers this to be desirable behaviour. Should we
really change the seriousness of the message in that case?

Regards,
Amit
Chase Douglas - March 9, 2010, 5:46 p.m.
On Tue, Mar 9, 2010 at 12:29 PM, Amit Kucheria
<amit.kucheria@canonical.com> wrote:
> On 10 Mar 09, Chase Douglas wrote:
>> Also, cleanup message to use %pR resource printing format.
>
> This was rather interesting. So %pR replaces the entire [0x%llx-0x%llx] mess?
> Cool.

It also prints out whether the resource was a memory region or io port
(I was also unaware until it was pointed out to me on linux-acpi).
There's some documentation on this and other extensions at [1].

> The very next line is:
>                if (acpi_enforce_resources == ENFORCE_RESOURCES_STRICT)
>                                        return -EBUSY;
>
> And, acpi_enforce_resources is initialised to ENFORCE_RESOURCES_STRICT by
> default. So upstream considers this to be desirable behaviour. Should we
> really change the seriousness of the message in that case?

First, Matthew Garret upstream has stated that he believes KERN_INFO
to be reasonable.

For a long time, the native hwmon drivers were accessing resources
that were owned by the ACPI driver as well. This hasn't caused any
issues yet because there weren't any ACPI drivers that accessed the
regions. The preferred method would be to have ACPI drivers for all
hwmon chips. Now, there's one ACPI driver for some ASUS motherboards.
In 2.6.31 they decided to default to strictly locking out native hwmon
drivers whenever they conflict with ACPI resources. Thus, this code
prevents the hwmon from working and prints out a message at KERN_ERR
level.

In reality, most people don't care about hwmon drivers. However, they
see this ugly error message at startup because KERN_ERR messages are
overlayed on top of the boot splash (plymouth?). This is one of those
"death by a thousand paper cuts" issues. If a user really wants hwmon
drivers, they likely searched out how to do it and will see error
messages in their dmesg and logs. It's also a well known issue and is
listed in the lmsensors FAQ, along with the work around boot
parameter.

So basically it's just ugly and unnecessary for the normal user. And
the "real" fix is to get ACPI drivers into the kernel in place of
native hwmon drivers. We don't need a warning message over our boot
splash just because there's a missing ACPI hwmon driver.

[1] http://lxr.linux.no/linux+v2.6.33/lib/vsprintf.c#L1171
Andy Whitcroft - March 31, 2010, 10:43 a.m.
On Tue, Mar 09, 2010 at 09:41:10AM -0500, Chase Douglas wrote:
> By default, ACPI resource conflict messages are logged at level
> KERN_ERR. This is a rather high level for a message that is more a
> warning than an indication of a real kernel error. Also, KERN_ERR level
> messages can appear over some boot splash screens, and this message is
> not serious enough to warrant such treatment. Thus, the log level has
> been reduced to KERN_INFO.
> 
> Also, cleanup message to use %pR resource printing format.
> 
> BugLink: http://bugs.launchpad.net/bugs/440470
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/acpi/osl.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 7c1c59e..f89f141 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1151,16 +1151,10 @@ int acpi_check_resource_conflict(struct resource *res)
>  
>  	if (clash) {
>  		if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) {
> -			printk("%sACPI: %s resource %s [0x%llx-0x%llx]"
> -			       " conflicts with ACPI region %s"
> -			       " [0x%llx-0x%llx]\n",
> -			       acpi_enforce_resources == ENFORCE_RESOURCES_LAX
> -			       ? KERN_WARNING : KERN_ERR,
> -			       ioport ? "I/O" : "Memory", res->name,
> -			       (long long) res->start, (long long) res->end,
> -			       res_list_elem->name,
> -			       (long long) res_list_elem->start,
> -			       (long long) res_list_elem->end);
> +			printk(KERN_INFO "ACPI: resource %s %pR"
> +			       " conflicts with ACPI region %s %pR\n",
> +			       res->name, res, res_list_elem->name,
> +			       res_list_elem);
>  			if (acpi_enforce_resources == ENFORCE_RESOURCES_LAX)
>  				printk(KERN_NOTICE "ACPI: This conflict may"
>  				       " cause random problems and system"

Sounds like upstream concurs on the level and this simplifies things
generally.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Patch

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 7c1c59e..f89f141 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1151,16 +1151,10 @@  int acpi_check_resource_conflict(struct resource *res)
 
 	if (clash) {
 		if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) {
-			printk("%sACPI: %s resource %s [0x%llx-0x%llx]"
-			       " conflicts with ACPI region %s"
-			       " [0x%llx-0x%llx]\n",
-			       acpi_enforce_resources == ENFORCE_RESOURCES_LAX
-			       ? KERN_WARNING : KERN_ERR,
-			       ioport ? "I/O" : "Memory", res->name,
-			       (long long) res->start, (long long) res->end,
-			       res_list_elem->name,
-			       (long long) res_list_elem->start,
-			       (long long) res_list_elem->end);
+			printk(KERN_INFO "ACPI: resource %s %pR"
+			       " conflicts with ACPI region %s %pR\n",
+			       res->name, res, res_list_elem->name,
+			       res_list_elem);
 			if (acpi_enforce_resources == ENFORCE_RESOURCES_LAX)
 				printk(KERN_NOTICE "ACPI: This conflict may"
 				       " cause random problems and system"