From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5655CC3ABC3 for ; Mon, 12 May 2025 16:25:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OYGTPvprh2yHBjev45QriTsPEU8EWl6pESotRhWCUCk=; b=e55e6pD8N5t+b6FdsvknK4OS4H tI2dbGlebW+qpMZvd5FNF/PvR2G8BxWluih7vkCorhod0tN5OdZV0/xMX5RFruyre+kiMVUI/Q3X6 c+vD4waYybuLKUb2UD6ReHWfW/64BFiy8r9OXNETDBXDfVEoXLJbwZqx684gNpYiRD7DTvNah+IFb Nu22nmaakF9fiQQInk02TN/zSf9k/1nYsRGvwpfmVQNOYHn0/5JQHwa2LaYuQSnWd1YFHwCkMcoXz 6XOnzGx2ParsnlhTNTmcqq1BsmhOAaeae1i97FVx4TgQygfgnug/Ic3Uk4SFrDx8R7k0iVodaC18l oLYyJU/w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uEVxk-0000000A1p9-0Vs1; Mon, 12 May 2025 16:25:08 +0000 Received: from nyc.source.kernel.org ([2604:1380:45d1:ec00::3]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uEVWa-00000009xO5-3o6B for linux-arm-kernel@lists.infradead.org; Mon, 12 May 2025 15:57:06 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 2CC2CA4D84F; Mon, 12 May 2025 15:57:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C0B32C4CEEF; Mon, 12 May 2025 15:57:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1747065423; bh=vIJFy1J5IitTiA0zWWnMwAXixJjGYXx9yRIC4TtCy9Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=cJilTLEzWJ+5/y/u6z/HiA2WABgTOZS+kDBIWyxXNQ9S8F0u5VUcSDqSd0dmfDjdU /oEmw7yBQMJ4ttM1993SPmSECYK9szAuh/vwTtkQh96XzsvGyugingJXYGyWqSALbm edeGk/SoxtjTZ1HzqcfNrp8T93fsNg7ouDzRfR53T4QxjKJocHj8tIboenGYx//J2T qgVBtzL/FlweuDhwKv5KGg8VWSd5WoMO/eUxEu+x3TF8x6UVT8TH9d44Mx57eb6z3b qacoxTFdyq1Y6QCfUILmPmBzRpR0+WUMFvInHLpjUN5/K6ngCQuL9SuAObmYr7UyTn WhzuegTndAoLA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1uEVWX-00EBWw-LB; Mon, 12 May 2025 16:57:01 +0100 Date: Mon, 12 May 2025 16:57:00 +0100 Message-ID: <86tt5pg6ur.wl-maz@kernel.org> From: Marc Zyngier To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Lorenzo Pieralisi , Sascha Bischoff , Timothy Hayes Subject: Re: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare() In-Reply-To: <87h61plx64.ffs@tglx> References: <20250511163520.1307654-1-maz@kernel.org> <20250511163520.1307654-2-maz@kernel.org> <87h61plx64.ffs@tglx> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: tglx@linutronix.de, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, lpieralisi@kernel.org, sascha.bischoff@arm.com, timothy.hayes@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250512_085705_082431_09DA2336 X-CRM114-Status: GOOD ( 27.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 12 May 2025 15:29:39 +0100, Thomas Gleixner wrote: > > On Sun, May 11 2025 at 17:35, Marc Zyngier wrote: > > > While the MSI ops do have a .msi_prepare() callback that is > > responsible for setting up the relevant (usually per-device) > > allocation, we don't have a callback reversing this setup. > > ..., there is no callback reversing ... > > > For this purpose, let's a .msi_teardown() callback. This is > > 'let's a ...' is not a sentence. Just say: add a .... calback. > > > reliying on the msi_domain_info structure having a non-NULL > > ^^^^^ spell check is your friend. I rely on humans for that. But maybe I should ask someone to put these recommendations into one of these AI bots, and generate the stuff automatically. It will be devoid of any actual reasoning, but at least it will have the "officially sanctioned" verbiage. > > > alloc_data field. > > > > Nobody is populating this field yet, so there is no change > > No driver is .. No. There is definitely no driver populating this, nor there will ever be. That's 100% MSI infrastructure. > > > > > +static void msi_domain_ops_teardown(struct irq_domain *domain, > > + msi_alloc_info_t *arg) > > No line break required. You mean... > > > +{ > > +} > > + > > static void msi_domain_ops_set_desc(msi_alloc_info_t *arg, > > struct msi_desc *desc) ... not like this? > > { > > @@ -821,6 +826,7 @@ static struct msi_domain_ops msi_domain_ops_default = { > > .get_hwirq = msi_domain_ops_get_hwirq, > > .msi_init = msi_domain_ops_init, > > .msi_prepare = msi_domain_ops_prepare, > > + .msi_teardown = msi_domain_ops_teardown, > > .set_desc = msi_domain_ops_set_desc, > > }; > > > > @@ -842,6 +848,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info) > > ops->msi_init = msi_domain_ops_default.msi_init; > > if (ops->msi_prepare == NULL) > > ops->msi_prepare = msi_domain_ops_default.msi_prepare; > > + if (ops->msi_teardown == NULL) > > + ops->msi_teardown = msi_domain_ops_default.msi_teardown; > > if (ops->set_desc == NULL) > > ops->set_desc = msi_domain_ops_default.set_desc; > > } > > @@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) > > > > dev->msi.data->__domains[domid].domain = NULL; > > info = domain->host_data; > > + > > + if (info->alloc_data) > > + info->ops->msi_teardown(domain, info->alloc_data); > > Hmm, that's weird. > > Why not call it unconditionally. The empty teardown() default callback > does not care about @arg being NULL. No? Because at this point, nothing populates that pointer. It is only after patch 3 that this pointer is valid. After patch 2, we get a non-default callback, which should never be presented with a NULL allocation context. And since I value keeping things bisectable, it has to happen in this order. Thanks, M. -- Without deviation from the norm, progress is not possible.