| Message ID | 1342644027-19559-2-git-send-email-toshi.kani@hp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > This patch introduces acpi_pr_<level>(), where <level> is a message > level such as err/warn/info, to support improved logging messages > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > to ACPI hotplug notify handlers from the ACPI CA. Therefore, it is > always available unlike other kernel objects, such as device. Hi Toshi. I'd be tempted to instead make the calls more like other <subsystem>_<level> uses and rename these to acpi_<level> and change the existing acpi_info to another name. Other than that, seems fine to me. cheers, Joe -- 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
On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > This patch introduces acpi_pr_<level>(), where <level> is a message > level such as err/warn/info, to support improved logging messages > for ACPI, esp. in hotplug operations. One more note: > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c [] > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, [] > +void > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) [] > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; [] > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); [] > + kfree(buffer.pointer); There's a failure mode here because buffer.pointer isn't guaranteed to be initialized or set to NULL. -- 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
On Wed, 2012-07-18 at 14:21 -0700, Joe Perches wrote: > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > This patch introduces acpi_pr_<level>(), where <level> is a message > > level such as err/warn/info, to support improved logging messages > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > to ACPI hotplug notify handlers from the ACPI CA. Therefore, it is > > always available unlike other kernel objects, such as device. > > Hi Toshi. > > I'd be tempted to instead make the calls more like > other <subsystem>_<level> uses and rename these to > acpi_<level> and change the existing acpi_info to > another name. Hi Joe, I agree with you. Unfortunately, the ACPI CA (ACPI FW interpreter) already uses them for its internal-use as follows, so I needed to come up with some other name... Hence, acpi_pr_<level>. /* * Error reporting. Callers module and line number are inserted by AE_INFO, * the plist contains a set of parens to allow variable-length lists. * These macros are used for both the debug and non-debug versions of the code. */ #define ACPI_INFO(plist) acpi_info plist #define ACPI_WARNING(plist) acpi_warning plist #define ACPI_EXCEPTION(plist) acpi_exception plist #define ACPI_ERROR(plist) acpi_error plist #define ACPI_DEBUG_OBJECT(obj,l,i) acpi_ex_do_debug_object(obj,l,i) > Other than that, seems fine to me. Great! Can I consider it as Ack? :) Thanks, -Toshi > cheers, Joe > -- 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
On Wed, 2012-07-18 at 14:27 -0700, Joe Perches wrote: > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > This patch introduces acpi_pr_<level>(), where <level> is a message > > level such as err/warn/info, to support improved logging messages > > for ACPI, esp. in hotplug operations. > > One more note: > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > [] > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > [] > > +void > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > [] > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > [] > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > [] > > + kfree(buffer.pointer); > > There's a failure mode here because buffer.pointer > isn't guaranteed to be initialized or set to NULL. Hi Joe, Yes, I thought that check was necessary as well. However, to my surprise, such check caused the following warning message from patchcheck.pl. So, I deleted the check... # check for needless kfree() checks if ($prevline =~ /\bif\s*\(([^\)]*)\)/) { my $expr = $1; if ($line =~ /\bkfree\(\Q$expr\E\);/) { WARN("NEEDLESS_KFREE", "kfree(NULL) is safe this check is probably not required\n" . $hereprev); } } Thanks, -Toshi -- 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
On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: > On Wed, 2012-07-18 at 14:21 -0700, Joe Perches wrote: > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > level such as err/warn/info, to support improved logging messages > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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. [] > > I'd be tempted to instead make the calls more like > > other <subsystem>_<level> uses and rename these to > > acpi_<level> and change the existing acpi_info to > > another name. [] > I agree with you. Unfortunately, the ACPI CA (ACPI FW interpreter) > already uses them for its internal-use as follows, so I needed to come > up with some other name... Hence, acpi_pr_<level>. > > /* > * Error reporting. Callers module and line number are inserted by AE_INFO, > * the plist contains a set of parens to allow variable-length lists. > * These macros are used for both the debug and non-debug versions of the code. > */ > #define ACPI_INFO(plist) acpi_info plist > #define ACPI_WARNING(plist) acpi_warning plist > #define ACPI_EXCEPTION(plist) acpi_exception plist > #define ACPI_ERROR(plist) acpi_error plist > #define ACPI_DEBUG_OBJECT(obj,l,i) acpi_ex_do_debug_object(obj,l,i) I wouldn't have a problem renaming a few of those to something like: #define ACPI_INFO(plist) acpi_old_info plist #define ACPI_WARNING(plist) acpi_old_warning plist #define ACPI_ERROR(plist) acpi_old_error plist The acpi folk might though. > > Other than that, seems fine to me. > Great! Can I consider it as Ack? :) Fix the kfree first. I rarely ack stuff as other people generally have to pick up the changes and I think acks are overrated. cheers, Joe -- 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
On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > This patch introduces acpi_pr_<level>(), where <level> is a message > level such as err/warn/info, to support improved logging messages > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > to ACPI hotplug notify handlers from the ACPI CA. 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: > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > --- > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > 2 files changed, 50 insertions(+), 0 deletions(-) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 3e87c9c..4097266 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > #endif > } > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > + > +/** > + * acpi_printk: Print messages with ACPI prefix and object path > + * > + * This function is intended to be called through acpi_pr_<level> macros. > + */ > +void > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > + char *path; > + acpi_status ret; > + > + va_start(args, fmt); > + > + vaf.fmt = fmt; > + vaf.va = &args; > + > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); One big problem I see with this approach is now each acpi_printk() will result in a call to acpi_get_name() which will invoke several ACPI calls, including a call to acpi_ut_initialize_buffer() which allocates buffer. Is this really warranted? What is the performance impact of this change? -- Shuah -- 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
On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: > On Wed, 2012-07-18 at 14:27 -0700, Joe Perches wrote: > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > level such as err/warn/info, to support improved logging messages > > > for ACPI, esp. in hotplug operations. > > > > One more note: > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > [] > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > [] > > > +void > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > [] > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > [] > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > [] > > > + kfree(buffer.pointer); > > > > There's a failure mode here because buffer.pointer > > isn't guaranteed to be initialized or set to NULL. > > Hi Joe, > > Yes, I thought that check was necessary as well. However, to my > surprise, such check caused the following warning message from > patchcheck.pl. So, I deleted the check... checkpatch ain't very bright. Because buffer is an automatic not a static, buffer.pointer needs to be set to NULL. struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER, .pointer = NULL, }; then the kfree is fine. -- 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
On Wed, 2012-07-18 at 14:54 -0700, Joe Perches wrote: > On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: > > On Wed, 2012-07-18 at 14:21 -0700, Joe Perches wrote: > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > level such as err/warn/info, to support improved logging messages > > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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. > [] > > > I'd be tempted to instead make the calls more like > > > other <subsystem>_<level> uses and rename these to > > > acpi_<level> and change the existing acpi_info to > > > another name. > [] > > I agree with you. Unfortunately, the ACPI CA (ACPI FW interpreter) > > already uses them for its internal-use as follows, so I needed to come > > up with some other name... Hence, acpi_pr_<level>. > > > > /* > > * Error reporting. Callers module and line number are inserted by AE_INFO, > > * the plist contains a set of parens to allow variable-length lists. > > * These macros are used for both the debug and non-debug versions of the code. > > */ > > #define ACPI_INFO(plist) acpi_info plist > > #define ACPI_WARNING(plist) acpi_warning plist > > #define ACPI_EXCEPTION(plist) acpi_exception plist > > #define ACPI_ERROR(plist) acpi_error plist > > #define ACPI_DEBUG_OBJECT(obj,l,i) acpi_ex_do_debug_object(obj,l,i) > > I wouldn't have a problem renaming a few of those to > something like: > > #define ACPI_INFO(plist) acpi_old_info plist > #define ACPI_WARNING(plist) acpi_old_warning plist > #define ACPI_ERROR(plist) acpi_old_error plist > > The acpi folk might though. Hi Joe, ACPI CA is being developed by Intel as OS-neutral code, and is used by multiple OSes including Linux. So, I am not sure how easy to make such changes. I am copying to Lin Ming. > > > Other than that, seems fine to me. > > Great! Can I consider it as Ack? :) > > Fix the kfree first. Please see my other email. Do you think the check should be added despite of the warning message? > I rarely ack stuff as other people generally have to > pick up the changes and I think acks are overrated. That's fair enough. Thanks! -Toshi > cheers, Joe > -- 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
On Wed, 2012-07-18 at 15:06 -0700, Joe Perches wrote: > On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: > > On Wed, 2012-07-18 at 14:27 -0700, Joe Perches wrote: > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > level such as err/warn/info, to support improved logging messages > > > > for ACPI, esp. in hotplug operations. > > > > > > One more note: > > > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > > [] > > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > > [] > > > > +void > > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > > [] > > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > > [] > > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > [] > > > > + kfree(buffer.pointer); > > > > > > There's a failure mode here because buffer.pointer > > > isn't guaranteed to be initialized or set to NULL. > > > > Hi Joe, > > > > Yes, I thought that check was necessary as well. However, to my > > surprise, such check caused the following warning message from > > patchcheck.pl. So, I deleted the check... > > checkpatch ain't very bright. > > Because buffer is an automatic not a static, > buffer.pointer needs to be set to NULL. > > struct acpi_buffer buffer = { > .length = ACPI_ALLOCATE_BUFFER, > .pointer = NULL, > }; > > then the kfree is fine. Hi Joe, Very good point! I will update with the change. Thanks, -Toshi > > -- > 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 -- 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
On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > This patch introduces acpi_pr_<level>(), where <level> is a message > > level such as err/warn/info, to support improved logging messages > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > to ACPI hotplug notify handlers from the ACPI CA. 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: > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > > --- > > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > > 2 files changed, 50 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > index 3e87c9c..4097266 100644 > > --- a/drivers/acpi/utils.c > > +++ b/drivers/acpi/utils.c > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > #endif > > } > > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > > + > > +/** > > + * acpi_printk: Print messages with ACPI prefix and object path > > + * > > + * This function is intended to be called through acpi_pr_<level> macros. > > + */ > > +void > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > +{ > > + struct va_format vaf; > > + va_list args; > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > + char *path; > > + acpi_status ret; > > + > > + va_start(args, fmt); > > + > > + vaf.fmt = fmt; > > + vaf.va = &args; > > + > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > One big problem I see with this approach is now each acpi_printk() will > result in a call to acpi_get_name() which will invoke several ACPI > calls, including a call to acpi_ut_initialize_buffer() which allocates > buffer. Is this really warranted? What is the performance impact of this > change? Hi Shuah, This interface is intended to be used by acpi_pr_<level>(), which is used for error, warning, debugging, etc. It is not intended to be used in any performance path. Thanks, -Toshi > > -- Shuah > > -- 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
On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > level such as err/warn/info, to support improved logging messages > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > > to ACPI hotplug notify handlers from the ACPI CA. 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: > > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > > > --- > > > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > > > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > > > 2 files changed, 50 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > > index 3e87c9c..4097266 100644 > > > --- a/drivers/acpi/utils.c > > > +++ b/drivers/acpi/utils.c > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > > #endif > > > } > > > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > > > + > > > +/** > > > + * acpi_printk: Print messages with ACPI prefix and object path > > > + * > > > + * This function is intended to be called through acpi_pr_<level> macros. > > > + */ > > > +void > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > > +{ > > > + struct va_format vaf; > > > + va_list args; > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > > + char *path; > > > + acpi_status ret; > > > + > > > + va_start(args, fmt); > > > + > > > + vaf.fmt = fmt; > > > + vaf.va = &args; > > > + > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > One big problem I see with this approach is now each acpi_printk() will > > result in a call to acpi_get_name() which will invoke several ACPI > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > buffer. Is this really warranted? What is the performance impact of this > > change? > > Hi Shuah, > > This interface is intended to be used by acpi_pr_<level>(), which is > used for error, warning, debugging, etc. It is not intended to be used > in any performance path. > How does one enable this interface to see errors, warns, debugging? Is there a special mode kernel needs to run in? I am trying to understand what you mean by "not intended to be used in any performance path". Does one build a special kernel similar to CONFIG_VM_DEBUG (just happen to the one I could think off) ? -- Shuah -- 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
On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > level such as err/warn/info, to support improved logging messages > > > for ACPI, esp. in hotplug operations. [] > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c [] > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, [] > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; [] > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > One big problem I see with this approach is now each acpi_printk() will > > result in a call to acpi_get_name() which will invoke several ACPI > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > buffer. Is this really warranted? What is the performance impact of this > > change? [] > This interface is intended to be used by acpi_pr_<level>(), which is > used for error, warning, debugging, etc. It is not intended to be used > in any performance path. While it's not performance critical, perhaps the buffer alloc/free could be avoided by using stack. Something like: char name[ACPI_PATH_SEGMENT_LENGTH * max_segments ? ]; struct acpi_buffer buffer = { .length = ACPI_PATH_SEGMENT_LENGTH, .buffer = name, }; -- 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
On Wed, 2012-07-18 at 16:40 -0600, Shuah Khan wrote: > On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > level such as err/warn/info, to support improved logging messages > > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > > > to ACPI hotplug notify handlers from the ACPI CA. 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: > > > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > > > > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > > > > --- > > > > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > > > > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > > > > 2 files changed, 50 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > > > index 3e87c9c..4097266 100644 > > > > --- a/drivers/acpi/utils.c > > > > +++ b/drivers/acpi/utils.c > > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > > > #endif > > > > } > > > > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > > > > + > > > > +/** > > > > + * acpi_printk: Print messages with ACPI prefix and object path > > > > + * > > > > + * This function is intended to be called through acpi_pr_<level> macros. > > > > + */ > > > > +void > > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > > > +{ > > > > + struct va_format vaf; > > > > + va_list args; > > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > > > + char *path; > > > > + acpi_status ret; > > > > + > > > > + va_start(args, fmt); > > > > + > > > > + vaf.fmt = fmt; > > > > + vaf.va = &args; > > > > + > > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > > > One big problem I see with this approach is now each acpi_printk() will > > > result in a call to acpi_get_name() which will invoke several ACPI > > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > > buffer. Is this really warranted? What is the performance impact of this > > > change? > > > > Hi Shuah, > > > > This interface is intended to be used by acpi_pr_<level>(), which is > > used for error, warning, debugging, etc. It is not intended to be used > > in any performance path. > > > > How does one enable this interface to see errors, warns, debugging? Is > there a special mode kernel needs to run in? I am trying to understand > what you mean by "not intended to be used in any performance path". Does > one build a special kernel similar to CONFIG_VM_DEBUG (just happen to > the one I could think off) ? acpi_pr_<level>() calls printk() with a corresponding message level, such as KERN_ERR, KERN_WARNING and KERN_DEBUG, which is by definition used for error, warning and debugging messages. Let me know if the change log was not clear about this. Anyway, I think one should not use a printk() in performance path in the first place... BTW, I realized that I forgot to define acpi_pr_debug()! (duh) I will update the patch to define it. Thanks, -Toshi > -- Shuah > -- 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
On Wed, 2012-07-18 at 16:52 -0600, Toshi Kani wrote: > On Wed, 2012-07-18 at 16:40 -0600, Shuah Khan wrote: > > On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > > > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > > level such as err/warn/info, to support improved logging messages > > > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > > > > to ACPI hotplug notify handlers from the ACPI CA. 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: > > > > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > > > > > > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > > > > > --- > > > > > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > > > > > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > > > > > 2 files changed, 50 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > > > > index 3e87c9c..4097266 100644 > > > > > --- a/drivers/acpi/utils.c > > > > > +++ b/drivers/acpi/utils.c > > > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > > > > #endif > > > > > } > > > > > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > > > > > + > > > > > +/** > > > > > + * acpi_printk: Print messages with ACPI prefix and object path > > > > > + * > > > > > + * This function is intended to be called through acpi_pr_<level> macros. > > > > > + */ > > > > > +void > > > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > > > > +{ > > > > > + struct va_format vaf; > > > > > + va_list args; > > > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > > > > + char *path; > > > > > + acpi_status ret; > > > > > + > > > > > + va_start(args, fmt); > > > > > + > > > > > + vaf.fmt = fmt; > > > > > + vaf.va = &args; > > > > > + > > > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > > > > > One big problem I see with this approach is now each acpi_printk() will > > > > result in a call to acpi_get_name() which will invoke several ACPI > > > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > > > buffer. Is this really warranted? What is the performance impact of this > > > > change? > > > > > > Hi Shuah, > > > > > > This interface is intended to be used by acpi_pr_<level>(), which is > > > used for error, warning, debugging, etc. It is not intended to be used > > > in any performance path. > > > > > > > How does one enable this interface to see errors, warns, debugging? Is > > there a special mode kernel needs to run in? I am trying to understand > > what you mean by "not intended to be used in any performance path". Does > > one build a special kernel similar to CONFIG_VM_DEBUG (just happen to > > the one I could think off) ? > > acpi_pr_<level>() calls printk() with a corresponding message level, > such as KERN_ERR, KERN_WARNING and KERN_DEBUG, which is by definition > used for error, warning and debugging messages. Let me know if the > change log was not clear about this. Anyway, I think one should not use > a printk() in performance path in the first place... KERN_ERR, KERN_WARNING, and KERN_DEBUG are used at run-time. What happens when these new interfaces start getting used widely during run-time. In the case of a serious error, shouldn't the kernel do the minimum to print the message out and not call several acpi routines? This type of feature definitely makes sense for debug, but not for other cases KERN_ERR, KERN_WARNING case. My concern is all the extra work that is done whenever one of these interfaces is called. Can we limit this to special debug cases only. -- Shuah -- 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
On Wed, 2012-07-18 at 15:49 -0700, Joe Perches wrote: > On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > level such as err/warn/info, to support improved logging messages > > > > for ACPI, esp. in hotplug operations. > [] > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > [] > > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > [] > > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > [] > > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > > > One big problem I see with this approach is now each acpi_printk() will > > > result in a call to acpi_get_name() which will invoke several ACPI > > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > > buffer. Is this really warranted? What is the performance impact of this > > > change? > [] > > This interface is intended to be used by acpi_pr_<level>(), which is > > used for error, warning, debugging, etc. It is not intended to be used > > in any performance path. > > While it's not performance critical, perhaps the buffer > alloc/free could be avoided by using stack. Something like: > > char name[ACPI_PATH_SEGMENT_LENGTH * max_segments ? ]; > struct acpi_buffer buffer = { > .length = ACPI_PATH_SEGMENT_LENGTH, > .buffer = name, > }; > Hi Joe, I thought about using stack initially, but I too was not sure how big the buffer size should be, and was a bit afraid of causing kernel stack overflow potentially. Since it is mainly used for error paths in ACPI hotplug handlers, I do not think alloc/free can lead any performance impact. Thanks, -Toshi -- 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
On Wed, 2012-07-18 at 17:18 -0600, Shuah Khan wrote: > On Wed, 2012-07-18 at 16:52 -0600, Toshi Kani wrote: > > On Wed, 2012-07-18 at 16:40 -0600, Shuah Khan wrote: > > > On Wed, 2012-07-18 at 16:26 -0600, Toshi Kani wrote: > > > > On Wed, 2012-07-18 at 15:59 -0600, Shuah Khan wrote: > > > > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > > > > > > This patch introduces acpi_pr_<level>(), where <level> is a message > > > > > > level such as err/warn/info, to support improved logging messages > > > > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed > > > > > > to ACPI hotplug notify handlers from the ACPI CA. 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: > > > > > > ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT > > > > > > > > > > > > Signed-off-by: Toshi Kani <toshi.kani@hp.com> > > > > > > --- > > > > > > drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ > > > > > > include/acpi/acpi_bus.h | 18 ++++++++++++++++++ > > > > > > 2 files changed, 50 insertions(+), 0 deletions(-) > > > > > > > > > > > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > > > > > index 3e87c9c..4097266 100644 > > > > > > --- a/drivers/acpi/utils.c > > > > > > +++ b/drivers/acpi/utils.c > > > > > > @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, > > > > > > #endif > > > > > > } > > > > > > EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); > > > > > > + > > > > > > +/** > > > > > > + * acpi_printk: Print messages with ACPI prefix and object path > > > > > > + * > > > > > > + * This function is intended to be called through acpi_pr_<level> macros. > > > > > > + */ > > > > > > +void > > > > > > +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) > > > > > > +{ > > > > > > + struct va_format vaf; > > > > > > + va_list args; > > > > > > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; > > > > > > + char *path; > > > > > > + acpi_status ret; > > > > > > + > > > > > > + va_start(args, fmt); > > > > > > + > > > > > > + vaf.fmt = fmt; > > > > > > + vaf.va = &args; > > > > > > + > > > > > > + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > > > > > > > > > One big problem I see with this approach is now each acpi_printk() will > > > > > result in a call to acpi_get_name() which will invoke several ACPI > > > > > calls, including a call to acpi_ut_initialize_buffer() which allocates > > > > > buffer. Is this really warranted? What is the performance impact of this > > > > > change? > > > > > > > > Hi Shuah, > > > > > > > > This interface is intended to be used by acpi_pr_<level>(), which is > > > > used for error, warning, debugging, etc. It is not intended to be used > > > > in any performance path. > > > > > > > > > > How does one enable this interface to see errors, warns, debugging? Is > > > there a special mode kernel needs to run in? I am trying to understand > > > what you mean by "not intended to be used in any performance path". Does > > > one build a special kernel similar to CONFIG_VM_DEBUG (just happen to > > > the one I could think off) ? > > > > acpi_pr_<level>() calls printk() with a corresponding message level, > > such as KERN_ERR, KERN_WARNING and KERN_DEBUG, which is by definition > > used for error, warning and debugging messages. Let me know if the > > change log was not clear about this. Anyway, I think one should not use > > a printk() in performance path in the first place... > > KERN_ERR, KERN_WARNING, and KERN_DEBUG are used at run-time. What > happens when these new interfaces start getting used widely during > run-time. In the case of a serious error, shouldn't the kernel do the > minimum to print the message out and not call several acpi routines? acpi_pr_<level>() does not replace pr_<level>(). When the kernel needs the minimum to print the message out, it can continue to use the regular pr_<level>() interface. > This type of feature definitely makes sense for debug, but not for other > cases KERN_ERR, KERN_WARNING case. Can you elaborate why you think this interface does not make sense for KERN_ERR and KERN_WARNING? As described in the change log, we need to know which object caused an error in order to diagnose an issue. This is a critical piece of the information to start analyzing. Without this interface, error paths in the hotplug handlers would have to call acpi_get_name() by itself in order to log the same information. This is much more complicated and is not saving any time. > My concern is all the extra work that is done whenever one of these > interfaces is called. Can we limit this to special debug cases only. 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. Thanks, -Toshi > -- Shuah > -- 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
>> I wouldn't have a problem renaming a few of those to >> something like: >> >> #define ACPI_INFO(plist) acpi_old_info plist >> #define ACPI_WARNING(plist) acpi_old_warning plist >> #define ACPI_ERROR(plist) acpi_old_error plist >> >> The acpi folk might though. > >Hi Joe, > >ACPI CA is being developed by Intel as OS-neutral code, and is used by >multiple OSes including Linux. So, I am not sure how easy to make such >changes. I am copying to Lin Ming. Please don't even consider doing something like this. As we continue to develop and maintain the ACPICA code, these kinds of OS-specific divergences from the base ACPICA code cause us all kinds of grief, including the accidental creation of new bugs as it becomes more and more difficult to integrate the base ACPICA code back into Linux. In fact, we have a major project this year (and probably far into next year) to continue to minimize (i.e., fix) this type of Linux/ACPICA divergence -- of which many have crept in over the years that ACPICA has been present in the Linux kernel. Bob >-----Original Message----- >From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- >owner@vger.kernel.org] On Behalf Of Toshi Kani >Sent: Wednesday, July 18, 2012 3:08 PM >To: Joe Perches; Lin, Ming M >Cc: lenb@kernel.org; linux-acpi@vger.kernel.org; linux- >kernel@vger.kernel.org; bhelgaas@google.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 1/4] ACPI: Add acpi_pr_<level>() interfaces > >On Wed, 2012-07-18 at 14:54 -0700, Joe Perches wrote: >> On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: >> > On Wed, 2012-07-18 at 14:21 -0700, Joe Perches wrote: >> > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: >> > > > This patch introduces acpi_pr_<level>(), where <level> is a message >> > > > level such as err/warn/info, to support improved logging messages >> > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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. >> [] >> > > I'd be tempted to instead make the calls more like >> > > other <subsystem>_<level> uses and rename these to >> > > acpi_<level> and change the existing acpi_info to >> > > another name. >> [] >> > I agree with you. Unfortunately, the ACPI CA (ACPI FW interpreter) >> > already uses them for its internal-use as follows, so I needed to come >> > up with some other name... Hence, acpi_pr_<level>. >> > >> > /* >> > * Error reporting. Callers module and line number are inserted by >AE_INFO, >> > * the plist contains a set of parens to allow variable-length lists. >> > * These macros are used for both the debug and non-debug versions of >the code. >> > */ >> > #define ACPI_INFO(plist) acpi_info plist >> > #define ACPI_WARNING(plist) acpi_warning plist >> > #define ACPI_EXCEPTION(plist) acpi_exception plist >> > #define ACPI_ERROR(plist) acpi_error plist >> > #define ACPI_DEBUG_OBJECT(obj,l,i) >acpi_ex_do_debug_object(obj,l,i) >> >> I wouldn't have a problem renaming a few of those to >> something like: >> >> #define ACPI_INFO(plist) acpi_old_info plist >> #define ACPI_WARNING(plist) acpi_old_warning plist >> #define ACPI_ERROR(plist) acpi_old_error plist >> >> The acpi folk might though. > >Hi Joe, > >ACPI CA is being developed by Intel as OS-neutral code, and is used by >multiple OSes including Linux. So, I am not sure how easy to make such >changes. I am copying to Lin Ming. > > >> > > Other than that, seems fine to me. >> > Great! Can I consider it as Ack? :) >> >> Fix the kfree first. > >Please see my other email. Do you think the check should be added >despite of the warning message? > > >> I rarely ack stuff as other people generally have to >> pick up the changes and I think acks are overrated. > >That's fair enough. > >Thanks! >-Toshi > > >> cheers, Joe >> > > >-- >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 -- 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
On Thu, 2012-07-19 at 05:35 +0000, Moore, Robert wrote: > >> I wouldn't have a problem renaming a few of those to > >> something like: > >> > >> #define ACPI_INFO(plist) acpi_old_info plist > >> #define ACPI_WARNING(plist) acpi_old_warning plist > >> #define ACPI_ERROR(plist) acpi_old_error plist > >> > >> The acpi folk might though. > > > >Hi Joe, > > > >ACPI CA is being developed by Intel as OS-neutral code, and is used by > >multiple OSes including Linux. So, I am not sure how easy to make such > >changes. I am copying to Lin Ming. > > > Please don't even consider doing something like this. As we continue to > develop and maintain the ACPICA code, these kinds of OS-specific divergences > from the base ACPICA code cause us all kinds of grief, including the accidental > creation of new bugs as it becomes more and more difficult to integrate the base > ACPICA code back into Linux. Hi Bob, Thanks for the clarification! I agree with you. > In fact, we have a major project this year (and probably far into next year) to > continue to minimize (i.e., fix) this type of Linux/ACPICA divergence -- of which > many have crept in over the years that ACPICA has been present in the Linux kernel. Cool! I think that's very good improvement. Thanks again, -Toshi > Bob > > > >-----Original Message----- > >From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- > >owner@vger.kernel.org] On Behalf Of Toshi Kani > >Sent: Wednesday, July 18, 2012 3:08 PM > >To: Joe Perches; Lin, Ming M > >Cc: lenb@kernel.org; linux-acpi@vger.kernel.org; linux- > >kernel@vger.kernel.org; bhelgaas@google.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 1/4] ACPI: Add acpi_pr_<level>() interfaces > > > >On Wed, 2012-07-18 at 14:54 -0700, Joe Perches wrote: > >> On Wed, 2012-07-18 at 15:41 -0600, Toshi Kani wrote: > >> > On Wed, 2012-07-18 at 14:21 -0700, Joe Perches wrote: > >> > > On Wed, 2012-07-18 at 14:40 -0600, Toshi Kani wrote: > >> > > > This patch introduces acpi_pr_<level>(), where <level> is a message > >> > > > level such as err/warn/info, to support improved logging messages > >> > > > for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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. > >> [] > >> > > I'd be tempted to instead make the calls more like > >> > > other <subsystem>_<level> uses and rename these to > >> > > acpi_<level> and change the existing acpi_info to > >> > > another name. > >> [] > >> > I agree with you. Unfortunately, the ACPI CA (ACPI FW interpreter) > >> > already uses them for its internal-use as follows, so I needed to come > >> > up with some other name... Hence, acpi_pr_<level>. > >> > > >> > /* > >> > * Error reporting. Callers module and line number are inserted by > >AE_INFO, > >> > * the plist contains a set of parens to allow variable-length lists. > >> > * These macros are used for both the debug and non-debug versions of > >the code. > >> > */ > >> > #define ACPI_INFO(plist) acpi_info plist > >> > #define ACPI_WARNING(plist) acpi_warning plist > >> > #define ACPI_EXCEPTION(plist) acpi_exception plist > >> > #define ACPI_ERROR(plist) acpi_error plist > >> > #define ACPI_DEBUG_OBJECT(obj,l,i) > >acpi_ex_do_debug_object(obj,l,i) > >> > >> I wouldn't have a problem renaming a few of those to > >> something like: > >> > >> #define ACPI_INFO(plist) acpi_old_info plist > >> #define ACPI_WARNING(plist) acpi_old_warning plist > >> #define ACPI_ERROR(plist) acpi_old_error plist > >> > >> The acpi folk might though. > > > >Hi Joe, > > > >ACPI CA is being developed by Intel as OS-neutral code, and is used by > >multiple OSes including Linux. So, I am not sure how easy to make such > >changes. I am copying to Lin Ming. > > > > > >> > > Other than that, seems fine to me. > >> > Great! Can I consider it as Ack? :) > >> > >> Fix the kfree first. > > > >Please see my other email. Do you think the check should be added > >despite of the warning message? > > > > > >> I rarely ack stuff as other people generally have to > >> pick up the changes and I think acks are overrated. > > > >That's fair enough. > > > >Thanks! > >-Toshi > > > > > >> cheers, Joe > >> > > > > > >-- > >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 -- 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
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. [ 0.000000] ACPI: RSDP 00000000000fc600 00024 (v02 HPQOEM) [ 0.000000] ACPI: XSDT 00000000bb7fe120 00084 (v01 HPQOEM SLIC-MPC 0000000F 01000013) [ 0.000000] ACPI: FACP 00000000bb7fc000 000F4 (v03 HPQOEM 172A 0000000F HP 00000001) [ 0.000000] ACPI: DSDT 00000000bb7d6000 203A2 (v02 HPQOEM 172A 00000001 INTL 20060912) [ 0.000000] ACPI: FACS 00000000bb760000 00040 [ 0.000000] ACPI: HPET 00000000bb7fb000 00038 (v01 HPQOEM 172A 00000001 HP 00000001) [ 0.000000] ACPI: APIC 00000000bb7fa000 000BC (v01 HPQOEM 172A 00000001 HP 00000001) [ 0.000000] ACPI: MCFG 00000000bb7f9000 0003C (v01 HPQOEM 172A 00000001 HP 00000001) [ 0.000000] ACPI: TCPA 00000000bb7f7000 00032 (v02 HPQOEM 172A 00000000 HP 00000001) [ 0.000000] ACPI: SSDT 00000000bb7d3000 00135 (v01 HPQOEM SataAhci 00001000 INTL 20060912) [ 0.000000] ACPI: SSDT 00000000bb7d2000 00314 (v01 HPQOEM PtidDevc 00001000 INTL 20060912) [ 0.000000] ACPI: SLIC 00000000bb7d1000 00176 (v01 HPQOEM SLIC-MPC 00000001 HP 00000001) [ 0.000000] ACPI: SSDT 00000000bb7d0000 00A10 (v01 PmRef CpuPm 00003000 INTL 20060912) [ 0.000000] ACPI: SSDT 00000000bb7cf000 00288 (v01 PmRef Cpu0Tst 00003000 INTL 20060912) [ 0.000000] ACPI: SSDT 00000000bb7ce000 00225 (v01 PmRef ApTst 00003000 INTL 20060912) [ 0.000000] ACPI: ASF! 00000000bb7f8000 000A0 (v32 HPQOEM 172A 00000001 HP 00000001) [ 0.000000] ACPI: Local APIC address 0xfee00000 [ 0.000000] ACPI: PM-Timer IO Port: 0x408 [ 0.000000] ACPI: Local APIC address 0xfee00000 [ 0.000000] ACPI: LAPIC (acpi_id[0x00] lapic_id[0x00] enabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x01] lapic_id[0x01] enabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x02] lapic_id[0x04] enabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x03] lapic_id[0x05] enabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x04] lapic_id[0x00] disabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x05] lapic_id[0x00] disabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x06] lapic_id[0x00] disabled) [ 0.000000] ACPI: LAPIC (acpi_id[0x07] lapic_id[0x00] disabled) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x00] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x01] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x02] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x03] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x04] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x05] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x06] high edge lint[0x1]) [ 0.000000] ACPI: LAPIC_NMI (acpi_id[0x07] high edge lint[0x1]) [ 0.000000] ACPI: IOAPIC (id[0x01] address[0xfec00000] gsi_base[0]) [ 0.000000] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl) [ 0.000000] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level) [ 0.000000] ACPI: IRQ0 used by override. [ 0.000000] ACPI: IRQ2 used by override. [ 0.000000] ACPI: IRQ9 used by override. [ 0.000000] Using ACPI (MADT) for SMP configuration information [ 0.000000] ACPI: HPET id: 0x8086a201 base: 0xfed00000 [ 0.003827] ACPI: Core revision 20120320 [ 0.229840] PM: Registering ACPI NVS region [mem 0xbb6c2000-0xbb7c1fff] (1048576 bytes) [ 0.230694] ACPI FADT declares the system doesn't support PCIe ASPM, so disable it [ 0.230698] ACPI: bus type pci registered [ 0.246838] ACPI: Added _OSI(Module Device) [ 0.246841] ACPI: Added _OSI(Processor Device) [ 0.246843] ACPI: Added _OSI(3.0 _SCP Extensions) [ 0.246845] ACPI: Added _OSI(Processor Aggregator Device) [ 0.248464] ACPI: EC: Look up EC in DSDT [ 0.250886] ACPI: Executed 1 blocks of module-level executable AML code [ 0.256201] [Firmware Bug]: ACPI: BIOS _OSI(Linux) query ignored [ 0.264485] ACPI: SSDT 00000000bb6bba18 0047D (v01 PmRef Cpu0Ist 00003000 INTL 20060912) [ 0.264874] ACPI: Dynamic OEM Table Load: [ 0.264878] ACPI: SSDT (null) 0047D (v01 PmRef Cpu0Ist 00003000 INTL 20060912) [ 0.264995] ACPI: SSDT 00000000bb6b9018 008AA (v01 PmRef Cpu0Cst 00003001 INTL 20060912) [ 0.265369] ACPI: Dynamic OEM Table Load: [ 0.265372] ACPI: SSDT (null) 008AA (v01 PmRef Cpu0Cst 00003001 INTL 20060912) [ 0.265589] ACPI: SSDT 00000000bb6baa98 00303 (v01 PmRef ApIst 00003000 INTL 20060912) [ 0.266008] ACPI: Dynamic OEM Table Load: [ 0.266012] ACPI: SSDT (null) 00303 (v01 PmRef ApIst 00003000 INTL 20060912) [ 0.266113] ACPI: SSDT 00000000bb6b8d98 00119 (v01 PmRef ApCst 00003000 INTL 20060912) [ 0.266506] ACPI: Dynamic OEM Table Load: [ 0.266510] ACPI: SSDT (null) 00119 (v01 PmRef ApCst 00003000 INTL 20060912) [ 0.698211] ACPI: Interpreter enabled [ 0.698230] ACPI: (supports S0 S3 S4 S5) [ 0.698235] ACPI: Using IOAPIC for interrupt routing [ 0.699577] ACPI: Power Resource [APPR] (off) [ 0.700772] ACPI: Power Resource [LPP] (on) [ 0.702856] ACPI: EC: GPE = 0x16, I/O: command/status = 0x66, data = 0x62 [ 0.703023] ACPI: No dock devices found. [ 0.703027] PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug [ 0.703627] ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-fe]) [ 0.713983] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT] [ 0.714162] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.PCIB._PRT] [ 0.714203] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP01._PRT] [ 0.714228] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP02._PRT] [ 0.714269] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP04._PRT] [ 0.714608] pci0000:00: Requesting ACPI _OSC control (0x1d) [ 0.715476] pci0000:00: ACPI _OSC control (0x1d) granted [ 0.719729] ACPI: PCI Root Bridge [CPBG] (domain 0000 [bus ff]) [ 0.719885] pci0000:ff: Requesting ACPI _OSC control (0x1d) [ 0.719888] pci0000:ff: ACPI _OSC request failed (AE_NOT_FOUND), returned control mask: 0x1d [ 0.719891] ACPI _OSC control for PCIe not granted, disabling ASPM [ 0.720049] ACPI: PCI Interrupt Link [LNKA] (IRQs 1 3 4 5 6 7 *10 12 14 15) [ 0.720087] ACPI: PCI Interrupt Link [LNKB] (IRQs 1 3 4 5 6 7 *11 12 14 15) [ 0.720124] ACPI: PCI Interrupt Link [LNKC] (IRQs 1 3 4 5 6 7 *10 12 14 15) [ 0.720160] ACPI: PCI Interrupt Link [LNKD] (IRQs 1 3 4 *5 6 7 11 12 14 15) [ 0.720195] ACPI: PCI Interrupt Link [LNKE] (IRQs 1 3 4 5 6 7 *10 12 14 15) [ 0.720230] ACPI: PCI Interrupt Link [LNKF] (IRQs 1 3 4 5 6 7 11 12 14 15) *10 [ 0.720266] ACPI: PCI Interrupt Link [LNKG] (IRQs 1 3 4 5 6 7 10 12 14 15) *0, disabled. [ 0.720301] ACPI: PCI Interrupt Link [LNKH] (IRQs 1 3 4 5 6 7 11 12 14 15) *0, disabled. [ 0.720559] ACPI: bus type usb registered [ 0.720658] PCI: Using ACPI for IRQ routing [ 0.737801] pnp: PnP ACPI init [ 0.737817] ACPI: bus type pnp registered [ 0.738257] pnp 00:00: Plug and Play ACPI device, IDs PNP0a08 PNP0a03 (active) [ 0.738500] system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active) [ 0.738593] pnp 00:02: Plug and Play ACPI device, IDs PNP0200 (active) [ 0.738619] pnp 00:03: Plug and Play ACPI device, IDs INT0800 (active) [ 0.738685] pnp 00:04: Plug and Play ACPI device, IDs IFX0102 PNP0c31 (active) [ 0.738770] pnp 00:05: Plug and Play ACPI device, IDs PNP0103 (active) [ 0.738810] pnp 00:06: Plug and Play ACPI device, IDs PNP0c04 (active) [ 0.738895] system 00:07: Plug and Play ACPI device, IDs PNP0c02 (active) [ 0.738930] pnp 00:08: Plug and Play ACPI device, IDs PNP0b00 (active) [ 0.739325] pnp 00:09: Plug and Play ACPI device, IDs PNP0401 (active) [ 0.739361] pnp 00:0a: Plug and Play ACPI device, IDs PNP0303 (active) [ 0.739396] pnp 00:0b: Plug and Play ACPI device, IDs SYN0165 SYN0100 SYN0002 PNP0f13 (active) [ 0.739602] pnp 00:0c: Plug and Play ACPI device, IDs HPQ0004 (active) [ 0.739742] pnp 00:0d: Plug and Play ACPI device, IDs PNP0a03 (active) [ 0.739841] pnp: PnP ACPI: found 14 devices [ 0.739844] ACPI: ACPI bus type pnp unregistered [ 1.078598] ACPI: Deprecated procfs I/F for AC is loaded, please retry with CONFIG_ACPI_PROCFS_POWER cleared [ 1.078674] ACPI: AC Adapter [AC] (on-line) [ 1.078843] ACPI: Sleep Button [SLPB] [ 1.078892] ACPI: Lid Switch [LID] [ 1.078923] ACPI: Power Button [PWRF] [ 1.078982] ACPI: Requesting acpi_cpufreq [ 1.093567] ACPI: Thermal Zone [EXTZ] (0 C) [ 1.095993] ACPI: Thermal Zone [EX2Z] (26 C) [ 1.097220] ACPI: Thermal Zone [PWMZ] (0 C) [ 1.099681] ACPI: Thermal Zone [LOCZ] (25 C) [ 1.100065] ACPI: Thermal Zone [GFXZ] (0 C) [ 1.110366] ACPI: Thermal Zone [BATZ] (27 C) [ 1.120051] ACPI: Thermal Zone [EGXZ] (0 C) [ 1.120219] ACPI: Thermal Zone [CPUZ] (38 C) [ 1.120350] ACPI: Thermal Zone [MCHZ] (35 C) [ 1.120475] ACPI: Thermal Zone [PCHZ] (59 C) [ 1.165755] ACPI: Deprecated procfs I/F for battery is loaded, please retry with CONFIG_ACPI_PROCFS_POWER cleared [ 1.165764] ACPI: Battery Slot [BAT0] (battery present) [ 1.165807] ACPI: Deprecated procfs I/F for battery is loaded, please retry with CONFIG_ACPI_PROCFS_POWER cleared [ 1.165813] ACPI: Battery Slot [BAT1] (battery absent) [ 5.465815] ACPI: Video Device [GFX0] (multi-head: yes rom: no post: no) [ 43.954919] parport_pc 00:09: reported by Plug and Play ACPI [ 45.027388] ACPI Warning: 0x0000000000000460-0x000000000000047f SystemIO conflicts with Region \PMIO 1 (20120320/utaddress-251) [ 45.027390] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver [ 45.027397] ACPI Warning: 0x0000000000000428-0x000000000000042f SystemIO conflicts with Region \PMIO 1 (20120320/utaddress-251) [ 45.027398] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver [ 45.027404] ACPI Warning: 0x0000000000000500-0x000000000000057f SystemIO conflicts with Region \GPIO 1 (20120320/utaddress-251) [ 45.027405] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver [ 47.432241] snd_hda_intel 0000:00:1b.0: power state changed by ACPI to D0 [ 47.432254] snd_hda_intel 0000:00:1b.0: power state changed by ACPI to D0 -- Shuah -- 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
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. or maybe cache one or two. > 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. > > [ 0.000000] ACPI: RSDP 00000000000fc600 00024 (v02 HPQOEM) > [ 0.000000] ACPI: XSDT 00000000bb7fe120 00084 (v01 HPQOEM SLIC-MPC > 0000000F 01000013) [120+ lines of ACPI stuff] > [ 0.739844] ACPI: ACPI bus type pnp unregistered I think ACPI is the noisiest subsystem. I'd rather see this logging made quieter by conversion to KERN_DEBUG or another selective mechanism. There just aren't many ACPI_INFO calls around and that why I thought it reasonable to convert the macro to call a different named function. -- 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
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 -- 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
On Thu, 2012-07-19 at 11:28 -0600, Toshi Kani wrote: > 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. On the contrary, your patch set could make the existing problems worse by introducing lot of complexity (makes the execution path very long for each and every one these messages) in the path that prints messages. As more and more acpi code paths start using the new interfaces, it will keep getting worse. I am not questioning the usefulness of the additional information, I am questioning the complexity your patch set adds. The added complexity isn't desirable. The design in this patch set needs refinement so it doesn't add to the execution path. -- Shuah -- 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
On Thu, 2012-07-19 at 13:25 -0600, Shuah Khan wrote: > On Thu, 2012-07-19 at 11:28 -0600, Toshi Kani wrote: > > 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. > > On the contrary, your patch set could make the existing problems worse > by introducing lot of complexity (makes the execution path very long for > each and every one these messages) in the path that prints messages. As > more and more acpi code paths start using the new interfaces, it will > keep getting worse. > > I am not questioning the usefulness of the additional information, I am > questioning the complexity your patch set adds. The added complexity > isn't desirable. That's good. In the last email, you suggested to make the interface debug-only, and make it non-default case. This totally defeats the purpose, which is why I explained it. When someone reported an issue, we do not want to tell him that he will need to reproduce it again with debug kernel/option. I am glad to know that you understand this point. > The design in this patch set needs refinement so it doesn't add to the > execution path. I am not sure why you are so much concerned about the complexity. Frankly, acpi_get_name() is one of the simplest and lightest interfaces in the ACPI CA. It does not execute AML at all. The slowness or complexity of ACPI comes when it executes AML due to the way ACPI is defined. acpi_get_name() simply builds a path info and copies it to an allocated buffer. One possible optimization is, like Joe suggested, to avoid a buffer allocation. I prefer not to use stack space as I explained to Joe. We could statically allocate per-CPU buffer for this, but I do not think it is worth doing it. After all, such optimization makes the code complicated, and does not make any difference in performance. At run-time, ACPI is only used when GPE occurs (or accessed from sysfs), which is infrequent event and has nothing to compare with the performance paths like syscall and I/Os. And acpi_pr_<level>() is used in the error paths of such infrequent events. If your concern is actually a performance bottleneck in acpi_get_name() you found in the code, you should report it to the ACPI CA team. Thanks, -Toshi > -- Shuah > > -- 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
On Thu, 2012-07-19 at 14:51 -0600, Toshi Kani wrote: > If your concern is actually a performance bottleneck in acpi_get_name() > you found in the code, you should report it to the ACPI CA team. I have tried my best to get you to understand the problems in bigger picture your patch set can exacerbate. Looking to somebody else to fix the problems doesn't help. It doesn't look like we can come to an agreement here, we just have to agree to disagree. caio, -- Shuah -- 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
On Thu, 2012-07-19 at 16:32 -0600, Shuah Khan wrote: > On Thu, 2012-07-19 at 14:51 -0600, Toshi Kani wrote: > > > If your concern is actually a performance bottleneck in acpi_get_name() > > you found in the code, you should report it to the ACPI CA team. > > I have tried my best to get you to understand the problems in bigger > picture your patch set can exacerbate. Looking to somebody else to fix > the problems doesn't help. It doesn't look like we can come to an > agreement here, we just have to agree to disagree. I am not asking someone to fix it. I tried my best to explain that acpi_get_name() does not lead any performance issue when it is called in the error paths of ACPI drivers, and why we have to call it to obtain an object path info for error analysis. If you still believe there is a performance issue in calling acpi_get_name() under this context, please help us understand where the performance bottleneck is in the code. (I hope you just concerned it because it has "acpi_" prefix...) I will then work on the issue with the ACPI CA team. Thanks, -Toshi > caio, > -- Shuah > -- 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
On Thu, 2012-07-19 at 09:34 -0700, Joe Perches wrote: > 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. > > or maybe cache one or two. Hi Joe, Sorry, I had overlooked this email yesterday... I agree that caching one or two is a good idea when we expect to see repeated calls to a same object. I think there may be a few repeated calls, such that callee fails and calls acpi_pr_<level>() with its error message, and then caller sees this error return and calls acpi_pr_<level>() with its own message. That said, considering additional complexity of locking cache data, etc., I'd prefer keeping the code simple for now since I do not expect this interface be called very often. > > 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. > > > > [ 0.000000] ACPI: RSDP 00000000000fc600 00024 (v02 HPQOEM) > > [ 0.000000] ACPI: XSDT 00000000bb7fe120 00084 (v01 HPQOEM SLIC-MPC > > 0000000F 01000013) > > [120+ lines of ACPI stuff] > > > [ 0.739844] ACPI: ACPI bus type pnp unregistered > > I think ACPI is the noisiest subsystem. I agree for the boot time messages. The use of ACPI is limited at run-time, such as hotplug operations, though. > I'd rather see this logging made quieter by conversion to > KERN_DEBUG or another selective mechanism. > > There just aren't many ACPI_INFO calls around and that why > I thought it reasonable to convert the macro to call a > different named function. I looked at the first two major cases as follows. Looks like there are some considerations to minimize them. ACPI_INFO is suppressed when ACPI_NO_ERROR_MESSAGES is defined. ACPI_INFO((AE_INFO, "RSDP %p %05X (v%.2d %6.6s)", ACPI_CAST_PTR (void, address), (ACPI_CAST_PTR(struct acpi_table_rsdp, header)-> revision > 0) ? ACPI_CAST_PTR(struct acpi_table_rsdp, header)->length : 20, ACPI_CAST_PTR(struct acpi_table_rsdp, header)->revision, local_header.oem_id)); LAPIC info is printed at KERN_INFO. printk(KERN_INFO PREFIX "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n", p->processor_id, p->id, (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" Thanks, -Toshi > > -- > 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 -- 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
On Thu, 2012-07-19 at 17:43 -0600, Toshi Kani wrote: > On Thu, 2012-07-19 at 16:32 -0600, Shuah Khan wrote: > > On Thu, 2012-07-19 at 14:51 -0600, Toshi Kani wrote: > > > > > If your concern is actually a performance bottleneck in acpi_get_name() > > > you found in the code, you should report it to the ACPI CA team. > > > > I have tried my best to get you to understand the problems in bigger > > picture your patch set can exacerbate. Looking to somebody else to fix > > the problems doesn't help. It doesn't look like we can come to an > > agreement here, we just have to agree to disagree. > > I am not asking someone to fix it. I tried my best to explain that > acpi_get_name() does not lead any performance issue when it is called in > the error paths of ACPI drivers, and why we have to call it to obtain an > object path info for error analysis. If you still believe there is a > performance issue in calling acpi_get_name() under this context, please > help us understand where the performance bottleneck is in the code. (I > hope you just concerned it because it has "acpi_" prefix...) I will then > work on the issue with the ACPI CA team. I have measured acpi_pr_<level>() to make sure my statement is correct. Here are the results: Avg. acpi_get_name() 587 ns Avg. printk() 3420 ns Avg. kfree() 238 ns Avg. acpi_get_time()+kfree() 825 ns The results indicate that acpi_pr_<level>() has 20% increase of the time compared to the regular printk(), which is less than 1 us. I believe the results endorse my statement, and may not cause any performance issue to the error paths of the ACPI drivers. -Toshi > Thanks, > -Toshi > > > > > caio, > > -- Shuah > > > -- 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
On Tue, 2012-07-24 at 09:55 -0600, Toshi Kani wrote: > On Thu, 2012-07-19 at 17:43 -0600, Toshi Kani wrote: > > On Thu, 2012-07-19 at 16:32 -0600, Shuah Khan wrote: > > > On Thu, 2012-07-19 at 14:51 -0600, Toshi Kani wrote: > > > > > > > If your concern is actually a performance bottleneck in acpi_get_name() > > > > you found in the code, you should report it to the ACPI CA team. > > > > > > I have tried my best to get you to understand the problems in bigger > > > picture your patch set can exacerbate. Looking to somebody else to fix > > > the problems doesn't help. It doesn't look like we can come to an > > > agreement here, we just have to agree to disagree. > > > > I am not asking someone to fix it. I tried my best to explain that > > acpi_get_name() does not lead any performance issue when it is called in > > the error paths of ACPI drivers, and why we have to call it to obtain an > > object path info for error analysis. If you still believe there is a > > performance issue in calling acpi_get_name() under this context, please > > help us understand where the performance bottleneck is in the code. (I > > hope you just concerned it because it has "acpi_" prefix...) I will then > > work on the issue with the ACPI CA team. > > I have measured acpi_pr_<level>() to make sure my statement is correct. > Here are the results: > > Avg. acpi_get_name() 587 ns > Avg. printk() 3420 ns > Avg. kfree() 238 ns > Avg. acpi_get_time()+kfree() 825 ns > > The results indicate that acpi_pr_<level>() has 20% increase of the time Oops, I should have stated that it is 24% increase to printk(), or 20% of time in acpi_pr_<level>(). -Toshi > compared to the regular printk(), which is less than 1 us. I believe > the results endorse my statement, and may not cause any performance > issue to the error paths of the ACPI drivers. > > -Toshi > > > > Thanks, > > -Toshi > > > > > > > > > caio, > > > -- Shuah > > > > > > -- 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/drivers/acpi/utils.c b/drivers/acpi/utils.c index 3e87c9c..4097266 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -454,3 +454,35 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event, #endif } EXPORT_SYMBOL(acpi_evaluate_hotplug_ost); + +/** + * acpi_printk: Print messages with ACPI prefix and object path + * + * This function is intended to be called through acpi_pr_<level> macros. + */ +void +acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER}; + char *path; + acpi_status ret; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = &args; + + ret = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); + if (ret == AE_OK) + path = buffer.pointer; + else + path = "<n/a>"; + + printk("%sACPI: %s: %pV", level, path, &vaf); + + va_end(args); + kfree(buffer.pointer); +} +EXPORT_SYMBOL(acpi_printk); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index b22b774..c3a175d 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -85,6 +85,24 @@ struct acpi_pld { acpi_status acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld *pld); + +void acpi_printk(const char *level, acpi_handle handle, const char *fmt, ...); + +#define acpi_pr_emerg(handle, fmt, ...) \ + acpi_printk(KERN_EMERG, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_alert(handle, fmt, ...) \ + acpi_printk(KERN_ALERT, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_crit(handle, fmt, ...) \ + acpi_printk(KERN_CRIT, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_err(handle, fmt, ...) \ + acpi_printk(KERN_ERR, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_warn(handle, fmt, ...) \ + acpi_printk(KERN_WARNING, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_notice(handle, fmt, ...) \ + acpi_printk(KERN_NOTICE, handle, fmt, ##__VA_ARGS__) +#define acpi_pr_info(handle, fmt, ...) \ + acpi_printk(KERN_INFO, handle, fmt, ##__VA_ARGS__) + #ifdef CONFIG_ACPI #include <linux/proc_fs.h>
This patch introduces acpi_pr_<level>(), where <level> is a message level such as err/warn/info, to support improved logging messages for ACPI, esp. in hotplug operations. acpi_pr_<level>() 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_<level>() takes acpi_handle as an argument, which is passed to ACPI hotplug notify handlers from the ACPI CA. 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: ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT Signed-off-by: Toshi Kani <toshi.kani@hp.com> --- drivers/acpi/utils.c | 32 ++++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 18 ++++++++++++++++++ 2 files changed, 50 insertions(+), 0 deletions(-)