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 F0B4AEE57C2 for ; Wed, 11 Sep 2024 16:35:15 +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:In-Reply-To:Content-Type: MIME-Version:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=ux7HoxGz927hOYI/1RZ9Xya9FMqtSWWDFNZQihRo02U=; b=Ge+LcREvHAzFxH seTPixILL4DBJjUjg/WO1guRRIdEWzewTyDaCcviWzg5jWKcuqjXd7AsKmmrcmfltu10dav2dfk9h yQCcn4CgeyMBK/SgWhyRKTd7hKvR+RdGrmF6UMV0l0MozQlZaPMb+jzZnaHZepSUaXgHFKa3L+CFz ksFUDCwKZBXKz6jXcFl7FRT9mVfy2cXWDXAjg6jwr5XaxTqUD8zDuPhJ8sm7LH0nKswRh7nwUeoEb d7w7xGBiOdVcyn2DvVlLPRJDCrJLVs4KvPpGtS1ckt3IcotFPIVulAlkUYwgzzrlY1s56oMoPR4jW riA5nnR1/hWZlHVtqcSA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1soQJ8-0000000AN0Q-1l3o; Wed, 11 Sep 2024 16:35:06 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1soQI4-0000000AMcH-0fUn for linux-arm-kernel@lists.infradead.org; Wed, 11 Sep 2024 16:34:01 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id D5DCB5C06E6; Wed, 11 Sep 2024 16:33:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C99A8C4CEC0; Wed, 11 Sep 2024 16:33:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1726072439; bh=me2IwCdr0YPbwk7LPVsZk6xYX4lHvRc5w/WlwEq5Ot0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Cke0NNEiY/Ntdi9DzOQWVIx2io2HmhtOE9Tfn/DtPR7XxhJ2nNd/qGrWUEHNo0Hu7 jtXTkS39wkSgMp5MdfGtAdXasMqWFKucOTqwcKosQ2zT355Tkz1r45LHx34oa7hkMk PFYXqcEQJnAV98EKJ1fw8P67GORnJ/TU3U6pBOKSv42z/bTShuyl5xPhDFjVxyN9fQ 3ZH2P8eMrByP8jTwXRBuAAT/mdCh4dEwC15fTixvA8LXqKJ/yDKBmitZw+BBY/475u M9thXb3k+g7hf2aRQkbAExivGG3g//uIsLFPSQ3l2XLhQkkGqPbGlheoEpDTYSCfcE xxQja9Fio1UyQ== Date: Wed, 11 Sep 2024 11:33:56 -0500 From: Bjorn Helgaas To: Frank Li Cc: Richard Zhu , Lucas Stach , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Rob Herring , Bjorn Helgaas , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Philipp Zabel , Liam Girdwood , Mark Brown , Manivannan Sadhasivam , Krzysztof Kozlowski , Conor Dooley , linux-pci@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, devicetree@vger.kernel.org, Qianqiang Liu Subject: Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC) support Message-ID: <20240911163356.GA643833@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240911_093400_351996_AB1827FE X-CRM114-Status: GOOD ( 27.34 ) 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 Wed, Sep 11, 2024 at 11:19:33AM -0400, Frank Li wrote: > On Wed, Sep 11, 2024 at 09:07:21AM -0500, Bjorn Helgaas wrote: > > [+cc Qianqiang] > > > > On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote: > > > From: Richard Zhu > > > > > > Implement i.MX8Q (i.MX8QM, i.MX8QXP, and i.MX8DXL) PCIe RC support. While > > > the controller resembles that of iMX8MP, the PHY differs significantly. > > > Notably, there's a distinction between PCI bus addresses and CPU addresses. > > > > > > Introduce IMX_PCIE_FLAG_CPU_ADDR_FIXUP in drvdata::flags to indicate driver > > > need the cpu_addr_fixup() callback to facilitate CPU address to PCI bus > > > address conversion according to "ranges" property. > > > > > +static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr) > > > +{ > > > + struct imx_pcie *imx_pcie = to_imx_pcie(pcie); > > > + struct dw_pcie_rp *pp = &pcie->pp; > > > + struct resource_entry *entry; > > > + unsigned int offset; > > > + > > > + if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_CPU_ADDR_FIXUP)) > > > + return cpu_addr; > > > + > > > + entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM); > > > + offset = entry->offset; > > > + return (cpu_addr - offset); > > > +} > > > > I'm sure that with enough effort, we could prove "entry" cannot be > > NULL here, but I'm not sure I want to spend the effort, and we're > > going to end up with more patches like this: > > > > https://lore.kernel.org/r/20240911125055.58555-1-qianqiang.liu@163.com > > > > I propose this minor change: > > > > entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM); > > if (!entry) > > return cpu_addr; > > > > return cpu_addr - entry->offset; > > > > I still think we should get rid of the .cpu_addr_fixup() callback if > > possible. But that's a discussion for another day. > > Stop these fake alarm from some tools's scan. entry never be NULL here. > I am working on EP side by involve a "ranges" support like RC side. > > Or just omit this kinds of patches. As I said initially, we probably *could* prove that "entry" can never be NULL here, but why should I have to spend the effort to do that? The "windows" list is not even built in this file, so it's not trivial. And even if "entry" can't be NULL now, what's to prevent that assumption from breaking in the future? I don't think there's anything wrong with checking for NULL here, and it avoids copy/pasting this somewhere where it *does* matter. So I'm in favor of this kind of patch. Bjorn