Messages in this thread | | | Date | Tue, 13 Sep 2016 16:50:34 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v5] genirq: Machine-parsable version of /proc/interrupts |
| |
On Mon, 12 Sep 2016, Craig Gallek wrote:
> From: Craig Gallek <kraig@google.com>
> 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
| |