On Thu, Oct 17, 2024 at 05:31:31PM +0300, Ilpo Järvinen wrote:
> PCI_ScanBusForNonBridge() has two loops, first searching for
> non-bridges and second that looks for bridges. The second loop has
> hints in a debug print it should do recursion for buses underneath the
> bridge but no recursion is attempted.
>
> Since the second loop is quite useless in its current form, just
> eliminate it. This code hasn't been touched for very long time so
> either it's unused or the missing parts are not important enough for
> anyone to attempt to add them.
>
> Leave only a simple comment about the missing recursion for the
> unlikely case that somebody comes across the lack of functionality. In
> any case, search whether an endpoint exists downstream of a bridge
> sounds generic enough to belong to core so if the functionality is to
> be extended it should probably be moved into PCI core.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> drivers/pci/hotplug/cpqphp_pci.c | 30 +++++++++++-------------------
> 1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
> index 558866c15e03..b2efc4a90864 100644
> --- a/drivers/pci/hotplug/cpqphp_pci.c
> +++ b/drivers/pci/hotplug/cpqphp_pci.c
> @@ -190,8 +190,7 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
> {
> u16 tdevice;
> u32 work;
> - int ret;
> - u8 tbus;
> + int ret = -1;
>
> ctrl->pci_bus->number = bus_num;
>
> @@ -208,26 +207,19 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
> *dev_num = tdevice;
> dbg("found it !\n");
> return 0;
> - }
> - }
> - for (tdevice = 0; tdevice < 0xFF; tdevice++) {
> - /* Scan for access first */
> - if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
> - continue;
> - ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, PCI_CLASS_REVISION, &work);
> - if (ret)
> - continue;
> - dbg("Looking for bridge bus_num %d dev_num %d\n", bus_num, tdevice);
> - /* Yep we got one. bridge ? */
> - if ((work >> 8) == PCI_TO_PCI_BRIDGE_CLASS) {
> - pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(tdevice, 0), PCI_SECONDARY_BUS, &tbus);
> - /* XXX: no recursion, wtf? */
> - dbg("Recurse on bus_num %d tdevice %d\n", tbus, tdevice);
> - return 0;
> + } else {
> + /*
> + * XXX: Code whose debug printout indicated
> + * recursion to buses underneath bridges might be
> + * necessary was removed because it never did
> + * any recursion.
> + */
> + ret = 0;
I'm OK with this except that I wonder if we should leave an actual
info or even warning level printk here as a more visible debugging
hint if somebody hits this. I'm not sure that simply returning 0
would be enough of a hint about why devices below the bridge weren't
found.
> }
> }
>
> - return -1;
> +
> + return ret;
> }
>
>
> --
> 2.39.5
>
