Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Alexey Kardashevskiy <aik@ozlabs.ru>, linux-kernel@vger.kernel.org
Cc:     =?utf-8?Q?C=C3=A9dric?= Le Goater <clg@kaod.org>,
        Frederic Barrat <fbarrat@linux.ibm.com>,
        Michal =?utf-8?Q?Such=C3=A1nek?= <msuchanek@suse.de>,
        Oliver O'Halloran <oohall@gmail.com>,
        Marc Zyngier <maz@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
        x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
        Alexey Kardashevskiy <aik@ozlabs.ru>
Subject: Re: [PATCH kernel v4 5/8] genirq: Add free_irq hook for IRQ descriptor and use for mapping disposal
In-Reply-To: <20201124061720.86766-6-aik@ozlabs.ru>
References: <20201124061720.86766-1-aik@ozlabs.ru> <20201124061720.86766-6-aik@ozlabs.ru>
Date:   Mon, 30 Nov 2020 23:18:34 +0100
Message-ID: <877dq2ij45.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Alexey,

On Tue, Nov 24 2020 at 17:17, Alexey Kardashevskiy wrote:
> We want to make the irq_desc.kobj's release hook free associated resources
> but we do not want to pollute the irqdesc code with domains.

Can you please describe your changelog in factual ways without 'we and I
and want'? See Documentation/process/

> This adds a free_irq hook which is called when the last reference to
> the descriptor is dropped.
>
> The first user is mapped irqs. This potentially can break the existing
> users; however they seem to do the right thing and call dispose once
> per mapping.

Q: How is this supposed to work with CONFIG_SPARSE_IRQ=n?
A: Not at all.

Also 'seem to do the right thing' is from the same quality as 'should
not break stuff'. Either you have done a proper analysis or you did
not. Aside of that how is anyone supposed to decode the subtle wreckage
which is this going to cause if 'seem to do the right thing' turns out
to be wrong?

> -void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
> +static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
>  {
>  	struct irq_data *irq_data = irq_get_irq_data(irq);
>  	irq_hw_number_t hwirq;
> @@ -582,6 +582,13 @@ void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_associate_many);
>  
> +static void irq_mapped_free_desc(struct irq_desc *desc)

That function name is really misleading and badly chosen. The function
is not about freeing the irq descriptor as the name suggests. It's
called from that code in order to clean up the mapping.

> +{
> +	unsigned int virq = desc->irq_data.irq;
> +
> +	irq_domain_disassociate(desc->irq_data.domain, virq);
> +}
> +
>  /**
>   * irq_create_direct_mapping() - Allocate an irq for direct mapping
>   * @domain: domain to allocate the irq for or NULL for default domain
> @@ -638,6 +645,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  {
>  	struct device_node *of_node;
>  	int virq;
> +	struct irq_desc *desc;

This code uses reverse fir tree ordering of variables

  	struct device_node *of_node;
	struct irq_desc *desc;
  	int virq;

Why? Because it's simpler to read than the vertical camel case above.

Thanks,

        tglx