On Tuesday, November 06, 2012 08:02:06 AM Toshi Kani wrote:
> This patch introduces acpi_pr_<level>(), where <level> is a kernel
> message level such as err/warn/info, to support improved logging
> messages for ACPI, esp. for hotplug operations. acpi_pr_<level>()
> appends "ACPI" prefix and ACPI object path to the messages. This
> improves diagnosis of hotplug operations since an error message in
> a log file identifies an object that caused an issue.
>
> acpi_pr_<level>() 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:
> 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_<level>() when they need to identify
> a target ACPI object path in their messages, such as error cases.
> The usage model is similar to dev_<level>(). acpi_pr_<level>() can
> be used when device is not created/valid, which may be the case in
> ACPI hotplug handlers. ACPI object path is also consistent on the
> platform, unlike device name that changes over hotplug operations.
>
> ACPI drivers should use dev_<level>() when device is valid and
> acpi_pr_<level>() is already used by the caller in its error path.
> Device name provides more user friendly information.
>
> ACPI drivers also continue to use pr_<level>() when messages do not
> need to specify device information, such as boot-up messages.
>
> Note: ACPI_[WARNING|INFO|ERROR]() are intended for the ACPICA and
> are not associated with the kernel message level.
Well, the idea is generally good, but unfortunately acpi_get_name() is
not a cheap operation. Namely, it takes the global namespace mutex,
so your acpi_printk() may be a source of serious contention on that
lock if used excessively from concurrent threads.
Do you think you can address this problem?
Moreover, this also means that acpi_printk() cannot be used from interrupt
context, so it is not a printk() replacement, which at least should be
documented.
Thanks,
Rafael
> Signed-off-by: Toshi Kani <toshi.kani@xxxxxx>
> Tested-by: Vijay Mohan Pandarathil <vijaymohan.pandarathil@xxxxxx>
> ---
> drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 462f7e3..4eade7d 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -457,3 +457,37 @@ 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 = {
> + .length = ACPI_ALLOCATE_BUFFER,
> + .pointer = NULL
> + };
> + const 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 2242c10..93d5852 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -56,6 +56,37 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
>
> acpi_status
> acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **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__)
> +
> +/* REVISIT: Support CONFIG_DYNAMIC_DEBUG when necessary */
> +#if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
> +#define acpi_pr_debug(handle, fmt, ...) \
> + acpi_printk(KERN_DEBUG, handle, fmt, ##__VA_ARGS__)
> +#else
> +#define acpi_pr_debug(handle, fmt, ...) \
> +({ \
> + if (0) \
> + acpi_printk(KERN_DEBUG, handle, fmt, ##__VA_ARGS__); \
> + 0; \
> +})
> +#endif
> +
> #ifdef CONFIG_ACPI
>
> #include <linux/proc_fs.h>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
