[PATCH] PCI: remove type return

[PATCH] PCI: remove type return

[Guilherme Giacomo Simoes]

I can see that the function pci_hp_add_brigde have a int return propagation.
But in the drivers that pci_hp_add_bridge is called, your return never is
cheked.
This make me a think that the add bridge for pci hotplug drivers is not crucial
for functionaly and your failed only should show a message in logs.

Then, I maked this patch for remove your return propagation for this moment.
---
 drivers/pci/pci.h   | 2 +-
 drivers/pci/probe.c | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 79c8398f3938..a35dbfd89961 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -189,7 +189,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 #endif
 
 /* Functions for PCI Hotplug drivers to use */
-int pci_hp_add_bridge(struct pci_dev *dev);
+void pci_hp_add_bridge(struct pci_dev *dev);
 
 #if defined(CONFIG_SYSFS) && defined(HAVE_PCI_LEGACY)
 void pci_create_legacy_files(struct pci_bus *bus);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..b13c4c912eb1 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3352,7 +3352,7 @@ void __init pci_sort_breadthfirst(void)
 	bus_sort_breadthfirst(&pci_bus_type, &pci_sort_bf_cmp);
 }
 
-int pci_hp_add_bridge(struct pci_dev *dev)
+void pci_hp_add_bridge(struct pci_dev *dev)
 {
 	struct pci_bus *parent = dev->bus;
 	int busnr, start = parent->busn_res.start;
@@ -3365,7 +3365,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	}
 	if (busnr-- > end) {
 		pci_err(dev, "No bus number available for hot-added bridge\n");
-		return -1;
+		return;
 	}
 
 	/* Scan bridges that are already configured */
@@ -3381,8 +3381,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
 
 	if (!dev->subordinate)
-		return -1;
+		pci_err(dev, "No bus subordinate");
 
-	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
-- 
2.46.0

Re: [PATCH] PCI: remove type return

[Ilpo Järvinen]

On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:

> I can see that the function pci_hp_add_brigde have a int return propagation.

typo in function name. Add parenthesis after function names like this:
pci_hp_add_bridge()

> But in the drivers that pci_hp_add_bridge is called, your return never is
> cheked.

checked.

> This make me a think that the add bridge for pci hotplug drivers is not crucial
> for functionaly and your failed only should show a message in logs.

functionality

> 
> Then, I maked this patch for remove your return propagation for this moment.

Please write the commit message using imperative tone. Don't use "I", 
"me", "you", "your", or "we" at all.

Also, you need to signoff your patches (please read 
Documentation/process/submitting-patches.rst).

The lack of return value checking seems to be on the list in
pci_hp_add_bridge(). So perhaps the right course of action would be to 
handle return values correctly.

-- 
 i.

> ---
>  drivers/pci/pci.h   | 2 +-
>  drivers/pci/probe.c | 7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 79c8398f3938..a35dbfd89961 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -189,7 +189,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
>  #endif
>  
>  /* Functions for PCI Hotplug drivers to use */
> -int pci_hp_add_bridge(struct pci_dev *dev);
> +void pci_hp_add_bridge(struct pci_dev *dev);
>  
>  #if defined(CONFIG_SYSFS) && defined(HAVE_PCI_LEGACY)
>  void pci_create_legacy_files(struct pci_bus *bus);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b14b9876c030..b13c4c912eb1 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3352,7 +3352,7 @@ void __init pci_sort_breadthfirst(void)
>  	bus_sort_breadthfirst(&pci_bus_type, &pci_sort_bf_cmp);
>  }
>  
> -int pci_hp_add_bridge(struct pci_dev *dev)
> +void pci_hp_add_bridge(struct pci_dev *dev)
>  {
>  	struct pci_bus *parent = dev->bus;
>  	int busnr, start = parent->busn_res.start;
> @@ -3365,7 +3365,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
>  	}
>  	if (busnr-- > end) {
>  		pci_err(dev, "No bus number available for hot-added bridge\n");
> -		return -1;
> +		return;
>  	}
>  
>  	/* Scan bridges that are already configured */
> @@ -3381,8 +3381,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
>  	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
>  
>  	if (!dev->subordinate)
> -		return -1;
> +		pci_err(dev, "No bus subordinate");
>  
> -	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
> 

Re: [PATCH] PCI: remove type return

[Guilherme Giácomo Simões]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

>
> On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:
>
> > I can see that the function pci_hp_add_brigde have a int return propagation.
>
> typo in function name. Add parenthesis after function names like this:
> pci_hp_add_bridge()
>
> > But in the drivers that pci_hp_add_bridge is called, your return never is
> > cheked.
>
> checked.
>
> > This make me a think that the add bridge for pci hotplug drivers is not crucial
> > for functionaly and your failed only should show a message in logs.
>
> functionality
>
> >
> > Then, I maked this patch for remove your return propagation for this moment.
>
> Please write the commit message using imperative tone. Don't use "I",
> "me", "you", "your", or "we" at all.
>
> Also, you need to signoff your patches (please read
> Documentation/process/submitting-patches.rst).
>
> The lack of return value checking seems to be on the list in
> pci_hp_add_bridge(). So perhaps the right course of action would be to
> handle return values correctly.
>
> --
>  i.
>

Ok, so if the right course is for the driver to handle return value,
then this is a
task for the driver developers, because only they know what to do when
pci_hp_add_bridge() doesn't work correctly, right?

Re: [PATCH] PCI: remove type return

[Bjorn Helgaas]

On Tue, Aug 06, 2024 at 05:54:15PM -0300, Guilherme Giácomo Simões wrote:
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:
> >
> > > I can see that the function pci_hp_add_brigde have a int return
> > > propagation.
> ...

> > The lack of return value checking seems to be on the list in
> > pci_hp_add_bridge(). So perhaps the right course of action would be to
> > handle return values correctly.
> 
> Ok, so if the right course is for the driver to handle return value,
> then this is a
> task for the driver developers, because only they know what to do when
> pci_hp_add_bridge() doesn't work correctly, right?

pci_hp_add_bridge() is only for hotplug drivers, so the list of
callers is short and completely under our control.  There's plenty of
opportunity for improving this.  Beyond just the return value, all the
callers of pci_hp_add_bridge() should be doing much of the same work
that could potentially be factored out.

Bjorn

Re: [PATCH] PCI: remove type return

[Guilherme Giácomo Simões]

Bjorn Helgaas <helgaas@kernel.org> writes:
>
> On Tue, Aug 06, 2024 at 05:54:15PM -0300, Guilherme Giácomo Simões wrote:
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:
> > >
> > > > I can see that the function pci_hp_add_brigde have a int return
> > > > propagation.
> > ...
>
> > > The lack of return value checking seems to be on the list in
> > > pci_hp_add_bridge(). So perhaps the right course of action would be to
> > > handle return values correctly.
> >
> > Ok, so if the right course is for the driver to handle return value,
> > then this is a
> > task for the driver developers, because only they know what to do when
> > pci_hp_add_bridge() doesn't work correctly, right?
>
> pci_hp_add_bridge() is only for hotplug drivers, so the list of
> callers is short and completely under our control.  There's plenty of
> opportunity for improving this.  Beyond just the return value, all the
> callers of pci_hp_add_bridge() should be doing much of the same work
> that could potentially be factored out.
>
> Bjorn

Okay, then what the action that the drivers must be do when the add
bridge is failed?

Re: [PATCH] PCI: remove type return

[Guilherme Giácomo Simões]

Guilherme Giácomo Simões <trintaeoitogc@gmail.com> write:
>
> Bjorn Helgaas <helgaas@kernel.org> writes:
> >
> > On Tue, Aug 06, 2024 at 05:54:15PM -0300, Guilherme Giácomo Simões wrote:
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > > On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:
> > > >
> > > > > I can see that the function pci_hp_add_brigde have a int return
> > > > > propagation.
> > > ...
> >
> > > > The lack of return value checking seems to be on the list in
> > > > pci_hp_add_bridge(). So perhaps the right course of action would be to
> > > > handle return values correctly.
> > >
> > > Ok, so if the right course is for the driver to handle return value,
> > > then this is a
> > > task for the driver developers, because only they know what to do when
> > > pci_hp_add_bridge() doesn't work correctly, right?
> >
> > pci_hp_add_bridge() is only for hotplug drivers, so the list of
> > callers is short and completely under our control.  There's plenty of
> > opportunity for improving this.  Beyond just the return value, all the
> > callers of pci_hp_add_bridge() should be doing much of the same work
> > that could potentially be factored out.
> >
> > Bjorn
>
> Okay, then what the action that the drivers must be do when the add
> bridge is failed?

Maybe we can make a change to the drivers that use pci_hp_add_brigde() to
check the return of the function and return an error to its caller like this:

ret = pci_hp_add_bridge(dev);
if (ret)
    goto out;

I have looked at all the drivers that use pci_hp_add_brigde() and they all have
an out flag to call pci_unlock_rescan_remove() to unlock the mutex and
return a code.
We can return in the out flag the code that is the return of
pci_hp_add_brigde() and
then we can set that return code to some #define to identify the error
that is returned.

Re: [PATCH] PCI: remove type return

[Bjorn Helgaas]

On Thu, Aug 08, 2024 at 06:05:54PM -0300, Guilherme Giácomo Simões wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> >
> > On Tue, Aug 06, 2024 at 05:54:15PM -0300, Guilherme Giácomo Simões wrote:
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > > On Sat, 3 Aug 2024, Guilherme Giacomo Simoes wrote:
> > > >
> > > > > I can see that the function pci_hp_add_brigde have a int return
> > > > > propagation.
> > > ...
> >
> > > > The lack of return value checking seems to be on the list in
> > > > pci_hp_add_bridge(). So perhaps the right course of action would be to
> > > > handle return values correctly.
> > >
> > > Ok, so if the right course is for the driver to handle return value,
> > > then this is a
> > > task for the driver developers, because only they know what to do when
> > > pci_hp_add_bridge() doesn't work correctly, right?
> >
> > pci_hp_add_bridge() is only for hotplug drivers, so the list of
> > callers is short and completely under our control.  There's plenty of
> > opportunity for improving this.  Beyond just the return value, all the
> > callers of pci_hp_add_bridge() should be doing much of the same work
> > that could potentially be factored out.
> 
> Okay, then what the action that the drivers must be do when the add
> bridge is failed?

pci_hp_add_bridge() fails when there's no bus number available to
assign to new hot-added devices.  When that happens, there's really
nothing the hotplug drivers can do to improve the situation.
pci_hp_add_bridge() already logs a message for one of the failure
cases.  It may be that it should also log a message for the other
failure case.  The end result is that we can't use the hot-added
devices because there's no space for them in the PCI bus number space,
so we can't address them.

Bjorn