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

[Thomas Gleixner]

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