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

Commit Message

Toshi Kani Nov. 6, 2012, 3:02 p.m. UTC

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.

Signed-off-by: Toshi Kani <[e-mail]>
Tested-by: Vijay Mohan Pandarathil <[e-mail]>
---
 drivers/acpi/utils.c    | 34 ++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

Comments

Rafael Wysocki Nov. 19, 2012, 8:10 a.m. UTC | #1

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 <[e-mail]>
> Tested-by: Vijay Mohan Pandarathil <[e-mail]>
> ---
>  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>
>

Joe Perches Nov. 19, 2012, 8:56 a.m. UTC | #2

On Tue, 2012-11-06 at 08:02 -0700, 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.
[]
> diff --git 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, ...);

This should be declared with __printf(3, 4)

is acpi_bus.h really the right file for these prototypes?

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Toshi Kani Nov. 19, 2012, 3:37 p.m. UTC | #3

On Mon, 2012-11-19 at 09:10 +0100, Rafael J. Wysocki wrote:
> 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?

Hi Rafael,

I agree with you that the interface name may sound too generic as if it
can be used for any way.  How about changing the interface name to
acpi_hp_<level>() to clarify that it is intended for hot-plug operations
only?  The key goal is to be able to identify a failed device in
hot-plug error messages, so that we can diagnose an issue.  When used in
hot-plug operations, especially in error paths, the lock contention is
not an issue.  In regular code path (i.e. non-hot-plug operations),
dev_<level>() should be used since device object is available.

Here is a measurement result for the interface.  When there is no
locking contention, acpi_get_name() is reasonably fast as it does not
execute AML.

Avg. acpi_get_name()             587 ns
Avg. printk()                   3420 ns
Avg. kfree()                     238 ns
Avg. acpi_get_time()+kfree()     825 ns


> 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.

Right.  I will document it in the comment and change log.


Thanks,
-Toshi


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Toshi Kani Nov. 19, 2012, 3:51 p.m. UTC | #4

On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> On Tue, 2012-11-06 at 08:02 -0700, 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.
> []
> > diff --git 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, ...);
> 
> This should be declared with __printf(3, 4)

Hi Joe,

Yes, I will add __printf(3,4).  Thanks for pointing this out.

> is acpi_bus.h really the right file for these prototypes?

This interface is limited for ACPI, so it should be declared in a header
file under include/acpi.  Among the files in this directory, acpi_bus.h
seems to be a good fit as it declares the interfaces provided by ACPI
core.

Thanks,
-Toshi  


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Joe Perches Nov. 19, 2012, 4:06 p.m. UTC | #5

On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > is acpi_bus.h really the right file for these prototypes?
> This interface is limited for ACPI, so it should be declared in a header
> file under include/acpi.  Among the files in this directory, acpi_bus.h
> seems to be a good fit as it declares the interfaces provided by ACPI
> core.

I'd've put them in acpi.h or maybe created acpi_utils.h 

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Toshi Kani Nov. 19, 2012, 4:16 p.m. UTC | #6

On Mon, 2012-11-19 at 08:06 -0800, Joe Perches wrote:
> On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> > On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > > is acpi_bus.h really the right file for these prototypes?
> > This interface is limited for ACPI, so it should be declared in a header
> > file under include/acpi.  Among the files in this directory, acpi_bus.h
> > seems to be a good fit as it declares the interfaces provided by ACPI
> > core.
> 
> I'd've put them in acpi.h or maybe created acpi_utils.h 

Hi Joe,

We cannot use acpi.h since it is an ACPICA header.  Creating
acpi_utils.h sounds good idea.  However, in line 40 of acpi_bus.h, it
has the following comment and declares multiple utility interfaces.

====
/* acpi_utils.h */
acpi_status
acpi_extract_package(union acpi_object *package,
                struct acpi_buffer *format, struct acpi_buffer *buffer);
  :
====

This looks to me that there was acpi_utils.h before, but someone had
merged it into acpi_bus.h for some reason.  This change was made before
the initial git repository was created, so I cannot check the change
log.  So, as of today, acpi_bus.h seems to be the place to declare ACPI
utility interfaces for ACPI core.

Thanks,
-Toshi 

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Toshi Kani Nov. 19, 2012, 5:26 p.m. UTC | #7

On Mon, 2012-11-19 at 18:35 +0100, Rafael J. Wysocki wrote:
> On Monday, November 19, 2012 09:16:52 AM Toshi Kani wrote:
> > On Mon, 2012-11-19 at 08:06 -0800, Joe Perches wrote:
> > > On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> > > > On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > > > > is acpi_bus.h really the right file for these prototypes?
> > > > This interface is limited for ACPI, so it should be declared in a header
> > > > file under include/acpi.  Among the files in this directory, acpi_bus.h
> > > > seems to be a good fit as it declares the interfaces provided by ACPI
> > > > core.
> > > 
> > > I'd've put them in acpi.h or maybe created acpi_utils.h 
> > 
> > Hi Joe,
> > 
> > We cannot use acpi.h since it is an ACPICA header.
> 
> Which acpi.h are you referring to?


I was referring "include/acpi/acpi.h".

Thanks,
-Toshi



--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael Wysocki Nov. 19, 2012, 5:35 p.m. UTC | #8

On Monday, November 19, 2012 09:16:52 AM Toshi Kani wrote:
> On Mon, 2012-11-19 at 08:06 -0800, Joe Perches wrote:
> > On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> > > On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > > > is acpi_bus.h really the right file for these prototypes?
> > > This interface is limited for ACPI, so it should be declared in a header
> > > file under include/acpi.  Among the files in this directory, acpi_bus.h
> > > seems to be a good fit as it declares the interfaces provided by ACPI
> > > core.
> > 
> > I'd've put them in acpi.h or maybe created acpi_utils.h 
> 
> Hi Joe,
> 
> We cannot use acpi.h since it is an ACPICA header.

Which acpi.h are you referring to?

Rafael

Rafael Wysocki Nov. 20, 2012, 1:04 a.m. UTC | #9

On Monday, November 19, 2012 10:26:43 AM Toshi Kani wrote:
> On Mon, 2012-11-19 at 18:35 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 19, 2012 09:16:52 AM Toshi Kani wrote:
> > > On Mon, 2012-11-19 at 08:06 -0800, Joe Perches wrote:
> > > > On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> > > > > On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > > > > > is acpi_bus.h really the right file for these prototypes?
> > > > > This interface is limited for ACPI, so it should be declared in a header
> > > > > file under include/acpi.  Among the files in this directory, acpi_bus.h
> > > > > seems to be a good fit as it declares the interfaces provided by ACPI
> > > > > core.
> > > > 
> > > > I'd've put them in acpi.h or maybe created acpi_utils.h 
> > > 
> > > Hi Joe,
> > > 
> > > We cannot use acpi.h since it is an ACPICA header.
> > 
> > Which acpi.h are you referring to?
> 
> 
> I was referring "include/acpi/acpi.h".

There is another acpi.h in include/linux that can be used for this purpose.

Thanks,
Rafael

Rafael Wysocki Nov. 20, 2012, 1:17 a.m. UTC | #10

On Monday, November 19, 2012 09:10:58 AM Rafael J. Wysocki wrote:
> 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.

Unfortunately, I lost your reply to my previous message in this thread
due to my e-mail client malfunction.  Sorry about that.

What about calling them acpi_handle_printk() and acpi_handle_<level>,
respectively?  Then, if it is clearly documented that those things
acquire the global namespace mutex and are not suitable for interrupt
context, it should be OK.

And please take Joe's feedback into account. :-)

Thanks,
Rafael

Toshi Kani Nov. 20, 2012, 1:18 a.m. UTC | #11

On Tue, 2012-11-20 at 02:17 +0100, Rafael J. Wysocki wrote:
> On Monday, November 19, 2012 09:10:58 AM Rafael J. Wysocki wrote:
> > 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.
> 
> Unfortunately, I lost your reply to my previous message in this thread
> due to my e-mail client malfunction.  Sorry about that.
> 
> What about calling them acpi_handle_printk() and acpi_handle_<level>,
> respectively?  Then, if it is clearly documented that those things
> acquire the global namespace mutex and are not suitable for interrupt
> context, it should be OK.

Sounds good to me.  I will update the function names and document it in
the comment and change log.

> And please take Joe's feedback into account. :-)

Yes, Joe has been great help on this patchset (Thanks Joe!).  I think
the only remaining point is the header file location, which I will reply
to your other email.

Thanks,
-Toshi


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Toshi Kani Nov. 20, 2012, 1:26 a.m. UTC | #12

On Tue, 2012-11-20 at 02:04 +0100, Rafael J. Wysocki wrote:
> On Monday, November 19, 2012 10:26:43 AM Toshi Kani wrote:
> > On Mon, 2012-11-19 at 18:35 +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 19, 2012 09:16:52 AM Toshi Kani wrote:
> > > > On Mon, 2012-11-19 at 08:06 -0800, Joe Perches wrote:
> > > > > On Mon, 2012-11-19 at 08:51 -0700, Toshi Kani wrote:
> > > > > > On Mon, 2012-11-19 at 00:56 -0800, Joe Perches wrote:
> > > > > > > is acpi_bus.h really the right file for these prototypes?
> > > > > > This interface is limited for ACPI, so it should be declared in a header
> > > > > > file under include/acpi.  Among the files in this directory, acpi_bus.h
> > > > > > seems to be a good fit as it declares the interfaces provided by ACPI
> > > > > > core.
> > > > > 
> > > > > I'd've put them in acpi.h or maybe created acpi_utils.h 
> > > > 
> > > > Hi Joe,
> > > > 
> > > > We cannot use acpi.h since it is an ACPICA header.
> > > 
> > > Which acpi.h are you referring to?
> > 
> > 
> > I was referring "include/acpi/acpi.h".
> 
> There is another acpi.h in include/linux that can be used for this purpose.

I see.  I agree that include/linux/acpi.h can be used.  I will update
the patchset to use this header.

Thanks,
-Toshi


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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>