From patchwork Thu Jul 26 21:37:30 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 1246351 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id B6ADBDFFCE for ; Thu, 26 Jul 2012 21:45:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753048Ab2GZVpj (ORCPT ); Thu, 26 Jul 2012 17:45:39 -0400 Received: from mail-ey0-f202.google.com ([209.85.215.202]:60095 "EHLO mail-ey0-f202.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752913Ab2GZVpf (ORCPT ); Thu, 26 Jul 2012 17:45:35 -0400 Received: by eaao12 with SMTP id o12so54120eaa.1 for ; Thu, 26 Jul 2012 14:45:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=d7c0P0ILExTuRb0p/qPQmWFgtdrekpmnyohVa9XIeQM=; b=NkfOzLFOXiZy3PyikTZ1IeTLQxCcencbWRCZKGx9hDKxqo6HhR23YcYBBtqOrI5k0s HjkZxV5R2viGRSb5WITDj52hKr4zqPMWf69JudCzkqBRPsndHmcq9F+ZidLyOTzlwOFR mPld5Fxz/A+3tDWilHUgQ8VMRb/nM4C/lp8A4aPOOJFPgz/hHzW1haFHaojP8sBF460I MfZ7thAwdp6xCW9jgH3GVwttxvICy71rdhBpaMsv+X2sRDL9V6T6+4jvYJdx8l98uY4N TnoFCnvXf9TZUYm6RuOD2qRKKjZ/NMsVYqDp2PVG+5ayKSRUuXgN3GjYkM0Y8rlvDAqZ 02lw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :x-gm-message-state; bh=d7c0P0ILExTuRb0p/qPQmWFgtdrekpmnyohVa9XIeQM=; b=F1Z9+CxIhmSLnxIKOJBL+AOaWEhaAMkNuHCSkXhFjE0h0bXq5pAa8OxZ/Fz8CHyJ2y 58Cw027D3pS/K0BgnXBiSTze+3n32bv0BQ9AWKqt70Ms09oQizCzTWzUzLIzMd+FMDef klfG+EW1p5ZkQrc6wrh7kx421CK/ly3p52mbppPf1rpzgWngLkvXZwyaYFtoe1ju776v dATHg7bXFLYWUPQfNlwCNBQn+5DyB+V5W60EfieKRBX+Zhyun9h0tt2dkmNzdAvj8grW J3Si4UWMbgMOttmWMfH2h/JJMFDKBka5+oHEaAdgLmCbAk+t+S7OyCZQPKtyYQ7NSI0F hY1A== Received: by 10.14.211.196 with SMTP id w44mr163878eeo.0.1343338651894; Thu, 26 Jul 2012 14:37:31 -0700 (PDT) Received: by 10.14.211.196 with SMTP id w44mr163856eeo.0.1343338651786; Thu, 26 Jul 2012 14:37:31 -0700 (PDT) Received: from hpza10.eem.corp.google.com ([74.125.121.33]) by gmr-mx.google.com with ESMTPS id v42si490846eep.0.2012.07.26.14.37.31 (version=TLSv1/SSLv3 cipher=AES128-SHA); Thu, 26 Jul 2012 14:37:31 -0700 (PDT) Received: from bhelgaas.mtv.corp.google.com (bhelgaas.mtv.corp.google.com [172.18.96.155]) by hpza10.eem.corp.google.com (Postfix) with ESMTP id 6F61620004E; Thu, 26 Jul 2012 14:37:31 -0700 (PDT) Received: by bhelgaas.mtv.corp.google.com (Postfix, from userid 131485) id B9BB918019D; Thu, 26 Jul 2012 14:37:30 -0700 (PDT) Date: Thu, 26 Jul 2012 15:37:30 -0600 From: Bjorn Helgaas To: Toshi Kani 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 Subject: Re: [PATCH v3 1/4] ACPI: Add acpi_pr_() interfaces Message-ID: <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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1343336330.3010.496.camel@misato.fc.hp.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-Gm-Message-State: ALoCoQle0ADUN6qwttJt24wS2bHuyzrKXb5Dsr+13lrYkPOzPKKDj/wcMP+dWvwZgTWgS3n1M9DPWMSBvobAGgWAeRPNWWUSFq3x44OnovYGhGS+cUGHfDkJFK0UMrJ5twFX+YueeDZSegtRSFQC0cm24j/IzXcmQupI7GRxYOISyXnu92Y7DRXy90KHWeAD4oTwpZgtyzGo Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org 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. > > 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: --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);