From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1758872AbcIMOxB (ORCPT <rfc822;w@1wt.eu>);
        Tue, 13 Sep 2016 10:53:01 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:59105 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753562AbcIMOw7 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 13 Sep 2016 10:52:59 -0400
Date: Tue, 13 Sep 2016 16:50:34 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Craig Gallek <kraigatgoog@gmail.com>
cc: David Decotigny <decot@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] genirq: Machine-parsable version of
 /proc/interrupts
In-Reply-To: <1473689534-125511-1-git-send-email-kraigatgoog@gmail.com>
Message-ID: <alpine.DEB.2.20.1609131642410.6233@nanos>
References: <1473438501-22278-1-git-send-email-kraigatgoog@gmail.com> <1473689534-125511-1-git-send-email-kraigatgoog@gmail.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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