Re: [PATCH] PCI: cadence: Fix NULL pointer error for ops

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Mar 12, 2025 at 10:08:43AM +0800, Chen Wang wrote:
> Hello, Bjorn, Lorenzo & Manivannan,
> 
> I find your names in MAINTAINERS for PCI controllers, could you please pick
> this patch for v6.15?
> 
> Or who else should I submit a PR for this patch to?
> 
> BTW, Siddharth signed the review for this patch (see [1]). Please add this
> when submitting, thanks in advance.
> 
> Link:
> https://lore.kernel.org/linux-pci/20250307151949.7rmxl22euubnzzpj@uda0492258/
> [1]

> On 2025/3/4 16:17, Chen Wang wrote:
> > From: Chen Wang <unicorn_wang@outlook.com>
> > 
> > ops of struct cdns_pcie may be NULL, direct use
> > will result in a null pointer error.
> > 
> > Add checking of pcie->ops before using it.
> > 
> > Fixes: 40d957e6f9eb ("PCI: cadence: Add support to start link and verify link status")

AFAICT this does not fix a problem in 40d957e6f9eb, since there is no
driver that calls cdns_pcie_host_setup() or cdns_pcie_ep_setup() with
a NULL pcie->ops pointer, so I think you should drop this Fixes: tag.

I see that you probably want to *add* an sg2042 driver [2] where you
don't need a pcie->ops pointer (although the current patch at [2]
*does* supply a valid pointer).

So there's no urgency to apply this until you post an sg2042 driver
that doesn't fill in the pcie->ops pointer.  The best way to do this
would be to include this patch in the series that adds the sg2042
driver.

Then the commit log can explain exactly why we need it (because the
sg2042 in the next patch of the series doesn't need a pcie->ops
pointer), and it will be easy to review.

[2] https://lore.kernel.org/r/ddedd8f76f83fea2c6d3887132d2fe6f2a6a02c1.1736923025.git.unicorn_wang@outlook.com

> > Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> > ---
> >   drivers/pci/controller/cadence/pcie-cadence-host.c | 2 +-
> >   drivers/pci/controller/cadence/pcie-cadence.c      | 4 ++--
> >   drivers/pci/controller/cadence/pcie-cadence.h      | 6 +++---
> >   3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > index 8af95e9da7ce..9b9d7e722ead 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > @@ -452,7 +452,7 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
> >   	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1);
> >   	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1);
> > -	if (pcie->ops->cpu_addr_fixup)
> > +	if (pcie->ops && pcie->ops->cpu_addr_fixup)
> >   		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> >   	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(12) |
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
> > index 204e045aed8c..56c3d6cdd70e 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> > @@ -90,7 +90,7 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
> >   	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(r), desc1);
> >   	/* Set the CPU address */
> > -	if (pcie->ops->cpu_addr_fixup)
> > +	if (pcie->ops && pcie->ops->cpu_addr_fixup)
> >   		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> >   	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) |
> > @@ -120,7 +120,7 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie,
> >   	}
> >   	/* Set the CPU address */
> > -	if (pcie->ops->cpu_addr_fixup)
> > +	if (pcie->ops && pcie->ops->cpu_addr_fixup)
> >   		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> >   	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(17) |
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> > index f5eeff834ec1..436630d18fe0 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > @@ -499,7 +499,7 @@ static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
> >   static inline int cdns_pcie_start_link(struct cdns_pcie *pcie)
> >   {
> > -	if (pcie->ops->start_link)
> > +	if (pcie->ops && pcie->ops->start_link)
> >   		return pcie->ops->start_link(pcie);
> >   	return 0;
> > @@ -507,13 +507,13 @@ static inline int cdns_pcie_start_link(struct cdns_pcie *pcie)
> >   static inline void cdns_pcie_stop_link(struct cdns_pcie *pcie)
> >   {
> > -	if (pcie->ops->stop_link)
> > +	if (pcie->ops && pcie->ops->stop_link)
> >   		pcie->ops->stop_link(pcie);
> >   }
> >   static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
> >   {
> > -	if (pcie->ops->link_up)
> > +	if (pcie->ops && pcie->ops->link_up)
> >   		return pcie->ops->link_up(pcie);
> >   	return true;
> > 
> > base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6