Re: [PATCH 4/4] PCI: cpqphp: Simplify PCI_ScanBusForNonBridge()

[Bjorn Helgaas <helgaas@kernel.org>]

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@linux.intel.com>
> ---
>  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
>