| Message ID | 20240803140443.23036-1-trintaeoitogc@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: remove type return | expand |
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.
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?
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
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?
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.
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
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);