On Wed, Nov 20, 2024 at 03:40:55PM +0900, Shin'ichiro Kawasaki wrote:
The subject probably wants to say "unhidden".
> When drivers access P2SB device resources, it calls p2sb_bar() to obtain
> the resources. 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 even when the BIOS does not hide
> the P2SB device. The commit introduced the P2SB device resource cache
> feature in the boot process. During the resource cache process,
> 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, add a new helper function p2sb_read_and_cache(), which
> does the same work as p2sb_scan_and_cache() but does not call
> pci_stop_and_remove_bus_device(). These two functions are almost same
> except the device scan and remove. Then make them call the single
> function p2sb_cache_devfn(), which takes the argument "bool scan".
>
> If the BIOS does not hide the P2SB device, just call
> p2sb_read_and_cache() to cache the resources. If not, do additional
> preparations (lock "rescan remove" for parallel scan, and unhide the
> P2SB device), then call p2sb_scan_and_cache().
Even for the simple read case we have to do that under rescan lock.
Yes, it might be just a theoretical issue, but that's how makes code
robust against possible enumeration changes at boot time.
...
> -static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> +static int p2sb_cache_devfn(struct pci_bus *bus, unsigned int devfn, bool scan)
> {
> - /* Scan the P2SB device and cache its BAR0 */
> - p2sb_scan_and_cache_devfn(bus, devfn);
> + /* Scan or read the P2SB device and cache its BAR0 */
> + __p2sb_cache_devfn(bus, devfn, scan);
Strictly speaking we don't need to cache values when the device is unhidden.
Moreover, the rescan can happen in between and the resource relocation to
another place, which makes cached value invalid.
> /* On Goldmont p2sb_bar() also gets called for the SPI controller */
> if (devfn == P2SB_DEVFN_GOLDMONT)
> - p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
> + __p2sb_cache_devfn(bus, SPI_DEVFN_GOLDMONT, scan);
>
> if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
> return -ENOENT;
> return 0;
> }
...
> + pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> + if ((value & P2SBC_HIDE) != P2SBC_HIDE)
> + return p2sb_read_and_cache(bus, devfn_p2sb);
This still has to be under rescan lock.
...
> - pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> - if (value & P2SBC_HIDE)
> - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> -
> + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> ret = p2sb_scan_and_cache(bus, devfn_p2sb);
That's why I proposed initially to have a conditional here, but see above,
it looks like the correct approach is to cache values if and only if the BIOS
hides the p2sb.
> - /* Hide the P2SB device, if it was hidden */
> - if (value & P2SBC_HIDE)
> - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
> + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
>
> pci_unlock_rescan_remove();
--
With Best Regards,
Andy Shevchenko
