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

[Bjorn Helgaas <helgaas@kernel.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 <hongxing.zhu@nxp.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