From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752707Ab2GZVzF (ORCPT ); Thu, 26 Jul 2012 17:55:05 -0400 Received: from g4t0016.houston.hp.com ([15.201.24.19]:46298 "EHLO g4t0016.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752032Ab2GZVzD (ORCPT ); Thu, 26 Jul 2012 17:55:03 -0400 Message-ID: <1343339411.3010.507.camel@misato.fc.hp.com> Subject: Re: [PATCH v3 1/4] ACPI: Add acpi_pr_() interfaces From: Toshi Kani To: Bjorn Helgaas Cc: lenb@kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, joe@perches.com, isimatu.yasuaki@jp.fujitsu.com, liuj97@gmail.com, srivatsa.bhat@linux.vnet.ibm.com, prarit@redhat.com, imammedo@redhat.com, vijaymohan.pandarathil@hp.com Date: Thu, 26 Jul 2012 15:50:11 -0600 In-Reply-To: <20120726213730.GA2149@google.com> References: <1343257978-7085-1-git-send-email-toshi.kani@hp.com> <1343257978-7085-2-git-send-email-toshi.kani@hp.com> <1343336330.3010.496.camel@misato.fc.hp.com> <20120726213730.GA2149@google.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3 (3.2.3-1.fc16) Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote: > On Thu, Jul 26, 2012 at 02:58:50PM -0600, Toshi Kani wrote: > > On Thu, 2012-07-26 at 13:22 -0600, Bjorn Helgaas wrote: > > > On Wed, Jul 25, 2012 at 5:12 PM, Toshi Kani wrote: > > > > This patch introduces acpi_pr_(), where is a kernel > > > > message level such as err/warn/info, to support improved logging > > > > messages for ACPI, esp. in hotplug operations. acpi_pr_() > > > > appends "ACPI" prefix and ACPI object path to the messages. This > > > > improves diagnostics in hotplug operations since it identifies an > > > > object that caused an issue in a log file. > > > > > > > > acpi_pr_() takes acpi_handle as an argument, which is passed > > > > to ACPI hotplug notify handlers from the ACPICA. Therefore, it is > > > > always available unlike other kernel objects, such as device. > > > > > > > > For example, the statement below > > > > acpi_pr_err(handle, "Device don't exist, dropping EJECT\n"); > > > > logs an error message like this at KERN_ERR. > > > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > > > > > ACPI drivers can use acpi_pr_() when they need to identify > > > > a target ACPI object in their messages, such as error messages. > > > > > > It's definitely an improvement to have *something* that identifies a > > > device in these messages. But the ACPI namespace path is not really > > > intended to be user-consumable, so I don't think we should expose it > > > indiscriminately. I think we should be using the ACPI device name > > > ("PNP0C02:00") whenever possible. Given the device name, we can get > > > the path from the sysfs "path" file. > > > > Hi Bjorn, > > > > Thanks for reviewing! Yes, ACPI device path is not good for regular > > users to analyze from the info. I also agree with you that device name > > is a better choice when users always diagnose issues by themselves right > > after they performed an operation. However, there are also cases that > > users ask someone to diagnose an issue remotely from the log files, and > > hotplug operations are performed automatically. In such cases, using > > ACPI device name alone is problematic for hotplug operations since a > > device name comes with an instance number that continues to change with > > hot-add/remove operations. Here is one example scenario. Let's say, > > user has automatic load-balancer or power-saving that can trigger > > hundreds of CPU hotplug operations per a day. This user then found that > > there were multiple hotplug errors logged in the past few days, and > > asked an IT guy to look at the error messages. When this user found the > > issue, all device names are gone from sysfs after repeated hotplug > > operations. This IT guy would have no idea if those errors were > > happening on a particular device or not from the error messages since > > their instance numbers continue to change. > > I agree that it's useful to be able to debug from the dmesg log > without having to ask a user to collect stuff from /sys. But rather > than including the namespace path in every message, I think it'd be > better to do one dev_info() in the hotplug notify event handler and > include the path there. Subsequent messages can just use dev_info() > without the namespace info. Hi Bjorn, I agree. For now, I will keep the use of acpi_pr_() within the hotplug notify handler functions. I will change any subsequent functions to use dev_(). This way, we have only one use of acpi_pr_() in an operation. I will also clarify this in the change log. > > > Another possible approach to this is to add a %p extension rather than > > > adding acpi_printk(). Then you could do, e.g., 'printk("%pA ...\n", > > > handle)', and printk could interpolate the namespace path. But I > > > really think there should be very few places where we need the path, > > > so I'm not sure it's worth it. > > > > Address of handle is not very helpful either when someone needs to > > analyze from log files. > > Sorry, I should have made this clearer. The %pA would expand to the ACPI > namespace path, so a "dev_info(dev, "new device for %pA\n", dev->handle)" > would produce output like this: > > PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO > > I fiddled with this a while ago; it would look something like this: I see. That sounds good idea. I agree that we can use it when device is valid. Since it's late to make such changes, I will consider making such changes for 3.7. Thanks, -Toshi > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index c3f36d41..201dcdb 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -551,6 +551,29 @@ char *symbol_string(char *buf, char *end, void *ptr, > #endif > } > > +#ifdef CONFIG_ACPI > +#include > + > +static noinline_for_stack > +char *acpi_name_string(char *buf, char *end, acpi_handle handle, > + struct printf_spec spec, const char *fmt) > +{ > + acpi_status status; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + u32 type = ACPI_SINGLE_NAME; > + char *p = buf; > + > + if (fmt[0] == 'A') > + type = ACPI_FULL_PATHNAME; > + > + status = acpi_get_name(handle, type, &buffer); > + if (ACPI_SUCCESS(status)) > + p = string(buf, end, buffer.pointer, spec); > + kfree(buffer.pointer); > + return p; > +} > +#endif > + > static noinline_for_stack > char *resource_string(char *buf, char *end, struct resource *res, > struct printf_spec spec, const char *fmt) > @@ -921,6 +944,8 @@ int kptr_restrict __read_mostly; > * > * Right now we handle: > * > + * - 'A' For full ACPI namespace names > + * - 'a' For single segment ACPI namespace names > * - 'F' For symbolic function descriptor pointers with offset > * - 'f' For simple symbolic function names without offset > * - 'S' For symbolic direct pointers with offset > @@ -982,6 +1007,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > } > > switch (*fmt) { > + case 'A': > + case 'a': > + return acpi_name_string(buf, end, ptr, spec, fmt); > case 'F': > case 'f': > ptr = dereference_function_descriptor(ptr);