Message-ID:
 <PN0PR01MB10393326D387DD49EF563F787FED52@PN0PR01MB10393.INDPRD01.PROD.OUTLOOK.COM>
Date: Fri, 7 Mar 2025 22:44:10 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Siddharth Vadapalli <s-vadapalli@...com>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Chen Wang <unicornxw@...il.com>, lpieralisi@...nel.org, kw@...ux.com,
 robh@...nel.org, bhelgaas@...gle.com, thomas.richard@...tlin.com,
 bwawrzyn@...co.com, wojciech.jasko-EXT@...tinental-corporation.com,
 kishon@...nel.org, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 sophgo@...ts.linux.dev
Subject: Re: [PATCH] PCI: cadence: Fix NULL pointer error for ops


On 2025/3/7 21:47, Siddharth Vadapalli wrote:
> On Fri, Mar 07, 2025 at 09:07:35PM +0800, Chen Wang wrote:
>> Hello~
>>
>> Any comment on this? Or can we have this bugfix patch picked for coming
>> v6.15?
> Is there a driver in Linux which is affected by the issue that you are
> trying to fix in this patch? Please point to the driver since I don't
> see such a driver.
>
> Regards,
> Siddharth.

Oh, sorry I didn't explain the change history clearly. I am developing a 
PCIe driver for a new soc (SG2042), and this PCIe controller uses 
cadence's IP. In the code, I found that if I don't assign a value to 
cdns_pcie.ops, it will crash during operation. At first, I didn't fix 
the bug in the cadence code, but used a workaround in the SG2042 driver. 
Later in the code review, Manivannan pointed out my problem and hoped 
that I would submit a patch to fix this problem in the cadence driver.

Adding Manivannan who should know about this.

Please take a look at this: 
https://lore.kernel.org/linux-riscv/20250119122353.v3tzitthmu5tu3dg@thinkpad/. 
For your convenience, I have excerpted some of the text below.

```

 > +static struct pci_ops sg2042_pcie_host_ops = {
 > +    .map_bus    = cdns_pci_map_bus,
 > +    .read        = sg2042_pcie_config_read,
 > +    .write        = sg2042_pcie_config_write,
 > +};
 > +
 > +/* Dummy ops which will be assigned to cdns_pcie.ops, which must be 
!NULL. */

Because cadence code driver doesn't check for !ops? Please fix it 
instead. And
the fix is trivial.

 > +static const struct cdns_pcie_ops sg2042_cdns_pcie_ops = {};
 > +
 > +static int sg2042_pcie_probe(struct platform_device *pdev)
 > +{
[......]
 > +    struct cdns_pcie *cdns_pcie;

[......]

 > +    cdns_pcie->ops = &sg2042_cdns_pcie_ops;
 > +    pcie->cdns_pcie = cdns_pcie;

```


Regards,

Chen


>> Regards,
>>
>> Chen
>>
>> On 2025/3/4 16:17, Chen Wang wrote:
>>> From: Chen Wang <unicorn_wang@...look.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")
>>> Signed-off-by: Chen Wang <unicorn_wang@...look.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

Powered by blists - more mailing lists