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

[Rafael J. Wysocki]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/