Re: [PATCH v5] genirq: Machine-parsable version of /proc/interrupts

From: Thomas Gleixner
Date: Tue Sep 13 2016 - 10:53:08 EST


On Mon, 12 Sep 2016, Craig Gallek wrote:

> From: Craig Gallek <kraig@xxxxxxxxxx>

> Add struct kobject to struct irq_desc to allow for easy export
> to sysfs. This allows for much simpler userspace-parsing of
> the information contained in struct irq_desc.

This lacks a rationale WHY we want to add this. Something like:

"/proc/interrupts is hard to parse by tools because <add content>."

Then add some blurb why and how a sysfs based interface solves the above
problem.

The fact that you add a kobject is an implementation detail and completely
irrelevant for the changelog. We can see that from the patch.

> Note that sysfs is not available at the time of early irq initialization.
> These interrupts are accounted for using a postcore_initcall callback.

That want's to be documented in the code .

> /*
> * Core internal functions to deal with irq descriptors
> @@ -45,6 +46,7 @@ struct pt_regs;
> * @rcu: rcu head for delayed free
> * @dir: /proc/irq/ procfs entry
> * @name: flow handler name for /proc/interrupts output
> + * @kobj: kobject used to represent this struct in sysfs
> */
> struct irq_desc {
> struct irq_common_data irq_common_data;
> @@ -92,6 +94,7 @@ struct irq_desc {
> int parent_irq;
> struct module *owner;
> const char *name;
> + struct kobject kobj;

Can you please move that into the CONFIG_SPARSE_IRQ conditional section
where we have the rcu head ?

> +
> +#ifdef CONFIG_SPARSE_IRQ
> +#ifdef CONFIG_SYSFS

We use

#if defined(A) && defined(B)

but please move it into the #ifdef SYSFS section which you add anyway.

> +static int __init irq_sysfs_init(void)
> +{
> + struct irq_desc *desc;
> + int irq;
> +
> + irq_kobj_base = kobject_create_and_add("irq", kernel_kobj);
> + if (!irq_kobj_base)
> + return -ENOMEM;

This is racy versus a concurrent interrupt setup. You need to move that
into the sparse locked section.

> +
> + irq_lock_sparse();
> + for_each_irq_desc(irq, desc)
> + irq_sysfs_add(irq, desc);
> + irq_unlock_sparse();

Thanks,

tglx