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@xxxxxxxxx>
> 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@xxxxxxx>
> ---
> 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);
>
