[v1] PCI: s32g: Fix ports parsing

[PATCH] PCI: s32g: Fix ports parsing

Posted by Vincent Guittot 4 days, 16 hours ago

No error return is missing after the loop resulting in removing the
ports from the list.

Fixes: 5cbc7d3e316e ("PCI: s32g: Add NXP S32G PCIe controller driver (RC)")
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
index 47745749f75c..767f5a622441 100644
--- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
+++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
@@ -285,6 +285,8 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
 			goto err_port;
 	}
 
+	return 0;
+
 err_port:
 	list_for_each_entry_safe(port, tmp, &s32g_pp->ports, list)
 		list_del(&port->list);
-- 
2.43.0

Re: [PATCH] PCI: s32g: Fix ports parsing

Posted by Manivannan Sadhasivam 4 days, 16 hours ago

On Mon, Feb 02, 2026 at 02:07:56PM +0100, Vincent Guittot wrote:
> No error return is missing after the loop resulting in removing the
> ports from the list.
> 
> Fixes: 5cbc7d3e316e ("PCI: s32g: Add NXP S32G PCIe controller driver (RC)")
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> index 47745749f75c..767f5a622441 100644
> --- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> @@ -285,6 +285,8 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
>  			goto err_port;
>  	}
>  
> +	return 0;
> +

Hmm. So the initial 'ret = -ENOENT' will become redundant and you'll return
success even if there are no Root Ports in DT.

You should do:

	if (!of_get_available_child_count(dev->of_node))
		return -ENOENT;

at the start and remove the 'ret' initialization.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: s32g: Fix ports parsing

Posted by Vincent Guittot 4 days, 16 hours ago

On Mon, 2 Feb 2026 at 14:18, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Mon, Feb 02, 2026 at 02:07:56PM +0100, Vincent Guittot wrote:
> > No error return is missing after the loop resulting in removing the
> > ports from the list.
> >
> > Fixes: 5cbc7d3e316e ("PCI: s32g: Add NXP S32G PCIe controller driver (RC)")
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > index 47745749f75c..767f5a622441 100644
> > --- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > @@ -285,6 +285,8 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
> >                       goto err_port;
> >       }
> >
> > +     return 0;
> > +
>
> Hmm. So the initial 'ret = -ENOENT' will become redundant and you'll return
> success even if there are no Root Ports in DT.

good point.

>
> You should do:
>
>         if (!of_get_available_child_count(dev->of_node))
>                 return -ENOENT;

or maybe

+       if (!ret)
+               return 0;

>
> at the start and remove the 'ret' initialization.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: s32g: Fix ports parsing

Posted by Manivannan Sadhasivam 4 days, 16 hours ago

On Mon, Feb 02, 2026 at 02:23:52PM +0100, Vincent Guittot wrote:
> On Mon, 2 Feb 2026 at 14:18, Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > On Mon, Feb 02, 2026 at 02:07:56PM +0100, Vincent Guittot wrote:
> > > No error return is missing after the loop resulting in removing the
> > > ports from the list.
> > >
> > > Fixes: 5cbc7d3e316e ("PCI: s32g: Add NXP S32G PCIe controller driver (RC)")
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > index 47745749f75c..767f5a622441 100644
> > > --- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > @@ -285,6 +285,8 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
> > >                       goto err_port;
> > >       }
> > >
> > > +     return 0;
> > > +
> >
> > Hmm. So the initial 'ret = -ENOENT' will become redundant and you'll return
> > success even if there are no Root Ports in DT.
> 
> good point.
> 
> >
> > You should do:
> >
> >         if (!of_get_available_child_count(dev->of_node))
> >                 return -ENOENT;
> 
> or maybe
> 
> +       if (!ret)
> +               return 0;
> 

That'll work too. For some reason, I initially thought it would be ugly, but it
doesn't :)

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: s32g: Fix ports parsing

Posted by Vincent Guittot 4 days, 16 hours ago

Le lundi 02 févr. 2026 à 18:58:34 (+0530), Manivannan Sadhasivam a écrit :
> On Mon, Feb 02, 2026 at 02:23:52PM +0100, Vincent Guittot wrote:
> > On Mon, 2 Feb 2026 at 14:18, Manivannan Sadhasivam <mani@kernel.org> wrote:
> > >
> > > On Mon, Feb 02, 2026 at 02:07:56PM +0100, Vincent Guittot wrote:
> > > > No error return is missing after the loop resulting in removing the
> > > > ports from the list.
> > > >
> > > > Fixes: 5cbc7d3e316e ("PCI: s32g: Add NXP S32G PCIe controller driver (RC)")
> > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > > index 47745749f75c..767f5a622441 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> > > > @@ -285,6 +285,8 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
> > > >                       goto err_port;
> > > >       }
> > > >
> > > > +     return 0;
> > > > +
> > >
> > > Hmm. So the initial 'ret = -ENOENT' will become redundant and you'll return
> > > success even if there are no Root Ports in DT.
> > 
> > good point.
> > 
> > >
> > > You should do:
> > >
> > >         if (!of_get_available_child_count(dev->of_node))
> > >                 return -ENOENT;
> > 
> > or maybe
> > 
> > +       if (!ret)
> > +               return 0;
> > 
> 
> That'll work too. For some reason, I initially thought it would be ugly, but it
> doesn't :)

In fact this enables more changes and I end up with:

@@ -282,12 +282,12 @@ static int s32g_pcie_parse_ports(struct device *dev, struct s32g_pcie *s32g_pp)
 
 		ret = s32g_pcie_parse_port(s32g_pp, of_port);
 		if (ret)
-			goto err_port;
+			break;
 	}
 
-err_port:
-	list_for_each_entry_safe(port, tmp, &s32g_pp->ports, list)
-		list_del(&port->list);
+	if (ret)
+		list_for_each_entry_safe(port, tmp, &s32g_pp->ports, list)
+			list_del(&port->list);
 
 	return ret;
 }
-- 


> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்