On Mon, 12 Sep 2016, Craig Gallek wrote:
> From: Craig Gallek <kr...@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
