From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A07D331E852 for ; Thu, 11 Jun 2026 14:43:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781189032; cv=none; b=tiSZ5EHokMapVb90QmNdVTXuh/T7DBimrjNMJv/MVOZMDwNyoifibnS5sHsZa1A6Z8eAwleWcAUzOQ6hB1ITmhbIZZ59t+wE7bmwGwwUG0E54e3yuLWO3m1JJRw2Ak+txXSpE9exe65kr2oRKILlvCkFkywfSjvDih4bs/uJfqA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781189032; c=relaxed/simple; bh=Ac8vDMPXk9UHAT0ZgwAzwtqG8oUOgYTEx0yej72AlHg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=JTE+HgyXpdppSXpZImUp3Zxx1+/KzSH9Y1Le15mv5peoZEKYc9zCxKhRcoKlRZcUV3eHIU4t6G7z92kTnzCDE2zRYO8hL9ImjskOXBhgKX3aNWNPJzR9n9eE6jCGqAux1izYv5ljy1ycy21LYys4bHqjilz4VC4TqiW9HIVRxVM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=VTKipLfd; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VTKipLfd" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A2E081F00893; Thu, 11 Jun 2026 14:43:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781189031; bh=iAWMmLVXf5klGQU+WpMZkRwAhqjZYQdX7DyoZmc5SG4=; h=From:To:Cc:Subject:In-Reply-To:References:Date; b=VTKipLfd7f0Scy1+scgFSuCCKDIwmwWGcaROHpujdrHUfc34wOm4GvookvjwxJGME VRCciAqjJpaPWtAhz9gj8PE0GBn2VLc/6sLq7+wt7ROjqgYKWm7CG+Mo4V98R0fIR8 Yu/HFlQ6SWgdb2CC7qOaQJJe1qsI7N0EE2IrsOWh/UkTxDct5a9rc8LU9dnlE2vqU+ 58U96ooPVGGrvBI0Zfwci3SneJgDni8rFA8HCGGqmEwmi0z/qstK+wuE88q4M8ug17 DXyyWMManbFDNlu69qrE0GI104Q3srBFiUU0gywfgmh7SDXUbPOC9WAw8/5QqKaqO8 h3NEuisq/a3kQ== From: Thomas Gleixner To: Bhargav Joshi , Tony Lindgren , Jason Cooper , Marc Zyngier Cc: linux-kernel@vger.kernel.org, goledhruva@gmail.com, m-chawdhry@ti.com, daniel.baluta@gmail.com, simona.toaca@nxp.com, j.bhargav.u@gmail.com Subject: Re: [PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq In-Reply-To: <20260610-irq-spinlock-fix-v2-1-a6824a74a8dd@gmail.com> References: <20260610-irq-spinlock-fix-v2-1-a6824a74a8dd@gmail.com> Date: Thu, 11 Jun 2026 16:43:48 +0200 Message-ID: <87ecid2l3v.ffs@fw13> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain 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 > --- > 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@smtp.kernel.org/ 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@gmail.com > --- > 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.