diff mbox series

[v11,04/11] PCI: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()

Message ID 20250313-pci_fixup_addr-v11-4-01d2313502ab@nxp.com (mailing list archive)
State New
Headers show
Series PCI: Use device bus range info to cleanup RC Host/EP pci_fixup_addr() | expand

Commit Message

Frank Li March 13, 2025, 3:38 p.m. UTC
Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
any DWC resource, moving it earlier improves code logic and readability.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Bjorn Helgaas March 13, 2025, 7:22 p.m. UTC | #1
On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> any DWC resource, moving it earlier improves code logic and readability.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c57831902686e..52a441662cabe 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	raw_spin_lock_init(&pp->lock);
>  
> +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> +	if (!bridge)
> +		return bridge;

This returns NULL (0) where it previously returned -ENOMEM.  Callers
interpret zero as "success", so I think it should stil return -ENOMEM.

I tentatively changed it back to -ENOMEM locally, let me know if
that's wrong.

> +	pp->bridge = bridge;
> +
>  	ret = dw_pcie_get_resources(pci);
>  	if (ret)
>  		return ret;
> @@ -460,12 +466,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		return ret;
>  
> -	bridge = devm_pci_alloc_host_bridge(dev, 0);
> -	if (!bridge)
> -		return -ENOMEM;
> -
> -	pp->bridge = bridge;
> -
>  	/* Get the I/O range from DT */
>  	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
>  	if (win) {
> 
> -- 
> 2.34.1
>
Frank Li March 13, 2025, 8:45 p.m. UTC | #2
On Thu, Mar 13, 2025 at 02:22:54PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> > Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> > Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> > any DWC resource, moving it earlier improves code logic and readability.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c57831902686e..52a441662cabe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >
> >  	raw_spin_lock_init(&pp->lock);
> >
> > +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > +	if (!bridge)
> > +		return bridge;
>
> This returns NULL (0) where it previously returned -ENOMEM.  Callers
> interpret zero as "success", so I think it should stil return -ENOMEM.

It should be -ENOMEM. Sorry for that. Strange, not sure what happen when
I copy/past code.

Do you need respin it or you can fix it?

Frank

>
> I tentatively changed it back to -ENOMEM locally, let me know if
> that's wrong.
>
> > +	pp->bridge = bridge;
> > +
> >  	ret = dw_pcie_get_resources(pci);
> >  	if (ret)
> >  		return ret;
> > @@ -460,12 +466,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (ret)
> >  		return ret;
> >
> > -	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > -	if (!bridge)
> > -		return -ENOMEM;
> > -
> > -	pp->bridge = bridge;
> > -
> >  	/* Get the I/O range from DT */
> >  	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
> >  	if (win) {
> >
> > --
> > 2.34.1
> >
Bjorn Helgaas March 13, 2025, 9:25 p.m. UTC | #3
On Thu, Mar 13, 2025 at 04:45:26PM -0400, Frank Li wrote:
> On Thu, Mar 13, 2025 at 02:22:54PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> > > Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> > > Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> > > any DWC resource, moving it earlier improves code logic and readability.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index c57831902686e..52a441662cabe 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >
> > >  	raw_spin_lock_init(&pp->lock);
> > >
> > > +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > > +	if (!bridge)
> > > +		return bridge;
> >
> > This returns NULL (0) where it previously returned -ENOMEM.  Callers
> > interpret zero as "success", so I think it should stil return -ENOMEM.
> 
> It should be -ENOMEM. Sorry for that. Strange, not sure what happen when
> I copy/past code.
> 
> Do you need respin it or you can fix it?

I fixed it locally.  But you should fix it, too, in case we do another
spin for other reasons.

> > I tentatively changed it back to -ENOMEM locally, let me know if
> > that's wrong.
> >
> > > +	pp->bridge = bridge;
> > > +
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index c57831902686e..52a441662cabe 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -452,6 +452,12 @@  int dw_pcie_host_init(struct dw_pcie_rp *pp)
 
 	raw_spin_lock_init(&pp->lock);
 
+	bridge = devm_pci_alloc_host_bridge(dev, 0);
+	if (!bridge)
+		return bridge;
+
+	pp->bridge = bridge;
+
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
 		return ret;
@@ -460,12 +466,6 @@  int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		return ret;
 
-	bridge = devm_pci_alloc_host_bridge(dev, 0);
-	if (!bridge)
-		return -ENOMEM;
-
-	pp->bridge = bridge;
-
 	/* Get the I/O range from DT */
 	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
 	if (win) {