Re: [PATCH 1/4] ACPI: Add acpi_pr_<level>() interfaces

[Toshi Kani <toshi.kani@hp.com>]

On Thu, 2012-07-19 at 10:15 -0600, Shuah Khan wrote:
> On Wed, 2012-07-18 at 18:38 -0600, Toshi Kani wrote:
> 
> > 
> > This interface is defined in acpi/acpi_bus.h, which is intended for ACPI
> > drivers which make many ACPI calls to proceed when they are called at
> > run-time today.  This interface does not change that, and I believe
> > acpi_get_name() is much faster compared to ACPI method calls these ACPI
> > drivers make in their normal code path.  The extra work to call
> > acpi_get_name() is simply a noise in this case (if you try to measure),
> > and the use of this interface is limited in error paths of such ACPI
> > drivers.
> 
> I understand the scope of the usage of this new interface. I don't think
> I am able to explain the problem I see with this interface as it gets
> used more and more from acpi drivers. Let me try another way.
> 
> If understand the this patch set, if and when acpi drivers that
> currently use pr_* interfaces switch to using acpi_pr_*, the execution
> path goes from a what printk() does to the following:
> 
> acpi_pr_*
> - setup static buffer
> - calls acpi_get_name()
> - acpi_get_name() calls acpi_ut_validate_buffer() and then calls
>   acpi_ns_handle_to_pathname()
> - acpi_ns_handle_to_pathname() calls acpi_ns_validate_handle() followed
>   by acpi_ns_get_pathname_length() and so on.
> 
> I think this should give you a good idea of my concern. I think
> acpi_pr_* full functionality should be enabled under special cases such
> as some acpi_debug level setting or some other way, and not for default
> case. I propose the following:
> 
> Make acpi_pr_* versions execute the full path to do acpi_get_name()
> conditionally and not as a default case.
> 
> To illustrate my point further, I currently see the following ACPI
> messages in my dmesg buffer on my laptop. I haven't taken the time to
> evaluate how many of them originate from acpi drivers, however I would
> not want to see all of these becoming acpi_pr_* versions that do more
> than what pr_* does today. I hope this explains my concern clearly.

I agree that there are many ACPI messages at boot, and I understand that
you concerned with them, but that is a different issue.  It requires a
different project to address them.  Changing my patchset won't make any
difference.

The issue I am trying to solve is well summarized in Bjorn's comment in
my earlier patch as follows: 
=== <quote> ===
>                result = acpi_processor_device_add(handle, &device);
>                if (result) {
>                        printk(KERN_ERR PREFIX "Unable to add the
device\n");

You didn't add this problem, but since you're touching the code, I'll
point it out.  This message will look to the user like:

    ACPI: Unable to add the device

which is useless.  The user has no idea what device we're talking
about or why we can't add it, so he can't *do* anything with the
message.
=== </quote> ===

This is very true.  And this issue is even worse when support teams need
to solve such customer issue from log files.  The current error messages
in ACPI hotplug handlers do not provide any information to identify a
device that cause an issue.

To address this issue, the ACPI drivers have to obtain an ACPI object
path information in their error handling code with acpi_get_name().

A possible alternative to acpi_pr_<level>() is to create a driver's
local printk function, just like acpi_print_osc_error().  However, I do
not think we should create such local functions in all ACPI drivers.

Performance is not an issue since the use of the interfaces is limited
to the error paths.  The boot messages you concerned with do not need to
any more extra-info to append ACPI device path, and therefore no need to
use acpi_pr_<level>().  dev_<level>() may be a good example of how this
type of interfaces is used in error paths today.


Thanks,
-Toshi