linux-kernel - Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240911163356.GA643833@bhelgaas>
Date: Wed, 11 Sep 2024 11:33:56 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Frank Li <Frank.li@....com>
Cc: Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
	devicetree@...r.kernel.org, Qianqiang Liu <qianqiang.liu@....com>
Subject: Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC)
 support

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 <hongxing.zhu@....com>
> > >
> > > 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