Re: [PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq

[Thomas Gleixner]

On Wed, Jun 10 2026 at 16:44, Bhargav Joshi wrote:

Subject prefix is: irqchip/crossbar: .....

allocate_git_irq is a function and wants to be denoted as such. See

  https://docs.kernel.org/process/maintainer-tip.html

> In allocate_gic_irq(), if irq_domain_alloc_irqs_parent() fails, the
> error path resets cb->irq_map[i] to IRQ_FREE. It modifies cb->irq_map[]
> without holding cb->lock. modifying without lock could cause data race.

That's not a useful explanation. You fail to tell what races against it
and what the side effects of a racy access are.

> Fix this by acquiring raw_spin_lock around cb->irq_map[] modification.
>
> Fixes: 783d31863fb8 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")
>

Pointless newline.

> Signed-off-by: Bhargav Joshi <j.bhargav.u@xxxxxxxxx>
> ---
> This bug was flagged by the Sashiko AI bot during the review process for
> the DT schema conversion of ti,irq-crossbar binding.
> https://lore.kernel.org/linux-devicetree/20260605210647.CCC881F00893@xxxxxxxxxxxxxxx/

Interesting. Because Sashiko said so, there is a bug, right?

Did you actually analyze whether there is a real bug or Sashiko just
pointing out something what looks like a bug? I don't think so otherwise
you could explain what the real problem with that unlocked access is and
ideally you could explain what the lock actually protects.

It protects nothing at all because interrupt domain allocation/free
operations related to a particular domain and the related hierarchy down
to the root domain are serialized in the core code already. So there is
_zero_ concurrency possible. The lock is a leftover from the original
code which predates hierarchical interrupt domains and modern core
locking and only provides voodoo protection.

So the real solution here is to remove the lock completely.

> ---
> Changes in v2:
> - Fixed typo in spin_unlock
> - Link to v1: https://patch.msgid.link/20260610-irq-spinlock-fix-v1-1-6f227ea9fa34@xxxxxxxxx
> ---
>  drivers/irqchip/irq-crossbar.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index cd1134101ace..3d8bb37c9141 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -100,8 +100,11 @@ static int allocate_gic_irq(struct irq_domain *domain, unsigned virq,
>  	fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>  
>  	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> -	if (err)
> +	if (err) {
> +		raw_spin_lock(&cb->lock);
>  		cb->irq_map[i] = IRQ_FREE;
> +		raw_spin_unlock(&cb->lock);

Not that it matters, but this should use guard() or scoped_guard()

> +	}
>  	else
>  		cb->write(i, hwirq);

If you add brackets to the if () part, you need to add them to the else
part too even if there is only a single code line. This

     if () {
        ....
     }
     else
        ...

is asymmetric and unreadable gunk.