diff mbox

[v3,1/4] ACPI: Add acpi_pr_<level>() interfaces

Message ID 1343257978-7085-2-git-send-email-toshi.kani@hp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Toshi Kani July 25, 2012, 11:12 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. 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 ACPICA.  Therefore, it is
always available unlike other kernel objects, such as device.

For example, the statement below
  acpi_pr_err(handle, "Device don't exist, dropping EJECT\n");
logs an error message like this at KERN_ERR.
  ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT

ACPI drivers can use acpi_pr_<level>() when they need to identify
a target ACPI object in their messages, such as error messages.
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 for
ACPI hotplug handlers.  ACPI drivers can continue to use dev_<level>()
when device is valid.

ACPI drivers also continue to use pr_<level>() when ACPI device
path does not have to be appended to the messages, 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 <toshi.kani@hp.com>
Tested-by: Vijay Mohan Pandarathil <vijaymohan.pandarathil@hp.com>
---
 drivers/acpi/utils.c    |   34 ++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |   31 +++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 0 deletions(-)

Comments

Bjorn Helgaas July 26, 2012, 7:22 p.m. UTC | #1
On Wed, Jul 25, 2012 at 5:12 PM, Toshi Kani <toshi.kani@hp.com> 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. 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 ACPICA.  Therefore, it is
> always available unlike other kernel objects, such as device.
>
> For example, the statement below
>   acpi_pr_err(handle, "Device don't exist, dropping EJECT\n");
> logs an error message like this at KERN_ERR.
>   ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT
>
> ACPI drivers can use acpi_pr_<level>() when they need to identify
> a target ACPI object in their messages, such as error messages.

It's definitely an improvement to have *something* that identifies a
device in these messages.  But the ACPI namespace path is not really
intended to be user-consumable, so I don't think we should expose it
indiscriminately.  I think we should be using the ACPI device name
("PNP0C02:00") whenever possible.  Given the device name, we can get
the path from the sysfs "path" file.

> 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 for
> ACPI hotplug handlers.  ACPI drivers can continue to use dev_<level>()
> when device is valid.

I'd argue that ACPI driver code should never be called unless the
device is valid, so drivers should *always* be able to use
dev_<level>.  Obviously, ACPI hotplug is currently screwed up (it's
mostly handled in drivers rather than in the ACPI core), so in some of
those hotplug paths in the drivers, we may not have a device yet.  But
those cases should be minimal.

Another possible approach to this is to add a %p extension rather than
adding acpi_printk().  Then you could do, e.g., 'printk("%pA ...\n",
handle)', and printk could interpolate the namespace path.  But I
really think there should be very few places where we need the path,
so I'm not sure it's worth it.

> ACPI drivers also continue to use pr_<level>() when ACPI device
> path does not have to be appended to the messages, 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 <toshi.kani@hp.com>
> Tested-by: Vijay Mohan Pandarathil <vijaymohan.pandarathil@hp.com>
> ---
>  drivers/acpi/utils.c    |   34 ++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 3e87c9c..ec0c6f9 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -454,3 +454,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 bde976e..1c855b8 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -85,6 +85,37 @@ 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__)
> +
> +/* REVISIT: Need to support CONFIG_DYNAMIC_DEBUG */
> +#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>
> --
> 1.7.7.6
>
--
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
Toshi Kani July 26, 2012, 8:58 p.m. UTC | #2
On Thu, 2012-07-26 at 13:22 -0600, Bjorn Helgaas wrote:
> On Wed, Jul 25, 2012 at 5:12 PM, Toshi Kani <toshi.kani@hp.com> 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. 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 ACPICA.  Therefore, it is
> > always available unlike other kernel objects, such as device.
> >
> > For example, the statement below
> >   acpi_pr_err(handle, "Device don't exist, dropping EJECT\n");
> > logs an error message like this at KERN_ERR.
> >   ACPI: \_SB_.SCK4.CPU4: Device don't exist, dropping EJECT
> >
> > ACPI drivers can use acpi_pr_<level>() when they need to identify
> > a target ACPI object in their messages, such as error messages.
> 
> It's definitely an improvement to have *something* that identifies a
> device in these messages.  But the ACPI namespace path is not really
> intended to be user-consumable, so I don't think we should expose it
> indiscriminately.  I think we should be using the ACPI device name
> ("PNP0C02:00") whenever possible.  Given the device name, we can get
> the path from the sysfs "path" file.

Hi Bjorn,

Thanks for reviewing!  Yes, ACPI device path is not good for regular
users to analyze from the info.  I also agree with you that device name
is a better choice when users always diagnose issues by themselves right
after they performed an operation.  However, there are also cases that
users ask someone to diagnose an issue remotely from the log files, and
hotplug operations are performed automatically.  In such cases, using
ACPI device name alone is problematic for hotplug operations since a
device name comes with an instance number that continues to change with
hot-add/remove operations.  Here is one example scenario.  Let's say,
user has automatic load-balancer or power-saving that can trigger
hundreds of CPU hotplug operations per a day.  This user then found that
there were multiple hotplug errors logged in the past few days, and
asked an IT guy to look at the error messages.  When this user found the
issue, all device names are gone from sysfs after repeated hotplug
operations.  This IT guy would have no idea if those errors were
happening on a particular device or not from the error messages since
their instance numbers continue to change.

> > 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 for
> > ACPI hotplug handlers.  ACPI drivers can continue to use dev_<level>()
> > when device is valid.
> 
> I'd argue that ACPI driver code should never be called unless the
> device is valid, so drivers should *always* be able to use
> dev_<level>.  Obviously, ACPI hotplug is currently screwed up (it's
> mostly handled in drivers rather than in the ACPI core), so in some of
> those hotplug paths in the drivers, we may not have a device yet.  But
> those cases should be minimal.

I think it makes sense for ACPI hotplug notify handlers to use
acpi_pr_<level>() for their error messages since ACPI device names are
static on the platform.  This info greatly helps in the scenario I
described above.  In the outside of the hotplug notify handlers, I agree
with you that dev_<level>() should be used.

> Another possible approach to this is to add a %p extension rather than
> adding acpi_printk().  Then you could do, e.g., 'printk("%pA ...\n",
> handle)', and printk could interpolate the namespace path.  But I
> really think there should be very few places where we need the path,
> so I'm not sure it's worth it.

Address of handle is not very helpful either when someone needs to
analyze from log files.

Thanks,
-Toshi


> > ACPI drivers also continue to use pr_<level>() when ACPI device
> > path does not have to be appended to the messages, 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 <toshi.kani@hp.com>
> > Tested-by: Vijay Mohan Pandarathil <vijaymohan.pandarathil@hp.com>
> > ---
> >  drivers/acpi/utils.c    |   34 ++++++++++++++++++++++++++++++++++
> >  include/acpi/acpi_bus.h |   31 +++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> > index 3e87c9c..ec0c6f9 100644
> > --- a/drivers/acpi/utils.c
> > +++ b/drivers/acpi/utils.c
> > @@ -454,3 +454,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 bde976e..1c855b8 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -85,6 +85,37 @@ 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__)
> > +
> > +/* REVISIT: Need to support CONFIG_DYNAMIC_DEBUG */
> > +#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>
> > --
> > 1.7.7.6
> >


--
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
Joe Perches July 26, 2012, 9:43 p.m. UTC | #3
On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote:
>     PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO
> 
> I fiddled with this a while ago; it would look something like this:
[]
> +static noinline_for_stack
> +char *acpi_name_string(char *buf, char *end, acpi_handle handle,
> +		       struct printf_spec spec, const char *fmt)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	u32 type = ACPI_SINGLE_NAME;
> +	char *p = buf;
> +
> +	if (fmt[0] == 'A')
> +		type = ACPI_FULL_PATHNAME;

maybe if (fmt[1] == 'f')

> @@ -982,6 +1007,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	}
>  
>  	switch (*fmt) {
> +	case 'A':
> +	case 'a':
> +		return acpi_name_string(buf, end, ptr, spec, fmt);

There are only so many letters, it might be better to
just use 'a' and another 'f' after that if necessary
for "full".

And of course that should be #ifdef'd too

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
Bjorn Helgaas July 26, 2012, 9:50 p.m. UTC | #4
On Thu, Jul 26, 2012 at 3:43 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote:
>>     PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO
>>
>> I fiddled with this a while ago; it would look something like this:
> []
>> +static noinline_for_stack
>> +char *acpi_name_string(char *buf, char *end, acpi_handle handle,
>> +                    struct printf_spec spec, const char *fmt)
>> +{
>> +     acpi_status status;
>> +     struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +     u32 type = ACPI_SINGLE_NAME;
>> +     char *p = buf;
>> +
>> +     if (fmt[0] == 'A')
>> +             type = ACPI_FULL_PATHNAME;
>
> maybe if (fmt[1] == 'f')
>
>> @@ -982,6 +1007,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>       }
>>
>>       switch (*fmt) {
>> +     case 'A':
>> +     case 'a':
>> +             return acpi_name_string(buf, end, ptr, spec, fmt);
>
> There are only so many letters, it might be better to
> just use 'a' and another 'f' after that if necessary
> for "full".
>
> And of course that should be #ifdef'd too

Yes.  I'm hesitant about this approach in general, because I don't
think printing the ACPI path is something we should be doing often.
It's not like a struct resource or a MAC address, where there are
dozens or hundreds of users.  I really think we should only print ACPI
paths in one or two places, so adding a %p extension would waste a
letter and encourage the wrong behavior.
--
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
Joe Perches July 26, 2012, 9:57 p.m. UTC | #5
On Thu, 2012-07-26 at 15:50 -0600, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2012 at 3:43 PM, Joe Perches <joe@perches.com> wrote:
> > On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote:
> >>     PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO
> >>
> >> I fiddled with this a while ago; it would look something like this:
> > []
> >> +static noinline_for_stack
> >> +char *acpi_name_string(char *buf, char *end, acpi_handle handle,
> >> +                    struct printf_spec spec, const char *fmt)
[]
> Yes.  I'm hesitant about this approach in general, because I don't
> think printing the ACPI path is something we should be doing often.
> It's not like a struct resource or a MAC address, where there are
> dozens or hundreds of users.  I really think we should only print ACPI
> paths in one or two places, so adding a %p extension would waste a
> letter and encourage the wrong behavior.

I don't much care for adding ACPI specific calls to vsprintf
as acpi is supposed to be OS generic anyway.

I don't think there's anything wrong with Toshi's approach.
Anyone that looks for speed in a logging message is looking
for an oddly fitting thing.  Tracing sure, but logging?

I also don't see anything wrong with renaming it to just
acpi_<level>, but that's a different discussion.

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
Toshi Kani July 27, 2012, 3:27 a.m. UTC | #6
On Thu, 2012-07-26 at 15:50 -0600, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2012 at 3:43 PM, Joe Perches <joe@perches.com> wrote:
> > On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote:
> >>     PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO
> >>
> >> I fiddled with this a while ago; it would look something like this:
> > []
> >> +static noinline_for_stack
> >> +char *acpi_name_string(char *buf, char *end, acpi_handle handle,
> >> +                    struct printf_spec spec, const char *fmt)
> >> +{
> >> +     acpi_status status;
> >> +     struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> +     u32 type = ACPI_SINGLE_NAME;
> >> +     char *p = buf;
> >> +
> >> +     if (fmt[0] == 'A')
> >> +             type = ACPI_FULL_PATHNAME;
> >
> > maybe if (fmt[1] == 'f')
> >
> >> @@ -982,6 +1007,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >>       }
> >>
> >>       switch (*fmt) {
> >> +     case 'A':
> >> +     case 'a':
> >> +             return acpi_name_string(buf, end, ptr, spec, fmt);
> >
> > There are only so many letters, it might be better to
> > just use 'a' and another 'f' after that if necessary
> > for "full".
> >
> > And of course that should be #ifdef'd too
> 
> Yes.  I'm hesitant about this approach in general, because I don't
> think printing the ACPI path is something we should be doing often.
> It's not like a struct resource or a MAC address, where there are
> dozens or hundreds of users.  I really think we should only print ACPI
> paths in one or two places, so adding a %p extension would waste a
> letter and encourage the wrong behavior.

That's a good point.  I agree.  So, let's continue to use
acpi_pr_<level>() for printing ACPI device path.  The use of this
interface is limited anyway. 

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
Toshi Kani July 27, 2012, 3:32 a.m. UTC | #7
On Thu, 2012-07-26 at 14:57 -0700, Joe Perches wrote:
> On Thu, 2012-07-26 at 15:50 -0600, Bjorn Helgaas wrote:
> > On Thu, Jul 26, 2012 at 3:43 PM, Joe Perches <joe@perches.com> wrote:
> > > On Thu, 2012-07-26 at 15:37 -0600, Bjorn Helgaas wrote:
> > >>     PNP0C01:00: new device for \_SB_.PCI0.ISA_.MBIO
> > >>
> > >> I fiddled with this a while ago; it would look something like this:
> > > []
> > >> +static noinline_for_stack
> > >> +char *acpi_name_string(char *buf, char *end, acpi_handle handle,
> > >> +                    struct printf_spec spec, const char *fmt)
> []
> > Yes.  I'm hesitant about this approach in general, because I don't
> > think printing the ACPI path is something we should be doing often.
> > It's not like a struct resource or a MAC address, where there are
> > dozens or hundreds of users.  I really think we should only print ACPI
> > paths in one or two places, so adding a %p extension would waste a
> > letter and encourage the wrong behavior.
> 
> I don't much care for adding ACPI specific calls to vsprintf
> as acpi is supposed to be OS generic anyway.
> 
> I don't think there's anything wrong with Toshi's approach.
> Anyone that looks for speed in a logging message is looking
> for an oddly fitting thing.  Tracing sure, but logging?

Fully agreed!  One cannot use printk in performance path.

Thanks,
-Toshi


> I also don't see anything wrong with renaming it to just
> acpi_<level>, but that's a different discussion.
> 
> cheers, Joe
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


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

Patch

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 3e87c9c..ec0c6f9 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -454,3 +454,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 bde976e..1c855b8 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -85,6 +85,37 @@  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__)
+
+/* REVISIT: Need to support CONFIG_DYNAMIC_DEBUG */
+#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>