Re: [PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Hans de Goede <hdegoede@redhat.com>]

Hi,

Seems I was a bit too quick with reviewing at a second
look I have found a small issue with this patch.

See my comment below.

On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:
> When drivers access P2SB device resources, it calls p2sb_bar(). Before
> the commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
> during PCI device probe"), p2sb_bar() obtained the resources and then
> called pci_stop_and_remove_bus_device() for clean up. Then the P2SB
> device disappeared. The commit 5913320eb0b3 introduced the P2SB device
> resource cache feature in the boot process. During the resource cache,
> pci_stop_and_remove_bus_device() is called for the P2SB device, then the
> P2SB device disappears regardless of whether p2sb_bar() is called or
> not. Such P2SB device disappearance caused a confusion [1]. To avoid the
> confusion, avoid the pci_stop_and_remove_bus_device() call when the BIOS
> does not hide the P2SB device.
> 
> For that purpose, cache the P2SB device resources only if the BIOS hides
> the P2SB device. Call p2sb_scan_and_cache() only if p2sb_hidden_by_bios
> is true. This allows removing two branches from p2sb_scan_and_cache().
> When p2sb_bar() is called, get the resources from the cache if the P2SB
> device is hidden. Otherwise, read the resources from the unhidden P2SB
> device.
> 
> Reported-by: "Daniel Walker (danielwa)" <danielwa@cisco.com>
> Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
> Closes: https://lore.kernel.org/lkml/ZzTI+biIUTvFT6NC@goliath/ [1]
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/platform/x86/p2sb.c | 40 +++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
> index 0bc6b21c4c20..1b2d83c4b02a 100644
> --- a/drivers/platform/x86/p2sb.c
> +++ b/drivers/platform/x86/p2sb.c
> @@ -100,10 +100,8 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
>  	/*
>  	 * The BIOS prevents the P2SB device from being enumerated by the PCI
>  	 * subsystem, so we need to unhide and hide it back to lookup the BAR.
> -	 * Unhide the P2SB device here, if needed.
>  	 */
> -	if (p2sb_hidden_by_bios)
> -		pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
> +	pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
>  
>  	/* Scan the P2SB device and cache its BAR0 */
>  	p2sb_scan_and_cache_devfn(bus, devfn);
> @@ -112,9 +110,7 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
>  	if (devfn == P2SB_DEVFN_GOLDMONT)
>  		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
>  
> -	/* Hide the P2SB device, if it was hidden */
> -	if (p2sb_hidden_by_bios)
> -		pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
> +	pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
>  
>  	if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
>  		return -ENOENT;
> @@ -167,7 +163,12 @@ static int p2sb_cache_resources(void)
>  	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
>  	p2sb_hidden_by_bios = value & P2SBC_HIDE;
>  
> -	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> +	/*
> +	 * If the BIOS does not hide the P2SB device then its resources
> +	 * are accesilble. Cache them only if the P2SB device is hidden.
> +	 */
> +	if (p2sb_hidden_by_bios)
> +		ret = p2sb_scan_and_cache(bus, devfn_p2sb);

ret will be returned uninitialized now when p2sb_hidden_by_bios is false,
so this patch also needs to initialize ret to 0 when declaring it.

With this fixed you can keep my Reviewed-by.

Regards,

Hans



>  
>  	pci_unlock_rescan_remove();
>  
> @@ -190,6 +191,26 @@ static int p2sb_read_from_cache(struct pci_bus *bus, unsigned int devfn,
>  	return 0;
>  }
>  
> +static int p2sb_read_from_dev(struct pci_bus *bus, unsigned int devfn,
> +			      struct resource *mem)
> +{
> +	struct pci_dev *pdev;
> +	int ret = 0;
> +
> +	pdev = pci_get_slot(bus, devfn);
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	if (p2sb_valid_resource(pci_resource_n(pdev, 0)))
> +		p2sb_read_bar0(pdev, mem);
> +	else
> +		ret = -ENOENT;
> +
> +	pci_dev_put(pdev);
> +
> +	return ret;
> +}
> +
>  /**
>   * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
>   * @bus: PCI bus to communicate with
> @@ -213,7 +234,10 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
>  	if (!devfn)
>  		p2sb_get_devfn(&devfn);
>  
> -	return p2sb_read_from_cache(bus, devfn, mem);
> +	if (p2sb_hidden_by_bios)
> +		return p2sb_read_from_cache(bus, devfn, mem);
> +
> +	return p2sb_read_from_dev(bus, devfn, mem);
>  }
>  EXPORT_SYMBOL_GPL(p2sb_bar);
>