| Message ID | 20250304081742.848985-1-unicornxw@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: cadence: Fix NULL pointer error for ops | expand |
Hello~ Any comment on this? Or can we have this bugfix patch picked for coming v6.15? Regards, Chen 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") > 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
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. > > Regards, > > Chen > > 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") > > 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
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@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") >>> 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
On Fri, Mar 07, 2025 at 10:44:10PM +0800, Chen Wang wrote: > > 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. Thank you for providing the context for this patch. Maybe the context should go into the commit message, but that's not a reason to block this patch, so: Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> Regards, Siddharth.
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] Regards, Chen 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") > 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
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
On 2025/3/12 23:33, Bjorn Helgaas wrote: > 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 OK, I'll do as you say. Regards, Chen >>> 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
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;