Message ID | 20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp1330579dyc; Mon, 19 Feb 2024 06:48:45 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCV0e+n3Sy3oGCVXzvnCH5dUF0yh0a9yl13BFAaLz6jMtU99z0b0p1xbVdHa8LVFZJg87EsJ2xixLLpvqjkyT+7y0XVecA== X-Google-Smtp-Source: AGHT+IEGxHXTgHyu9XLqCV2rXLA8ipvsr8wMch9JoSFSTEtadH3YFFKlCti29Louldi9z71oUtS/ X-Received: by 2002:a17:906:2c10:b0:a3e:4813:d4d9 with SMTP id e16-20020a1709062c1000b00a3e4813d4d9mr3303117ejh.51.1708354125660; Mon, 19 Feb 2024 06:48:45 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708354125; cv=pass; d=google.com; s=arc-20160816; b=WxInPTEiH8VakvxxqTBxjXRj3I2EqdLcYnjZvyXBnILpA+7/FrPNDQKosZZp2ERnNw zeYcLcShNd3OLHQCq6lOcvXsKlVDz7s7pzktvNk/nUdH+jaiy7T8NtiDBhIu61Q5fjvT ZgUrbTcSjzRRl9gwGIUbVYw6sKXiyyYRdGWa5sj9SvpDLqAtz9twPwgGF30VbekmAIkt aMu7ITebgi9gCmNuA6JxrwHq26V9gjxG40caby7gd/wnKTSl9WhqSjqjH+Os+lDcEpM3 Xh1y83XK6IdiADQQwXsUw1p9j0k+odDnPZKPV0vp7tAJHQCMkzkWeHVZ8kwy4o731RHL M79w== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:message-id:content-transfer-encoding:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:subject:date :from:dkim-signature; bh=5NUo4d1gxTmiY7z7v99UlcjZS6ZKrBDK3bbgjglglyk=; fh=RIH1/CLp0xrcSzIYpm0co9XzbXU9OUZYi7yBtTysJfQ=; b=ck2BxxA+iTcDEz6a1K2FoId0UwDURpqaKvMs5McLggzDJ2hXonwSdSxE2EXnFhizzm aCoJOHCPhsC0xZSn1zY3nqAPF0GT2iPkyl5e0y1zCMAQL3uvmieUKPss95JxkZ/wBiRb XX3DoT7WrG1izEojlb5f/DEHoCZ41VjI9GLOS0MHvdmH4J/7iGEyRwMFp9JLzcekz+Ph EU8nSdEHHzjDKPnobhWbQD/wk9YupgHkhDCxA0ZAk61nT2+Ryor5K04G5MV8hce9NQgp Yx6n01jE2Cz22GELaOqR6HOCaXtRs6dVtjYtI9zcyRSgHzL/O4xc+Yh+i7+x8623KoHr JdXA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Q2HRZbqx; arc=pass (i=1 spf=pass spfdomain=linaro.org dkim=pass dkdomain=linaro.org dmarc=pass fromdomain=linaro.org); spf=pass (google.com: domain of linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id gs30-20020a1709072d1e00b00a3e433eed92si1872641ejc.320.2024.02.19.06.48.45 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Feb 2024 06:48:45 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Q2HRZbqx; arc=pass (i=1 spf=pass spfdomain=linaro.org dkim=pass dkdomain=linaro.org dmarc=pass fromdomain=linaro.org); spf=pass (google.com: domain of linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-71504-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 345591F24EE4 for <ouuuleilei@gmail.com>; Mon, 19 Feb 2024 14:47:57 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 9C001376FE; Mon, 19 Feb 2024 14:47:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Q2HRZbqx" Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F30311EA80 for <linux-kernel@vger.kernel.org>; Mon, 19 Feb 2024 14:47:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.49 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708354061; cv=none; b=h23SwCp4KKcxU7SOdt65xDreloRJUK+AVUqj+xOnT6eR+dIHhOXEYBn108OTLYCjsU66KjxOK2BE+USQQkBIJcmZ7Y2cqV+205i766iQuZGN6pWfc9e2DPr3L7lwHVpGNgrgjRWroXoC/RPI0hjHOskPJthwOSd0T1SQM2hkvVE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708354061; c=relaxed/simple; bh=EHbRQ7dLHAog9MqSFM/E+Sdhe/5P8jIuOjtEE4j2X68=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:To:Cc; b=FVyvh2YrMZMlLkcruyddQ1DgNZ/nc4KR05KyXPywxQgKCJ3Pkg44n++pk8diRBhHijbc5g8Zhrhgz3+D2GuRsE7rcdr0+WC+l8Qmzd8+QnGAFGQBEpSO/jdVHhGkWjhNY4lm1k4wl+5VQ3BATHn3R9r5VagRFow6JsEOZRZdfTA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=Q2HRZbqx; arc=none smtp.client-ip=209.85.167.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-512b3b04995so965409e87.3 for <linux-kernel@vger.kernel.org>; Mon, 19 Feb 2024 06:47:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1708354058; x=1708958858; darn=vger.kernel.org; h=cc:to:message-id:content-transfer-encoding:mime-version:subject :date:from:from:to:cc:subject:date:message-id:reply-to; bh=5NUo4d1gxTmiY7z7v99UlcjZS6ZKrBDK3bbgjglglyk=; b=Q2HRZbqxi1I8dtM5d8BAv2sw1Q1OVcXcwMMutizeRuKueAoguhm6WIkmJqHxkzDIJO yv4EDu/qlspXK2lG5Vn8QXPsDpib1ynTROXoKSIi/st4HSvfTL7kXXSit4ayo/2JZxWF I88MZbyZOgKihO2x+481kcvG+sR6jMf9tjdcE3r4rHcfgPASxOQSRqsDH/A4+nnsC77p mtZOeiAdXjYm/N9g2+AkiVWZnapbDxny2W3xFpIgoXeBImkEnsSgPOfBB2toZZBV/dGX 72WWc6yq/G7SsSgcbcwOD4srmJJnNInDKRcmWs8od42zP8NmVkbLokGD/3rSBimOiWLC iZjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708354058; x=1708958858; h=cc:to:message-id:content-transfer-encoding:mime-version:subject :date:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5NUo4d1gxTmiY7z7v99UlcjZS6ZKrBDK3bbgjglglyk=; b=Wn82DlLuYUV7Ssr/6+QPelueJsXbnsnvLIDwmo7ucyLf9KSDsDtXRzXFNV6v6Hx5pM KijfN6CnohkigTCCQRjKa6ktk7l3FVTnA89VJJR0eTH4Ik+Zgu6AWYI9mfmsiHzUwoCX JdBT4zkFY1hq0fPasCvAo+13e62P4lMVG4DsRO45Coz089tkBX9XEpBmhj8N6HnImkRd pGu7LTtReHTHOQ+YuC72xnANIPn315YYSTswsYpfcqzbpVcBwKrC6BDufrPHguU6wwR4 gvNUIpz2mhvgxRqu07s/ywQXQR8HBO7h9tZkiLpsrKEThYabx18VbAYIpev/HHo0+RlA 6r1w== X-Forwarded-Encrypted: i=1; AJvYcCW8om5bbl7IZvWLBCNuo9J5pfe7XGxmc3nr9RpY8JQFN7QOH0sf7byhnwpnlqz6qq7PJtJ9a1x0d0zfsZwDjyJWR7gRJ0cwITynBq3x X-Gm-Message-State: AOJu0YxFHvGHeQYEWjMSD8TyH+6WNTDZuwmLCweG+cKIHUFdKvZXu8Fr KAVaZK2OuSx1EY8zOF/wEMHOwjEwLMw/nZwujvGzuU21r9osMwSR0ckWgEKbuA0= X-Received: by 2002:a05:6512:6d4:b0:511:e296:e563 with SMTP id u20-20020a05651206d400b00511e296e563mr9724280lff.2.1708354057815; Mon, 19 Feb 2024 06:47:37 -0800 (PST) Received: from umbar.lan ([192.130.178.91]) by smtp.gmail.com with ESMTPSA id v29-20020a056512049d00b005128d5d670csm924254lfq.193.2024.02.19.06.47.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Feb 2024 06:47:37 -0800 (PST) From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Date: Mon, 19 Feb 2024 16:47:37 +0200 Subject: [PATCH] irqchip/gic-v3: handle DOMAIN_BUS_ANY in gic_irq_domain_select Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org> X-B4-Tracking: v=1; b=H4sIAAhq02UC/x2MQQqAIBAAvxJ7bkHFwPpKdDBdbaEsFCKQ/p50H JiZCoUyU4Gpq5Dp5sJnaiD7DtxmUyRk3xiUUFooOWJkh4EfdBvvHv15WE5orDRarDSGMEBLr0z N+bfz8r4fqFrqTWYAAAA= To: Marc Zyngier <maz@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Anup Patel <apatel@ventanamicro.com> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> X-Mailer: b4 0.12.4 X-Developer-Signature: v=1; a=openpgp-sha256; l=1663; i=dmitry.baryshkov@linaro.org; h=from:subject:message-id; bh=EHbRQ7dLHAog9MqSFM/E+Sdhe/5P8jIuOjtEE4j2X68=; b=owEBbQGS/pANAwAKAYs8ij4CKSjVAcsmYgBl02oJZo2K7nEm2susxBahpdq/LMzQO89GN/01X IghlhaAWmWJATMEAAEKAB0WIQRMcISVXLJjVvC4lX+LPIo+Aiko1QUCZdNqCQAKCRCLPIo+Aiko 1dr2B/4iG4WVuvwqRcRvLAR7OJUV4wyiF8CtzhWVKinJ6V3wT+pna+GA8Cn3+mK3WJwaVmFedql oJKMb9ykN0akdVxAA7eZh7UWNNdx1Lwn4EnrXWEzhGDq49r+tuJANZoSXrSaPB/rl8Nw8bg2DMm dvNnIRyQr5i7MhLceVChQ30DQuVfilQBIHMeXBiRnIjFw9HxuQx7yOApeFBHMV14HsXNvgZeGLw ym0wzk/ft10ot8T0Avdnr6wxJs/adnIjDfWI5nBhoaXEuIrsLMcD2tifsXK0LsmR15cqP3zmbmI 31h4E5so8h3/+aNhdKZUPfqtMcL75NNBnSpKNvFeH/Wa++Yo X-Developer-Key: i=dmitry.baryshkov@linaro.org; a=openpgp; fpr=8F88381DD5C873E4AE487DA5199BF1243632046A X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791339135607388586 X-GMAIL-MSGID: 1791339135607388586 |
Series |
irqchip/gic-v3: handle DOMAIN_BUS_ANY in gic_irq_domain_select
|
|
Commit Message
Dmitry Baryshkov
Feb. 19, 2024, 2:47 p.m. UTC
Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
count restriction from select()") the irq_find_matching_fwspec() was
handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
the select() callback. However the callback of GICv3 (even though it got
modified to handle zero param_count) wasn't prepared to return true for
DOMAIN_BUS_ANY bus_token.
This breaks probing of any of the child IRQ domains, since
platform_irqchip_probe() uses irq_find_matching_host(par_np,
DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/irqchip/irq-gic-v3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 35a4fdde2466b9d90af297f249436a270ef9d30e
change-id: 20240219-gic-fix-child-domain-8a1840be9ff5
Best regards,
Comments
On Mon, 19 Feb 2024 14:47:37 +0000, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > count restriction from select()") the irq_find_matching_fwspec() was > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > the select() callback. However the callback of GICv3 (even though it got > modified to handle zero param_count) wasn't prepared to return true for > DOMAIN_BUS_ANY bus_token. > > This breaks probing of any of the child IRQ domains, since > platform_irqchip_probe() uses irq_find_matching_host(par_np, > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/irqchip/irq-gic-v3.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 6fb276504bcc..e9e9643c653f 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > /* Handle pure domain searches */ > if (!fwspec->param_count) > - return d->bus_token == bus_token; > + return d->bus_token == bus_token || > + bus_token == DOMAIN_BUS_ANY; > > /* If this is not DT, then we have a single domain */ > if (!is_of_node(fwspec->fwnode)) > I really dislike the look of this. If that's the case, any irqchip that has a 'select' method (such as imx-intmux) should be similarly hacked. And at this point, this should be handled by the core code. Can you try this instead? I don't have any HW that relies on behaviour, but I'd expect this to work. Thanks, M. diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index aeb41655d6de..3dd1c871e091 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, */ mutex_lock(&irq_domain_mutex); list_for_each_entry(h, &irq_domain_list, link) { - if (h->ops->select) + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) rc = h->ops->select(h, fwspec, bus_token); else if (h->ops->match) rc = h->ops->match(h, to_of_node(fwnode), bus_token);
On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 19 Feb 2024 14:47:37 +0000, > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > > count restriction from select()") the irq_find_matching_fwspec() was > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > > the select() callback. However the callback of GICv3 (even though it got > > modified to handle zero param_count) wasn't prepared to return true for > > DOMAIN_BUS_ANY bus_token. > > > > This breaks probing of any of the child IRQ domains, since > > platform_irqchip_probe() uses irq_find_matching_host(par_np, > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/irqchip/irq-gic-v3.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 6fb276504bcc..e9e9643c653f 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > > > /* Handle pure domain searches */ > > if (!fwspec->param_count) > > - return d->bus_token == bus_token; > > + return d->bus_token == bus_token || > > + bus_token == DOMAIN_BUS_ANY; > > > > /* If this is not DT, then we have a single domain */ > > if (!is_of_node(fwspec->fwnode)) > > > > I really dislike the look of this. If that's the case, any irqchip > that has a 'select' method (such as imx-intmux) should be similarly > hacked. And at this point, this should be handled by the core code. > > Can you try this instead? I don't have any HW that relies on > behaviour, but I'd expect this to work. > > Thanks, > > M. > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index aeb41655d6de..3dd1c871e091 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > */ > mutex_lock(&irq_domain_mutex); > list_for_each_entry(h, &irq_domain_list, link) { > - if (h->ops->select) > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > rc = h->ops->select(h, fwspec, bus_token); > else if (h->ops->match) > rc = h->ops->match(h, to_of_node(fwnode), bus_token); This works. But I wonder if the following change is even better. WDYT? diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index aeb41655d6de..2f0d2700709e 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -449,14 +449,17 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, */ mutex_lock(&irq_domain_mutex); list_for_each_entry(h, &irq_domain_list, link) { - if (h->ops->select) + if (fwnode != NULL && + h->fwnode == fwnode && + bus_token == DOMAIN_BUS_ANY) + rc = true; + else if (h->ops->select) rc = h->ops->select(h, fwspec, bus_token); else if (h->ops->match) rc = h->ops->match(h, to_of_node(fwnode), bus_token); else rc = ((fwnode != NULL) && (h->fwnode == fwnode) && - ((bus_token == DOMAIN_BUS_ANY) || - (h->bus_token == bus_token))); + (h->bus_token == bus_token)); if (rc) { found = h;
On Mon, 19 Feb 2024 16:21:06 +0000, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 19 Feb 2024 14:47:37 +0000, > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > > > count restriction from select()") the irq_find_matching_fwspec() was > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > > > the select() callback. However the callback of GICv3 (even though it got > > > modified to handle zero param_count) wasn't prepared to return true for > > > DOMAIN_BUS_ANY bus_token. > > > > > > This breaks probing of any of the child IRQ domains, since > > > platform_irqchip_probe() uses irq_find_matching_host(par_np, > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/irqchip/irq-gic-v3.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > index 6fb276504bcc..e9e9643c653f 100644 > > > --- a/drivers/irqchip/irq-gic-v3.c > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > > > > > /* Handle pure domain searches */ > > > if (!fwspec->param_count) > > > - return d->bus_token == bus_token; > > > + return d->bus_token == bus_token || > > > + bus_token == DOMAIN_BUS_ANY; > > > > > > /* If this is not DT, then we have a single domain */ > > > if (!is_of_node(fwspec->fwnode)) > > > > > > > I really dislike the look of this. If that's the case, any irqchip > > that has a 'select' method (such as imx-intmux) should be similarly > > hacked. And at this point, this should be handled by the core code. > > > > Can you try this instead? I don't have any HW that relies on > > behaviour, but I'd expect this to work. > > > > Thanks, > > > > M. > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > index aeb41655d6de..3dd1c871e091 100644 > > --- a/kernel/irq/irqdomain.c > > +++ b/kernel/irq/irqdomain.c > > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > */ > > mutex_lock(&irq_domain_mutex); > > list_for_each_entry(h, &irq_domain_list, link) { > > - if (h->ops->select) > > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > > rc = h->ops->select(h, fwspec, bus_token); > > else if (h->ops->match) > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > This works. But I wonder if the following change is even better. WDYT? > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index aeb41655d6de..2f0d2700709e 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -449,14 +449,17 @@ struct irq_domain > *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > */ > mutex_lock(&irq_domain_mutex); > list_for_each_entry(h, &irq_domain_list, link) { > - if (h->ops->select) > + if (fwnode != NULL && > + h->fwnode == fwnode && > + bus_token == DOMAIN_BUS_ANY) > + rc = true; > + else if (h->ops->select) > rc = h->ops->select(h, fwspec, bus_token); > else if (h->ops->match) > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > else > rc = ((fwnode != NULL) && (h->fwnode == fwnode) && > - ((bus_token == DOMAIN_BUS_ANY) || > - (h->bus_token == bus_token))); > + (h->bus_token == bus_token)); > > if (rc) { > found = h; > Can't say I like it either. It duplicates the existing check without any obvious benefit. Honestly, this code is shit enough that we should try to make it simpler, not more complex... I'd rather we keep the impact as minimal as possible, and use the upcoming weeks to weed out the effects of these changes (there is another report of some Renesas machine falling over itself here[1]). Thanks, M. [1] https://lore.kernel.org/all/170802702416.398.14922976721740218856.tip-bot2@tip-bot2
On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 19 Feb 2024 16:21:06 +0000, > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Mon, 19 Feb 2024 14:47:37 +0000, > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > > > > count restriction from select()") the irq_find_matching_fwspec() was > > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > > > > the select() callback. However the callback of GICv3 (even though it got > > > > modified to handle zero param_count) wasn't prepared to return true for > > > > DOMAIN_BUS_ANY bus_token. > > > > > > > > This breaks probing of any of the child IRQ domains, since > > > > platform_irqchip_probe() uses irq_find_matching_host(par_np, > > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > > > > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/irqchip/irq-gic-v3.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > > index 6fb276504bcc..e9e9643c653f 100644 > > > > --- a/drivers/irqchip/irq-gic-v3.c > > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > > > > > > > /* Handle pure domain searches */ > > > > if (!fwspec->param_count) > > > > - return d->bus_token == bus_token; > > > > + return d->bus_token == bus_token || > > > > + bus_token == DOMAIN_BUS_ANY; > > > > > > > > /* If this is not DT, then we have a single domain */ > > > > if (!is_of_node(fwspec->fwnode)) > > > > > > > > > > I really dislike the look of this. If that's the case, any irqchip > > > that has a 'select' method (such as imx-intmux) should be similarly > > > hacked. And at this point, this should be handled by the core code. > > > > > > Can you try this instead? I don't have any HW that relies on > > > behaviour, but I'd expect this to work. > > > > > > Thanks, > > > > > > M. > > > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > > index aeb41655d6de..3dd1c871e091 100644 > > > --- a/kernel/irq/irqdomain.c > > > +++ b/kernel/irq/irqdomain.c > > > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > > */ > > > mutex_lock(&irq_domain_mutex); > > > list_for_each_entry(h, &irq_domain_list, link) { > > > - if (h->ops->select) > > > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > > > rc = h->ops->select(h, fwspec, bus_token); > > > else if (h->ops->match) > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > > > This works. But I wonder if the following change is even better. WDYT? > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > index aeb41655d6de..2f0d2700709e 100644 > > --- a/kernel/irq/irqdomain.c > > +++ b/kernel/irq/irqdomain.c > > @@ -449,14 +449,17 @@ struct irq_domain > > *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > */ > > mutex_lock(&irq_domain_mutex); > > list_for_each_entry(h, &irq_domain_list, link) { > > - if (h->ops->select) > > + if (fwnode != NULL && > > + h->fwnode == fwnode && > > + bus_token == DOMAIN_BUS_ANY) > > + rc = true; > > + else if (h->ops->select) > > rc = h->ops->select(h, fwspec, bus_token); > > else if (h->ops->match) > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > else > > rc = ((fwnode != NULL) && (h->fwnode == fwnode) && > > - ((bus_token == DOMAIN_BUS_ANY) || > > - (h->bus_token == bus_token))); > > + (h->bus_token == bus_token)); > > > > if (rc) { > > found = h; > > > > Can't say I like it either. It duplicates the existing check without > any obvious benefit. Honestly, this code is shit enough that we should > try to make it simpler, not more complex... Only the fwnode conditions are duplicated. And it makes sense: we should check for the DOMAIN_BUS_ANY first, before going to select. I'm not sure whether at some point we'd have to add (&& bus_token != DOMAIN_BUS_ANY) to the ops->match check. > > I'd rather we keep the impact as minimal as possible, and use the > upcoming weeks to weed out the effects of these changes (there is > another report of some Renesas machine falling over itself here[1]). > > Thanks, > > M. > > [1] https://lore.kernel.org/all/170802702416.398.14922976721740218856.tip-bot2@tip-bot2 > > -- > Without deviation from the norm, progress is not possible.
On Mon, 19 Feb 2024 17:41:37 +0000, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 19 Feb 2024 16:21:06 +0000, > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Mon, 19 Feb 2024 14:47:37 +0000, > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > > > > > count restriction from select()") the irq_find_matching_fwspec() was > > > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > > > > > the select() callback. However the callback of GICv3 (even though it got > > > > > modified to handle zero param_count) wasn't prepared to return true for > > > > > DOMAIN_BUS_ANY bus_token. > > > > > > > > > > This breaks probing of any of the child IRQ domains, since > > > > > platform_irqchip_probe() uses irq_find_matching_host(par_np, > > > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > > > > > > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > > > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > drivers/irqchip/irq-gic-v3.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > > > index 6fb276504bcc..e9e9643c653f 100644 > > > > > --- a/drivers/irqchip/irq-gic-v3.c > > > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > > > > > > > > > /* Handle pure domain searches */ > > > > > if (!fwspec->param_count) > > > > > - return d->bus_token == bus_token; > > > > > + return d->bus_token == bus_token || > > > > > + bus_token == DOMAIN_BUS_ANY; > > > > > > > > > > /* If this is not DT, then we have a single domain */ > > > > > if (!is_of_node(fwspec->fwnode)) > > > > > > > > > > > > > I really dislike the look of this. If that's the case, any irqchip > > > > that has a 'select' method (such as imx-intmux) should be similarly > > > > hacked. And at this point, this should be handled by the core code. > > > > > > > > Can you try this instead? I don't have any HW that relies on > > > > behaviour, but I'd expect this to work. > > > > > > > > Thanks, > > > > > > > > M. > > > > > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > > > index aeb41655d6de..3dd1c871e091 100644 > > > > --- a/kernel/irq/irqdomain.c > > > > +++ b/kernel/irq/irqdomain.c > > > > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > > > */ > > > > mutex_lock(&irq_domain_mutex); > > > > list_for_each_entry(h, &irq_domain_list, link) { > > > > - if (h->ops->select) > > > > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > > > > rc = h->ops->select(h, fwspec, bus_token); > > > > else if (h->ops->match) > > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > > > > > This works. But I wonder if the following change is even better. WDYT? > > > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > > index aeb41655d6de..2f0d2700709e 100644 > > > --- a/kernel/irq/irqdomain.c > > > +++ b/kernel/irq/irqdomain.c > > > @@ -449,14 +449,17 @@ struct irq_domain > > > *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > > */ > > > mutex_lock(&irq_domain_mutex); > > > list_for_each_entry(h, &irq_domain_list, link) { > > > - if (h->ops->select) > > > + if (fwnode != NULL && > > > + h->fwnode == fwnode && > > > + bus_token == DOMAIN_BUS_ANY) > > > + rc = true; > > > + else if (h->ops->select) > > > rc = h->ops->select(h, fwspec, bus_token); > > > else if (h->ops->match) > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > > else > > > rc = ((fwnode != NULL) && (h->fwnode == fwnode) && > > > - ((bus_token == DOMAIN_BUS_ANY) || > > > - (h->bus_token == bus_token))); > > > + (h->bus_token == bus_token)); > > > > > > if (rc) { > > > found = h; > > > > > > > Can't say I like it either. It duplicates the existing check without > > any obvious benefit. Honestly, this code is shit enough that we should > > try to make it simpler, not more complex... > > Only the fwnode conditions are duplicated. And it makes sense: we > should check for the DOMAIN_BUS_ANY first, before going to select. I'm > not sure whether at some point we'd have to add (&& bus_token != > DOMAIN_BUS_ANY) to the ops->match check. ops->match should just *die*, and it is not going to see any sort of semantic upgrade. Ever. No new code should be added using match. And look at what my change does. It checks for DOMAIN_BUS_ANY before doing anything else, ensuring that the default clause does the job. So no, your suggestion doesn't make much sense to me. M.
On Mon, 19 Feb 2024 at 19:51, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 19 Feb 2024 17:41:37 +0000, > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Mon, 19 Feb 2024 16:21:06 +0000, > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Mon, 19 Feb 2024 14:47:37 +0000, > > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param > > > > > > count restriction from select()") the irq_find_matching_fwspec() was > > > > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of > > > > > > the select() callback. However the callback of GICv3 (even though it got > > > > > > modified to handle zero param_count) wasn't prepared to return true for > > > > > > DOMAIN_BUS_ANY bus_token. > > > > > > > > > > > > This breaks probing of any of the child IRQ domains, since > > > > > > platform_irqchip_probe() uses irq_find_matching_host(par_np, > > > > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain. > > > > > > > > > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count") > > > > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > --- > > > > > > drivers/irqchip/irq-gic-v3.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > > > > index 6fb276504bcc..e9e9643c653f 100644 > > > > > > --- a/drivers/irqchip/irq-gic-v3.c > > > > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, > > > > > > > > > > > > /* Handle pure domain searches */ > > > > > > if (!fwspec->param_count) > > > > > > - return d->bus_token == bus_token; > > > > > > + return d->bus_token == bus_token || > > > > > > + bus_token == DOMAIN_BUS_ANY; > > > > > > > > > > > > /* If this is not DT, then we have a single domain */ > > > > > > if (!is_of_node(fwspec->fwnode)) > > > > > > > > > > > > > > > > I really dislike the look of this. If that's the case, any irqchip > > > > > that has a 'select' method (such as imx-intmux) should be similarly > > > > > hacked. And at this point, this should be handled by the core code. > > > > > > > > > > Can you try this instead? I don't have any HW that relies on > > > > > behaviour, but I'd expect this to work. > > > > > > > > > > Thanks, > > > > > > > > > > M. > > > > > > > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > > > > index aeb41655d6de..3dd1c871e091 100644 > > > > > --- a/kernel/irq/irqdomain.c > > > > > +++ b/kernel/irq/irqdomain.c > > > > > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > > > > */ > > > > > mutex_lock(&irq_domain_mutex); > > > > > list_for_each_entry(h, &irq_domain_list, link) { > > > > > - if (h->ops->select) > > > > > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > > > > > rc = h->ops->select(h, fwspec, bus_token); > > > > > else if (h->ops->match) > > > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > > > > > > > This works. But I wonder if the following change is even better. WDYT? > > > > > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > > > > index aeb41655d6de..2f0d2700709e 100644 > > > > --- a/kernel/irq/irqdomain.c > > > > +++ b/kernel/irq/irqdomain.c > > > > @@ -449,14 +449,17 @@ struct irq_domain > > > > *irq_find_matching_fwspec(struct irq_fwspec *fwspec, > > > > */ > > > > mutex_lock(&irq_domain_mutex); > > > > list_for_each_entry(h, &irq_domain_list, link) { > > > > - if (h->ops->select) > > > > + if (fwnode != NULL && > > > > + h->fwnode == fwnode && > > > > + bus_token == DOMAIN_BUS_ANY) > > > > + rc = true; > > > > + else if (h->ops->select) > > > > rc = h->ops->select(h, fwspec, bus_token); > > > > else if (h->ops->match) > > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > > > > else > > > > rc = ((fwnode != NULL) && (h->fwnode == fwnode) && > > > > - ((bus_token == DOMAIN_BUS_ANY) || > > > > - (h->bus_token == bus_token))); > > > > + (h->bus_token == bus_token)); > > > > > > > > if (rc) { > > > > found = h; > > > > > > > > > > Can't say I like it either. It duplicates the existing check without > > > any obvious benefit. Honestly, this code is shit enough that we should > > > try to make it simpler, not more complex... > > > > Only the fwnode conditions are duplicated. And it makes sense: we > > should check for the DOMAIN_BUS_ANY first, before going to select. I'm > > not sure whether at some point we'd have to add (&& bus_token != > > DOMAIN_BUS_ANY) to the ops->match check. > > ops->match should just *die*, and it is not going to see any sort of > semantic upgrade. Ever. No new code should be added using match. > > And look at what my change does. It checks for DOMAIN_BUS_ANY before > doing anything else, ensuring that the default clause does the job. So > no, your suggestion doesn't make much sense to me. Yeah, I was worried about the DOMAIN_BUS_ANY vs match call. If that's not an issue, your patch looks fine to me. Please use 'Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>' with your patch. > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 6fb276504bcc..e9e9643c653f 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d, /* Handle pure domain searches */ if (!fwspec->param_count) - return d->bus_token == bus_token; + return d->bus_token == bus_token || + bus_token == DOMAIN_BUS_ANY; /* If this is not DT, then we have a single domain */ if (!is_of_node(fwspec->fwnode))