2008-02-05 07:30:48

by Len Brown

[permalink] [raw]
Subject: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

From: Len Brown <[email protected]>

Here is the ACPI GPE statistics patch, forward ported to grok 2.6.25's kobject changes.
(Greg, let me know if I was able to unleash the inner beauty of the kobj interface)
(yes, I know you don't like sysfs files with more than 1 value,
but I violate that rule for only one of the 33 files:-)

thanks,
-Len
--

# ls /sys/firmware/acpi/interrupts/
gpe00 gpe02 gpe04 gpe06 gpe08 gpe0A gpe0C gpe0E gpe10 gpe12 gpe14 gpe16 gpe18 gpe1A gpe1C gpe1E summary
gpe01 gpe03 gpe05 gpe07 gpe09 gpe0B gpe0D gpe0F gpe11 gpe13 gpe15 gpe17 gpe19 gpe1B gpe1D gpe1F

# cat /sys/firmware/acpi/interrupts/summary
pm_timer 0
glbl_lock 0
power_btn 0
sleep_btn 0
rtc 0
gpe00 0
gpe01 0
gpe02 0
gpe03 0
gpe04 0
gpe05 0
gpe06 0
gpe07 0
gpe08 0
gpe09 2
gpe0A 0
gpe0B 0
gpe0C 0
gpe0D 0
gpe0E 0
gpe0F 0
gpe10 0
gpe11 60
gpe12 0
gpe13 0
gpe14 0
gpe15 0
gpe16 0
gpe17 0
gpe18 0
gpe19 1
gpe1A 0
gpe1B 0
gpe1C 0
gpe1D 0
gpe1E 0
gpe1F 0
gpe_hi 0
gpe_total 63
acpi_irq 63

Inspired-by-original-patch-by: Luming Yu <[email protected]>
Signed-off-by: Len Brown <[email protected]>
---
drivers/acpi/events/evevent.c | 2 +-
drivers/acpi/events/evgpe.c | 2 +
drivers/acpi/osl.c | 12 ++-
drivers/acpi/system.c | 218 +++++++++++++++++++++++++++++++++++++
drivers/acpi/utilities/utglobal.c | 2 +
include/acpi/acglobal.h | 1 +
include/acpi/aclocal.h | 1 +
include/acpi/acpiosxf.h | 2 +
include/linux/acpi.h | 2 +
9 files changed, 240 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/events/evevent.c b/drivers/acpi/events/evevent.c
index e412878..18514bf 100644
--- a/drivers/acpi/events/evevent.c
+++ b/drivers/acpi/events/evevent.c
@@ -259,7 +259,7 @@ u32 acpi_ev_fixed_event_detect(void)
enable_bit_mask)) {

/* Found an active (signalled) event */
-
+ acpi_fixed_event_count[i]++;
int_status |= acpi_ev_fixed_event_dispatch((u32) i);
}
}
diff --git a/drivers/acpi/events/evgpe.c b/drivers/acpi/events/evgpe.c
index e22f4a9..515128f 100644
--- a/drivers/acpi/events/evgpe.c
+++ b/drivers/acpi/events/evgpe.c
@@ -620,6 +620,8 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)

acpi_gpe_count++;

+ acpi_os_gpe_count(gpe_number);
+
/*
* If edge-triggered, clear the GPE status bit now. Note that
* level-triggered events are cleared after the GPE is serviced.
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e53fb51..1087efe 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -332,7 +332,15 @@ acpi_os_table_override(struct acpi_table_header * existing_table,

static irqreturn_t acpi_irq(int irq, void *dev_id)
{
- return (*acpi_irq_handler) (acpi_irq_context) ? IRQ_HANDLED : IRQ_NONE;
+ u32 handled;
+
+ handled = (*acpi_irq_handler) (acpi_irq_context);
+
+ if (handled) {
+ acpi_irq_handled++;
+ return IRQ_HANDLED;
+ } else
+ return IRQ_NONE;
}

acpi_status
@@ -341,6 +349,8 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
{
unsigned int irq;

+ acpi_irq_stats_init();
+
/*
* Ignore the GSI from the core, and use the value in our copy of the
* FADT. It may not be the same if an interrupt source override exists
diff --git a/drivers/acpi/system.c b/drivers/acpi/system.c
index 5ffe0ea..538d154 100644
--- a/drivers/acpi/system.c
+++ b/drivers/acpi/system.c
@@ -40,6 +40,8 @@ ACPI_MODULE_NAME("system");
#define ACPI_SYSTEM_CLASS "system"
#define ACPI_SYSTEM_DEVICE_NAME "System"

+u32 acpi_irq_handled;
+
/*
* Make ACPICA version work as module param
*/
@@ -166,6 +168,222 @@ static int acpi_system_sysfs_init(void)
return 0;
}

+/*
+ * ACPI IRQ counters
+ *
+ * /sys/firmware/acpi/interrupts/
+ * summary -- IRQ, Fixed Event, and GPE counters
+ * gpeXX -- broken-out counters for up to 32 GPEs
+ */
+
+static u32 *acpi_gpe_counters;
+static u32 gpe_counter_high; /* bucket for GPE's >= 32 */
+static u32 number_of_gpes;
+static struct attribute **all_attrs;
+
+static struct attribute_group interrupt_stats_attr_group = {
+ .name = "interrupts",
+};
+static struct kobj_attribute *gpe_attrs;
+
+static int count_num_gpes(void)
+{
+ int count = 0;
+ struct acpi_gpe_xrupt_info *gpe_xrupt_info;
+ struct acpi_gpe_block_info *gpe_block;
+ acpi_cpu_flags flags;
+
+ flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+ gpe_xrupt_info = acpi_gbl_gpe_xrupt_list_head;
+ while (gpe_xrupt_info) {
+ gpe_block = gpe_xrupt_info->gpe_block_list_head;
+ while (gpe_block) {
+ count += gpe_block->register_count *
+ ACPI_GPE_REGISTER_WIDTH;
+ gpe_block = gpe_block->next;
+ }
+ gpe_xrupt_info = gpe_xrupt_info->next;
+ }
+ acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+
+ return count;
+}
+
+static void delete_gpe_attr_array(void)
+{
+
+ u32 *tmp = acpi_gpe_counters;
+
+ acpi_gpe_counters = NULL;
+ kfree(tmp);
+
+ if (gpe_attrs) {
+ int i;
+
+ for (i = 0; i < number_of_gpes; i++)
+ kfree(gpe_attrs[i].attr.name);
+
+ kfree(gpe_attrs);
+ }
+ kfree(all_attrs);
+
+ return;
+}
+
+static ssize_t interrupt_summary_show(struct kobject *kobj,
+ struct kobj_attribute *attribute, char *buf)
+{
+ int i;
+ ssize_t count = 0;
+
+ /*
+ * Fixed event counters
+ */
+ count += sprintf(buf + count, "pm_timer %4d\n",
+ acpi_fixed_event_count[ACPI_EVENT_PMTIMER]);
+
+ count += sprintf(buf + count, "glbl_lock %4d\n",
+ acpi_fixed_event_count[ACPI_EVENT_GLOBAL]);
+
+ count += sprintf(buf + count, "power_btn %4d\n",
+ acpi_fixed_event_count[ACPI_EVENT_POWER_BUTTON]);
+
+ count += sprintf(buf + count, "sleep_btn %4d\n",
+ acpi_fixed_event_count[ACPI_EVENT_SLEEP_BUTTON]);
+
+ count += sprintf(buf + count, "rtc %4d\n",
+ acpi_fixed_event_count[ACPI_EVENT_RTC]);
+
+ /*
+ * General Purpose Events
+ */
+ for (i = 0; i < number_of_gpes; i++) {
+ count += sprintf(buf + count, "gpe%02X %4d\n",
+ i, acpi_gpe_counters[i]);
+ }
+
+ count += sprintf(buf + count, "gpe_hi %4d\n",
+ gpe_counter_high);
+
+ count += sprintf(buf + count, "gpe_total %4d\n",
+ acpi_gpe_count);
+
+ count += sprintf(buf + count, "acpi_irq %4d\n",
+ acpi_irq_handled);
+
+ return count;
+}
+
+/*
+ * write anything to summary file: resets all counters
+ */
+static ssize_t interrupt_summary_reset(struct kobject *kobj,
+ struct kobj_attribute *attribute, const char *buf, size_t count)
+{
+ int i;
+
+ for (i = 0; i < ACPI_NUM_FIXED_EVENTS; ++i)
+ acpi_fixed_event_count[i] = 0;
+
+ for (i = 0; i < number_of_gpes; ++i)
+ acpi_gpe_counters[i] = 0;
+
+ acpi_gpe_count = 0;
+ gpe_counter_high = 0;
+ acpi_irq_handled = 0;
+
+ return count;
+}
+void acpi_os_gpe_count(u32 gpe_number)
+{
+
+ if (!acpi_gpe_counters)
+ return;
+
+ if (gpe_number < number_of_gpes)
+ acpi_gpe_counters[gpe_number]++;
+ else
+ gpe_counter_high++;
+
+ return;
+}
+
+static struct kobj_attribute summary_attribute =
+ __ATTR(summary, 0644, interrupt_summary_show, interrupt_summary_reset);
+
+
+static ssize_t gpe_count_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", acpi_gpe_counters[attr - gpe_attrs]);
+}
+
+static ssize_t gpe_count_reset(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t size)
+{
+ acpi_gpe_counters[attr - gpe_attrs] = strtoul(buf, NULL, 0);
+
+ return size;
+}
+
+void acpi_irq_stats_init(void)
+{
+ int i;
+
+ number_of_gpes = count_num_gpes();
+
+ all_attrs = kzalloc(sizeof(struct attribute *) * (number_of_gpes + 2),
+ GFP_KERNEL);
+ if (all_attrs == NULL)
+ return;
+
+ acpi_gpe_counters = kzalloc(sizeof(u32) * (number_of_gpes), GFP_KERNEL);
+ if (acpi_gpe_counters == NULL)
+ goto fail;
+
+ gpe_attrs = kzalloc(sizeof(struct kobj_attribute) * (number_of_gpes),
+ GFP_KERNEL);
+ if (gpe_attrs == NULL)
+ goto fail;
+
+ for (i = 0; i < number_of_gpes; ++i) {
+ char buffer[10];
+ char *name;
+
+ sprintf(buffer, "gpe%02X", i);
+ name = kzalloc(strlen(buffer) + 1, GFP_KERNEL);
+ if (name == NULL)
+ goto fail;
+ strncpy(name, buffer, strlen(buffer) + 1);
+
+ gpe_attrs[i].attr.name = name;
+ gpe_attrs[i].attr.mode = 0644;
+ gpe_attrs[i].show = gpe_count_show;
+ gpe_attrs[i].store = gpe_count_reset;
+
+ all_attrs[i] = &gpe_attrs[i].attr;
+ }
+ all_attrs[number_of_gpes] = &summary_attribute.attr;
+
+ interrupt_stats_attr_group.attrs = all_attrs;
+ sysfs_create_group(acpi_kobj, &interrupt_stats_attr_group);
+ return;
+
+fail:
+ delete_gpe_attr_array();
+ return;
+}
+
+static void __exit interrupt_stats_exit(void)
+{
+ sysfs_remove_group(acpi_kobj, &interrupt_stats_attr_group);
+
+ delete_gpe_attr_array();
+
+ return;
+}
+
/* --------------------------------------------------------------------------
FS Interface (/proc)
-------------------------------------------------------------------------- */
diff --git a/drivers/acpi/utilities/utglobal.c b/drivers/acpi/utilities/utglobal.c
index 93ea829..9cc1c3a 100644
--- a/drivers/acpi/utilities/utglobal.c
+++ b/drivers/acpi/utilities/utglobal.c
@@ -672,6 +672,8 @@ void acpi_ut_init_globals(void)
/* GPE support */

acpi_gpe_count = 0;
+ for (i = 0; i < ACPI_NUM_FIXED_EVENTS; i++)
+ acpi_fixed_event_count[i] = 0;
acpi_gbl_gpe_xrupt_list_head = NULL;
acpi_gbl_gpe_fadt_blocks[0] = NULL;
acpi_gbl_gpe_fadt_blocks[1] = NULL;
diff --git a/include/acpi/acglobal.h b/include/acpi/acglobal.h
index 347a911..7b3a2ba 100644
--- a/include/acpi/acglobal.h
+++ b/include/acpi/acglobal.h
@@ -120,6 +120,7 @@ extern u32 acpi_gbl_nesting_level;
/* Event counters */

ACPI_EXTERN u32 acpi_gpe_count;
+ACPI_EXTERN u32 acpi_fixed_event_count[ACPI_NUM_FIXED_EVENTS];

/* Support for dynamic control method tracing mechanism */

diff --git a/include/acpi/aclocal.h b/include/acpi/aclocal.h
index 202cd42..cabdf12 100644
--- a/include/acpi/aclocal.h
+++ b/include/acpi/aclocal.h
@@ -368,6 +368,7 @@ struct acpi_gpe_event_info {
struct acpi_gpe_register_info *register_info; /* Backpointer to register info */
u8 flags; /* Misc info about this GPE */
u8 gpe_number; /* This GPE */
+ u32 count;
};

/* Information about a GPE register pair, one per each status/enable pair in an array */
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index ca882b8..4a64da5 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -181,6 +181,8 @@ acpi_os_install_interrupt_handler(u32 gsi,
acpi_status
acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler service_routine);

+void acpi_os_gpe_count(u32 gpe_number);
+
/*
* Threads and Scheduling
*/
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 63f2e6e..cb911f3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -115,7 +115,9 @@ int acpi_unmap_lsapic(int cpu);

int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base);
int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base);
+void acpi_irq_stats_init(void);

+extern u32 acpi_irq_handled;
extern int acpi_mp_config;

extern struct acpi_mcfg_allocation *pci_mmcfg_config;
--
1.5.4.18.gd0b8


2008-02-05 22:20:28

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tue, Feb 05, 2008 at 02:30:10AM -0500, Len Brown wrote:
> # cat /sys/firmware/acpi/interrupts/summary
> pm_timer 0
> glbl_lock 0
> power_btn 0
> sleep_btn 0
> rtc 0
> gpe00 0
> gpe01 0
> gpe02 0
> gpe03 0
> gpe04 0
> gpe05 0
> gpe06 0
> gpe07 0
> gpe08 0
> gpe09 2
> gpe0A 0
> gpe0B 0
> gpe0C 0
> gpe0D 0
> gpe0E 0
> gpe0F 0
> gpe10 0
> gpe11 60
> gpe12 0
> gpe13 0
> gpe14 0
> gpe15 0
> gpe16 0
> gpe17 0
> gpe18 0
> gpe19 1
> gpe1A 0
> gpe1B 0
> gpe1C 0
> gpe1D 0
> gpe1E 0
> gpe1F 0
> gpe_hi 0
> gpe_total 63
> acpi_irq 63

Eeek! Why? What's wrong with individual files here? What's to ensure
that you aren't going to overflow your buffer? There's a reason we
don't have the seq file interface for sysfs, to prevent this very kind
of thing...


> + /*
> + * General Purpose Events
> + */
> + for (i = 0; i < number_of_gpes; i++) {
> + count += sprintf(buf + count, "gpe%02X %4d\n",
> + i, acpi_gpe_counters[i]);
> + }

Nope, no error checking, fun chance of blowing the memory buffer :(

Please change the interface to not do this.

Oh, and for every new sysfs file, you need a Documentation/ABI/
addition, please also add that.

thanks,

greg k-h

2008-02-05 23:12:42

by Len Brown

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tuesday 05 February 2008 17:18, Greg KH wrote:
> On Tue, Feb 05, 2008 at 02:30:10AM -0500, Len Brown wrote:
> > # cat /sys/firmware/acpi/interrupts/summary
> > pm_timer 0
> > glbl_lock 0
> > power_btn 0
> > sleep_btn 0
> > rtc 0
> > gpe00 0
...
> > gpe1F 0
> > gpe_hi 0
> > gpe_total 63
> > acpi_irq 63
>
> Eeek! Why? What's wrong with individual files here?

My expectation is that this is a shell interface for debugging,
not an API for programs. ala /proc/interrupts.

if we have 40 individual files, each with a number in it,
it is less convenient to cat the file and paste the results
into an email or bug report and have the receiver easily
see what what count goes with what file -- or is there
a version of cat that prints the file name before
the contents of each file?

I've seen
more * | cat
but one has to wonder why an interface would be built
to make something so simple not be simple.

> What's to ensure
> that you aren't going to overflow your buffer?

Good question. What's to ensure we don't overflow it
when I print any other random string?
Who allocates it and how big is it?

> There's a reason we
> don't have the seq file interface for sysfs, to prevent this very kind
> of thing...

If sysfs can't handle it, I suppose I could instead
print the needed information to the console....

> > + /*
> > + * General Purpose Events
> > + */
> > + for (i = 0; i < number_of_gpes; i++) {
> > + count += sprintf(buf + count, "gpe%02X %4d\n",
> > + i, acpi_gpe_counters[i]);
> > + }
>
> Nope, no error checking, fun chance of blowing the memory buffer :(

you're right, there is rumour of a future machine
with more than 32 GPEs...

> Please change the interface to not do this.
>
> Oh, and for every new sysfs file, you need a Documentation/ABI/
> addition, please also add that.

ok.

thanks,
-Len

2008-02-05 23:23:16

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tue, Feb 05, 2008 at 06:12:09PM -0500, Len Brown wrote:
> On Tuesday 05 February 2008 17:18, Greg KH wrote:
> > On Tue, Feb 05, 2008 at 02:30:10AM -0500, Len Brown wrote:
> > > # cat /sys/firmware/acpi/interrupts/summary
> > > pm_timer 0
> > > glbl_lock 0
> > > power_btn 0
> > > sleep_btn 0
> > > rtc 0
> > > gpe00 0
> ...
> > > gpe1F 0
> > > gpe_hi 0
> > > gpe_total 63
> > > acpi_irq 63
> >
> > Eeek! Why? What's wrong with individual files here?
>
> My expectation is that this is a shell interface for debugging,
> not an API for programs. ala /proc/interrupts.

Great, then use debugfs for it. Please, don't put debug stuff like this
in sysfs, that's not what it is there for. You can do whatever you want
in debugfs :)

> if we have 40 individual files, each with a number in it,
> it is less convenient to cat the file and paste the results
> into an email or bug report and have the receiver easily
> see what what count goes with what file -- or is there
> a version of cat that prints the file name before
> the contents of each file?
>
> I've seen
> more * | cat
> but one has to wonder why an interface would be built
> to make something so simple not be simple.

Use debugfs please.

> > What's to ensure
> > that you aren't going to overflow your buffer?
>
> Good question. What's to ensure we don't overflow it
> when I print any other random string?
> Who allocates it and how big is it?

The core does, and if you are putting something too big in it, you are
using the sysfs interface wrong :)

Simple, one value per file is what sysfs is for. Use debugfs for
debugging stuff.

thanks,

greg k-h

2008-02-06 00:43:38

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tuesday 05 February 2008 04:12:09 pm Len Brown wrote:
> is there
> a version of cat that prints the file name before
> the contents of each file?

I use "grep . *" for this sort of thing.

2008-02-06 01:53:44

by Len Brown

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tuesday 05 February 2008 19:44, Bjorn Helgaas wrote:
> On Tuesday 05 February 2008 04:12:09 pm Len Brown wrote:
> > is there
> > a version of cat that prints the file name before
> > the contents of each file?
>
> I use "grep . *" for this sort of thing.

good tip, bjorn, thanks!

/sys/devices/system/cpu/cpu0/cpufreq # grep . *
affected_cpus:0
cpuinfo_cur_freq:1596000
cpuinfo_max_freq:2660000
cpuinfo_min_freq:1596000
scaling_available_frequencies:2660000 2128000 1596000
scaling_available_governors:conservative ondemand userspace powersave performance
scaling_cur_freq:1596000
scaling_driver:centrino
scaling_governor:ondemand
scaling_max_freq:2660000
scaling_min_freq:1596000

2008-02-06 01:59:22

by Len Brown

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tuesday 05 February 2008 18:18, Greg KH wrote:
> On Tue, Feb 05, 2008 at 06:12:09PM -0500, Len Brown wrote:
> > On Tuesday 05 February 2008 17:18, Greg KH wrote:
> > > On Tue, Feb 05, 2008 at 02:30:10AM -0500, Len Brown wrote:
> > > > # cat /sys/firmware/acpi/interrupts/summary
> > > > pm_timer 0
> > > > glbl_lock 0
> > > > power_btn 0
> > > > sleep_btn 0
> > > > rtc 0
> > > > gpe00 0
> > ...
> > > > gpe1F 0
> > > > gpe_hi 0
> > > > gpe_total 63
> > > > acpi_irq 63
> > >
> > > Eeek! Why? What's wrong with individual files here?
> >
> > My expectation is that this is a shell interface for debugging,
> > not an API for programs. ala /proc/interrupts.
>
> Great, then use debugfs for it. Please, don't put debug stuff like this
> in sysfs, that's not what it is there for. You can do whatever you want
> in debugfs :)

Can you point to a model of good behaviour that I can copy?

note that I want this information to be available on every system,
just like /proc/interrupts is.

/proc/ has seqfile support, is there a reason I shouldn't use it?
I'd banned additional files from /proc/acpi for a long time
since the directory layout was ill-conceived. But maybe I
should re-consider the headlong rush to use sysfs?

thanks,
-Len

2008-02-06 02:45:51

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Tue, Feb 05, 2008 at 08:58:50PM -0500, Len Brown wrote:
> On Tuesday 05 February 2008 18:18, Greg KH wrote:
> > On Tue, Feb 05, 2008 at 06:12:09PM -0500, Len Brown wrote:
> > > On Tuesday 05 February 2008 17:18, Greg KH wrote:
> > > > On Tue, Feb 05, 2008 at 02:30:10AM -0500, Len Brown wrote:
> > > > > # cat /sys/firmware/acpi/interrupts/summary
> > > > > pm_timer 0
> > > > > glbl_lock 0
> > > > > power_btn 0
> > > > > sleep_btn 0
> > > > > rtc 0
> > > > > gpe00 0
> > > ...
> > > > > gpe1F 0
> > > > > gpe_hi 0
> > > > > gpe_total 63
> > > > > acpi_irq 63
> > > >
> > > > Eeek! Why? What's wrong with individual files here?
> > >
> > > My expectation is that this is a shell interface for debugging,
> > > not an API for programs. ala /proc/interrupts.
> >
> > Great, then use debugfs for it. Please, don't put debug stuff like this
> > in sysfs, that's not what it is there for. You can do whatever you want
> > in debugfs :)
>
> Can you point to a model of good behaviour that I can copy?

Any user of the debugfs api you could copy for this.

> note that I want this information to be available on every system,
> just like /proc/interrupts is.

Ah, then /proc perhaps?

> /proc/ has seqfile support, is there a reason I shouldn't use it?
> I'd banned additional files from /proc/acpi for a long time
> since the directory layout was ill-conceived. But maybe I
> should re-consider the headlong rush to use sysfs?

One of the main problems of /proc was that the files were not
documented, or that the format would change between versions, or that
they were different on different arches.

For something like this, yes, maybe you do need to use proc. It can
handle almost infinite length files, and just make sure you document it
well.

But I would just stick with debugfs, all distros enable it when
shipping, you just have to ask them to mount it by hand usually:
mount -t debugfs none /sys/kernel/debug/

Either way, I wouldn't recommend sysfs for this.

thanks,

greg k-h

2008-02-06 06:33:52

by Len Brown

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

Thanks for the reply, Greg.

If these counters are not always available by default, then
I've lost most of their value for supporting systems in the field.

Plus, I think I can live with just the individual sysfs files,
given Bjorn's "grep . *" tip.

So if I follow the rules, do you think it is okay to put
these stats in sysfs? In the hopes the answer is "yes",
I'll reply with a refreshed patch

cheers,
-Len

2008-02-06 06:40:56

by Len Brown

[permalink] [raw]
Subject: [PATCH] ACPI: create /sys/firmware/acpi/interrupts/ (v2)

From: Len Brown <[email protected]>

See Documentation/ABI/testing/sysfs-firmware-acpi

Inspired-by: Luming Yu <[email protected]>
Signed-off-by: Len Brown <[email protected]>
---
Documentation/ABI/testing/sysfs-firmware-acpi | 99 ++++++++++++
drivers/acpi/events/evevent.c | 2 +-
drivers/acpi/events/evgpe.c | 2 +-
drivers/acpi/osl.c | 12 ++-
drivers/acpi/system.c | 208 +++++++++++++++++++++++++
drivers/acpi/utilities/utglobal.c | 2 -
include/acpi/acglobal.h | 4 -
include/acpi/acpiosxf.h | 3 +
include/linux/acpi.h | 2 +
9 files changed, 325 insertions(+), 9 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-acpi

diff --git a/Documentation/ABI/testing/sysfs-firmware-acpi b/Documentation/ABI/testing/sysfs-firmware-acpi
new file mode 100644
index 0000000..9470ed9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-acpi
@@ -0,0 +1,99 @@
+What: /sys/firmware/acpi/interrupts/
+Date: February 2008
+Contact: Len Brown <[email protected]>
+Description:
+ All ACPI interrupts are handled via a single IRQ,
+ the System Control Interrupt (SCI), which appears
+ as "acpi" in /proc/interrupts.
+
+ However, one of the main functions of ACPI is to make
+ the platform understand random hardware without
+ special driver support. So while the SCI handles a few
+ well known (fixed feature) interrupts sources, such
+ as the power button, it can also handle a variable
+ number of a "General Purpose Events" (GPE).
+
+ A GPE vectors to a specified handler in AML, which
+ can do a anything the BIOS writer wants from
+ OS context. GPE 0x12, for example, would vector
+ to a level or edge handler called _L12 or _E12.
+ The handler may do its business and return.
+ Or the handler may send send a Notify event
+ to a Linux device driver registered on an ACPI device,
+ such as a battery, or a processor.
+
+ To figure out where all the SCI's are coming from,
+ /sys/firmware/acpi/interrupts contains a file listing
+ every possible source, and the count of how many
+ times it has triggered.
+
+ $ cd /sys/firmware/acpi/interrupts
+ $ grep . *
+ error:0
+ ff_gbl_lock:0
+ ff_pmtimer:0
+ ff_pwr_btn:0
+ ff_rt_clk:0
+ ff_slp_btn:0
+ gpe00:0
+ gpe01:0
+ gpe02:0
+ gpe03:0
+ gpe04:0
+ gpe05:0
+ gpe06:0
+ gpe07:0
+ gpe08:0
+ gpe09:174
+ gpe0A:0
+ gpe0B:0
+ gpe0C:0
+ gpe0D:0
+ gpe0E:0
+ gpe0F:0
+ gpe10:0
+ gpe11:60
+ gpe12:0
+ gpe13:0
+ gpe14:0
+ gpe15:0
+ gpe16:0
+ gpe17:0
+ gpe18:0
+ gpe19:7
+ gpe1A:0
+ gpe1B:0
+ gpe1C:0
+ gpe1D:0
+ gpe1E:0
+ gpe1F:0
+ gpe_all:241
+ sci:241
+
+ sci - The total number of times the ACPI SCI
+ has claimed an interrupt.
+
+ gpe_all - count of SCI caused by GPEs.
+
+ gpeXX - count for individual GPE source
+
+ ff_gbl_lock - Global Lock
+
+ ff_pmtimer - PM Timer
+
+ ff_pwr_btn - Power Button
+
+ ff_rt_clk - Real Time Clock
+
+ ff_slp_btn - Sleep Button
+
+ error - an interrupt that can't be accounted for above.
+
+ Root has permission to clear any of these counters. Eg.
+ # echo 0 > gpe11
+
+ All counters can be cleared by clearing the total "sci":
+ # echo 0 > sci
+
+ None of these counters has an effect on the function
+ of the system, they are simply statistics.
diff --git a/drivers/acpi/events/evevent.c b/drivers/acpi/events/evevent.c
index e412878..3048801 100644
--- a/drivers/acpi/events/evevent.c
+++ b/drivers/acpi/events/evevent.c
@@ -259,7 +259,7 @@ u32 acpi_ev_fixed_event_detect(void)
enable_bit_mask)) {

/* Found an active (signalled) event */
-
+ acpi_os_fixed_event_count(i);
int_status |= acpi_ev_fixed_event_dispatch((u32) i);
}
}
diff --git a/drivers/acpi/events/evgpe.c b/drivers/acpi/events/evgpe.c
index e22f4a9..4bd9e22 100644
--- a/drivers/acpi/events/evgpe.c
+++ b/drivers/acpi/events/evgpe.c
@@ -618,7 +618,7 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)

ACPI_FUNCTION_TRACE(ev_gpe_dispatch);

- acpi_gpe_count++;
+ acpi_os_gpe_count(gpe_number);

/*
* If edge-triggered, clear the GPE status bit now. Note that
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e53fb51..1087efe 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -332,7 +332,15 @@ acpi_os_table_override(struct acpi_table_header * existing_table,

static irqreturn_t acpi_irq(int irq, void *dev_id)
{
- return (*acpi_irq_handler) (acpi_irq_context) ? IRQ_HANDLED : IRQ_NONE;
+ u32 handled;
+
+ handled = (*acpi_irq_handler) (acpi_irq_context);
+
+ if (handled) {
+ acpi_irq_handled++;
+ return IRQ_HANDLED;
+ } else
+ return IRQ_NONE;
}

acpi_status
@@ -341,6 +349,8 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
{
unsigned int irq;

+ acpi_irq_stats_init();
+
/*
* Ignore the GSI from the core, and use the value in our copy of the
* FADT. It may not be the same if an interrupt source override exists
diff --git a/drivers/acpi/system.c b/drivers/acpi/system.c
index 5ffe0ea..ce88171 100644
--- a/drivers/acpi/system.c
+++ b/drivers/acpi/system.c
@@ -40,6 +40,8 @@ ACPI_MODULE_NAME("system");
#define ACPI_SYSTEM_CLASS "system"
#define ACPI_SYSTEM_DEVICE_NAME "System"

+u32 acpi_irq_handled;
+
/*
* Make ACPICA version work as module param
*/
@@ -166,6 +168,212 @@ static int acpi_system_sysfs_init(void)
return 0;
}

+/*
+ * Detailed ACPI IRQ counters in /sys/firmware/acpi/interrupts/
+ * See Documentation/ABI/testing/sysfs-firmware-acpi
+ */
+
+#define COUNT_GPE 0
+#define COUNT_SCI 1 /* acpi_irq_handled */
+#define COUNT_ERROR 2 /* other */
+#define NUM_COUNTERS_EXTRA 3
+
+static u32 *all_counters;
+static u32 num_gpes;
+static u32 num_counters;
+static struct attribute **all_attrs;
+static u32 acpi_gpe_count;
+
+static struct attribute_group interrupt_stats_attr_group = {
+ .name = "interrupts",
+};
+static struct kobj_attribute *counter_attrs;
+
+static int count_num_gpes(void)
+{
+ int count = 0;
+ struct acpi_gpe_xrupt_info *gpe_xrupt_info;
+ struct acpi_gpe_block_info *gpe_block;
+ acpi_cpu_flags flags;
+
+ flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+ gpe_xrupt_info = acpi_gbl_gpe_xrupt_list_head;
+ while (gpe_xrupt_info) {
+ gpe_block = gpe_xrupt_info->gpe_block_list_head;
+ while (gpe_block) {
+ count += gpe_block->register_count *
+ ACPI_GPE_REGISTER_WIDTH;
+ gpe_block = gpe_block->next;
+ }
+ gpe_xrupt_info = gpe_xrupt_info->next;
+ }
+ acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+
+ return count;
+}
+
+static void delete_gpe_attr_array(void)
+{
+ u32 *tmp = all_counters;
+
+ all_counters = NULL;
+ kfree(tmp);
+
+ if (counter_attrs) {
+ int i;
+
+ for (i = 0; i < num_gpes; i++)
+ kfree(counter_attrs[i].attr.name);
+
+ kfree(counter_attrs);
+ }
+ kfree(all_attrs);
+
+ return;
+}
+
+void acpi_os_gpe_count(u32 gpe_number)
+{
+ acpi_gpe_count++;
+
+ if (!all_counters)
+ return;
+
+ if (gpe_number < num_gpes)
+ all_counters[gpe_number]++;
+ else
+ all_counters[num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_ERROR]++;
+
+ return;
+}
+
+void acpi_os_fixed_event_count(u32 event_number)
+{
+ if (!all_counters)
+ return;
+
+ if (event_number < ACPI_NUM_FIXED_EVENTS)
+ all_counters[num_gpes + event_number]++;
+ else
+ all_counters[num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_ERROR]++;
+
+ return;
+}
+
+static ssize_t counter_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ all_counters[num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_SCI] =
+ acpi_irq_handled;
+ all_counters[num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_GPE] =
+ acpi_gpe_count;
+
+ return sprintf(buf, "%d\n", all_counters[attr - counter_attrs]);
+}
+
+/*
+ * counter_set() sets the specified counter.
+ * setting the total "sci" file to any value clears all counters.
+ */
+static ssize_t counter_set(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t size)
+{
+ int index = attr - counter_attrs;
+
+ if (index == num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_SCI) {
+ int i;
+ for (i = 0; i < num_counters; ++i)
+ all_counters[i] = 0;
+ acpi_gpe_count = 0;
+ acpi_irq_handled = 0;
+
+ } else
+ all_counters[index] = strtoul(buf, NULL, 0);
+
+ return size;
+}
+
+void acpi_irq_stats_init(void)
+{
+ int i;
+
+ if (all_counters)
+ return;
+
+ num_gpes = count_num_gpes();
+ num_counters = num_gpes + ACPI_NUM_FIXED_EVENTS + NUM_COUNTERS_EXTRA;
+
+ all_attrs = kzalloc(sizeof(struct attribute *) * (num_counters + 1),
+ GFP_KERNEL);
+ if (all_attrs == NULL)
+ return;
+
+ all_counters = kzalloc(sizeof(u32) * (num_counters), GFP_KERNEL);
+ if (all_counters == NULL)
+ goto fail;
+
+ counter_attrs = kzalloc(sizeof(struct kobj_attribute) * (num_counters),
+ GFP_KERNEL);
+ if (counter_attrs == NULL)
+ goto fail;
+
+ for (i = 0; i < num_counters; ++i) {
+ char buffer[10];
+ char *name;
+
+ if (i < num_gpes)
+ sprintf(buffer, "gpe%02X", i);
+ else if (i == num_gpes + ACPI_EVENT_PMTIMER)
+ sprintf(buffer, "ff_pmtimer");
+ else if (i == num_gpes + ACPI_EVENT_GLOBAL)
+ sprintf(buffer, "ff_gbl_lock");
+ else if (i == num_gpes + ACPI_EVENT_POWER_BUTTON)
+ sprintf(buffer, "ff_pwr_btn");
+ else if (i == num_gpes + ACPI_EVENT_SLEEP_BUTTON)
+ sprintf(buffer, "ff_slp_btn");
+ else if (i == num_gpes + ACPI_EVENT_RTC)
+ sprintf(buffer, "ff_rt_clk");
+ else if (i == num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_GPE)
+ sprintf(buffer, "gpe_all");
+ else if (i == num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_SCI)
+ sprintf(buffer, "sci");
+ else if (i == num_gpes + ACPI_NUM_FIXED_EVENTS + COUNT_ERROR)
+ sprintf(buffer, "error");
+ else
+ sprintf(buffer, "bug%02X", i);
+
+ name = kzalloc(strlen(buffer) + 1, GFP_KERNEL);
+ if (name == NULL)
+ goto fail;
+ strncpy(name, buffer, strlen(buffer) + 1);
+
+ counter_attrs[i].attr.name = name;
+ counter_attrs[i].attr.mode = 0644;
+ counter_attrs[i].show = counter_show;
+ counter_attrs[i].store = counter_set;
+
+ all_attrs[i] = &counter_attrs[i].attr;
+ }
+
+ interrupt_stats_attr_group.attrs = all_attrs;
+ sysfs_create_group(acpi_kobj, &interrupt_stats_attr_group);
+ return;
+
+fail:
+ delete_gpe_attr_array();
+ return;
+}
+
+static void __exit interrupt_stats_exit(void)
+{
+ sysfs_remove_group(acpi_kobj, &interrupt_stats_attr_group);
+
+ delete_gpe_attr_array();
+
+ return;
+}
+
/* --------------------------------------------------------------------------
FS Interface (/proc)
-------------------------------------------------------------------------- */
diff --git a/drivers/acpi/utilities/utglobal.c b/drivers/acpi/utilities/utglobal.c
index 93ea829..630c9a2 100644
--- a/drivers/acpi/utilities/utglobal.c
+++ b/drivers/acpi/utilities/utglobal.c
@@ -671,7 +671,6 @@ void acpi_ut_init_globals(void)

/* GPE support */

- acpi_gpe_count = 0;
acpi_gbl_gpe_xrupt_list_head = NULL;
acpi_gbl_gpe_fadt_blocks[0] = NULL;
acpi_gbl_gpe_fadt_blocks[1] = NULL;
@@ -735,4 +734,3 @@ void acpi_ut_init_globals(void)

ACPI_EXPORT_SYMBOL(acpi_dbg_level)
ACPI_EXPORT_SYMBOL(acpi_dbg_layer)
- ACPI_EXPORT_SYMBOL(acpi_gpe_count)
diff --git a/include/acpi/acglobal.h b/include/acpi/acglobal.h
index 347a911..47a1fd8 100644
--- a/include/acpi/acglobal.h
+++ b/include/acpi/acglobal.h
@@ -117,10 +117,6 @@ extern u32 acpi_dbg_layer;

extern u32 acpi_gbl_nesting_level;

-/* Event counters */
-
-ACPI_EXTERN u32 acpi_gpe_count;
-
/* Support for dynamic control method tracing mechanism */

ACPI_EXTERN u32 acpi_gbl_original_dbg_level;
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index ca882b8..1a16cfb 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -181,6 +181,9 @@ acpi_os_install_interrupt_handler(u32 gsi,
acpi_status
acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler service_routine);

+void acpi_os_gpe_count(u32 gpe_number);
+void acpi_os_fixed_event_count(u32 fixed_event_number);
+
/*
* Threads and Scheduling
*/
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 63f2e6e..cb911f3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -115,7 +115,9 @@ int acpi_unmap_lsapic(int cpu);

int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base);
int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base);
+void acpi_irq_stats_init(void);

+extern u32 acpi_irq_handled;
extern int acpi_mp_config;

extern struct acpi_mcfg_allocation *pci_mmcfg_config;
--
1.5.4.23.gef5b9

2008-02-06 06:45:31

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH for review] ACPI: Create /sys/firmware/acpi/interrupts/ counters

On Wed, Feb 06, 2008 at 01:33:25AM -0500, Len Brown wrote:
> Thanks for the reply, Greg.
>
> If these counters are not always available by default, then
> I've lost most of their value for supporting systems in the field.
>
> Plus, I think I can live with just the individual sysfs files,
> given Bjorn's "grep . *" tip.
>
> So if I follow the rules, do you think it is okay to put
> these stats in sysfs? In the hopes the answer is "yes",
> I'll reply with a refreshed patch

Sure, one value per file is fine. I'll be glad to review a patch like
that, with a Documentation/ABI/ entry :)

thanks,

greg k-h

2008-02-07 00:41:49

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] ACPI: create /sys/firmware/acpi/interrupts/ (v2)

On Wed, Feb 06, 2008 at 01:40:26AM -0500, Len Brown wrote:
> From: Len Brown <[email protected]>
>
> See Documentation/ABI/testing/sysfs-firmware-acpi
>
> Inspired-by: Luming Yu <[email protected]>
> Signed-off-by: Len Brown <[email protected]>

Looks good to me, nice job. And thanks for the documentation file.

Acked-by: Greg Kroah-Hartman <[email protected]>