| Message ID | 20241120064055.245969-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | p2sb: Do not scan and remove the P2SB device when it is hidden | expand |
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();
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c index 31f38309b389..43b712b7a1ea 100644 --- a/drivers/platform/x86/p2sb.c +++ b/drivers/platform/x86/p2sb.c @@ -79,29 +79,37 @@ static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) mem->desc = bar0->desc; } -static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn) +static void __p2sb_cache_devfn(struct pci_bus *bus, unsigned int devfn, + bool scan) { struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; struct pci_dev *pdev; - pdev = pci_scan_single_device(bus, devfn); + if (scan) + pdev = pci_scan_single_device(bus, devfn); + else + pdev = pci_get_slot(bus, devfn); + if (!pdev) return; p2sb_read_bar0(pdev, &cache->res); cache->bus_dev_id = bus->dev.id; - pci_stop_and_remove_bus_device(pdev); + if (scan) + pci_stop_and_remove_bus_device(pdev); + else + pci_dev_put(pdev); } -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); /* 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; @@ -109,6 +117,16 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) return 0; } +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) +{ + return p2sb_cache_devfn(bus, devfn, true); +} + +static int p2sb_read_and_cache(struct pci_bus *bus, unsigned int devfn) +{ + return p2sb_cache_devfn(bus, devfn, false); +} + static struct pci_bus *p2sb_get_bus(struct pci_bus *bus) { static struct pci_bus *p2sb_bus; @@ -145,6 +163,14 @@ static int p2sb_cache_resources(void) if (!PCI_POSSIBLE_ERROR(class) && class != PCI_CLASS_MEMORY_OTHER) return -ENODEV; + /* + * The BIOS does not hide the P2SB device then it should have been + * enumerated by the PCI subsystem. Get the resources from it. + */ + pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); + if ((value & P2SBC_HIDE) != P2SBC_HIDE) + return p2sb_read_and_cache(bus, devfn_p2sb); + /* * Prevent concurrent PCI bus scan from seeing the P2SB device and * removing via sysfs while it is temporarily exposed. @@ -154,17 +180,10 @@ static int p2sb_cache_resources(void) /* * 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. */ - 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); - - /* 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();
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(). 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 | 51 +++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 16 deletions(-)