| Message ID | 20241125042326.304780-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] p2sb: Do not scan and remove the P2SB device when it is unhidden | expand |
On Mon, Nov 25, 2024 at 01:23:26PM +0900, 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 P2SB > device is hidden. When p2sb_cache_resources() is called, check if the > device is hidden and store the result in the global flag p2sb_hidden. > Check the flag in p2sb_bar() and if the device is hidden, refer to the > cached resources. Otherwise, read the resources from the unhidden P2SB > device. ... > +static bool p2sb_hidden; I would call it p2sb_was_hidden or p2sb_hidden_by_bios. > + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); > ret = p2sb_scan_and_cache(bus, devfn_p2sb); > + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); We already pass the devfn_p2sb to that call, perhaps you can simply move these configuration writes there? > - /* Hide the P2SB device, if it was hidden */ > - if (value & P2SBC_HIDE) > - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); Thinking of how this change is done, I'm wondering if we better split it to some preliminary refactoring to make it clearer of what's going on on each step. Note, it's not a problem to have a series of patches to fix something and it does not rarely occur (I believe Hans had done this many times in the past). ... > - cache = &p2sb_resources[PCI_FUNC(devfn)]; > - if (cache->bus_dev_id != bus->dev.id) > - return -ENODEV; > + /* > + * If the P2SB device is hidden, refer to the cached resources. > + * Otherwise, read the resources on the fly. > + */ > + if (p2sb_hidden) { > + cache = &p2sb_resources[PCI_FUNC(devfn)]; > + if (cache->bus_dev_id != bus->dev.id) > + return -ENODEV; > > - if (!p2sb_valid_resource(&cache->res)) > - return -ENOENT; > + if (!p2sb_valid_resource(&cache->res)) > + return -ENOENT; > > - memcpy(mem, &cache->res, sizeof(*mem)); > - return 0; > + memcpy(mem, &cache->res, sizeof(*mem)); > + } else { > + 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); > + } Can you split these two branches to two helper functions. In the result it will look better in my opinion.
On Nov 25, 2024 / 13:02, Andy Shevchenko wrote: > On Mon, Nov 25, 2024 at 01:23:26PM +0900, 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 P2SB > > device is hidden. When p2sb_cache_resources() is called, check if the > > device is hidden and store the result in the global flag p2sb_hidden. > > Check the flag in p2sb_bar() and if the device is hidden, refer to the > > cached resources. Otherwise, read the resources from the unhidden P2SB > > device. > > ... > > > +static bool p2sb_hidden; > > I would call it p2sb_was_hidden or p2sb_hidden_by_bios. Okay, will rename it. > > > + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); > > ret = p2sb_scan_and_cache(bus, devfn_p2sb); > > + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); > > We already pass the devfn_p2sb to that call, perhaps you can simply move these > configuration writes there? I see, will move the two pci_bus_write_config_dword() calls to p2sb_scan_and_cache(). > > > - /* Hide the P2SB device, if it was hidden */ > > - if (value & P2SBC_HIDE) > > - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); > > > Thinking of how this change is done, I'm wondering if we better split it to > some preliminary refactoring to make it clearer of what's going on on each > step. Note, it's not a problem to have a series of patches to fix something and > it does not rarely occur (I believe Hans had done this many times in the past). I see, I will try it with v3. > > ... > > > - cache = &p2sb_resources[PCI_FUNC(devfn)]; > > - if (cache->bus_dev_id != bus->dev.id) > > - return -ENODEV; > > + /* > > + * If the P2SB device is hidden, refer to the cached resources. > > + * Otherwise, read the resources on the fly. > > + */ > > + if (p2sb_hidden) { > > + cache = &p2sb_resources[PCI_FUNC(devfn)]; > > + if (cache->bus_dev_id != bus->dev.id) > > + return -ENODEV; > > > > - if (!p2sb_valid_resource(&cache->res)) > > - return -ENOENT; > > + if (!p2sb_valid_resource(&cache->res)) > > + return -ENOENT; > > > > - memcpy(mem, &cache->res, sizeof(*mem)); > > - return 0; > > + memcpy(mem, &cache->res, sizeof(*mem)); > > + } else { > > + 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); > > + } > > Can you split these two branches to two helper functions. In the result it will > look better in my opinion. Okay, will do. Thanks for the comments.
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c index 31f38309b389..0b1d604fcfe5 100644 --- a/drivers/platform/x86/p2sb.c +++ b/drivers/platform/x86/p2sb.c @@ -42,6 +42,7 @@ struct p2sb_res_cache { }; static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE]; +static bool p2sb_hidden; static void p2sb_get_devfn(unsigned int *devfn) { @@ -152,20 +153,23 @@ static int p2sb_cache_resources(void) pci_lock_rescan_remove(); /* - * 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. + * The BIOS does not hide the P2SB device then its resources are + * accesilble. Do not cache them. */ pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); - if (value & P2SBC_HIDE) - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); + p2sb_hidden = value & P2SBC_HIDE; + if (!p2sb_hidden) + goto unlock; + /* + * 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. + */ + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); ret = p2sb_scan_and_cache(bus, devfn_p2sb); + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); - /* Hide the P2SB device, if it was hidden */ - if (value & P2SBC_HIDE) - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); - +unlock: pci_unlock_rescan_remove(); return ret; @@ -188,6 +192,8 @@ static int p2sb_cache_resources(void) int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) { struct p2sb_res_cache *cache; + struct pci_dev *pdev; + int ret = 0; bus = p2sb_get_bus(bus); if (!bus) @@ -196,15 +202,33 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) if (!devfn) p2sb_get_devfn(&devfn); - cache = &p2sb_resources[PCI_FUNC(devfn)]; - if (cache->bus_dev_id != bus->dev.id) - return -ENODEV; + /* + * If the P2SB device is hidden, refer to the cached resources. + * Otherwise, read the resources on the fly. + */ + if (p2sb_hidden) { + cache = &p2sb_resources[PCI_FUNC(devfn)]; + if (cache->bus_dev_id != bus->dev.id) + return -ENODEV; - if (!p2sb_valid_resource(&cache->res)) - return -ENOENT; + if (!p2sb_valid_resource(&cache->res)) + return -ENOENT; - memcpy(mem, &cache->res, sizeof(*mem)); - return 0; + memcpy(mem, &cache->res, sizeof(*mem)); + } else { + 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; } EXPORT_SYMBOL_GPL(p2sb_bar);
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 P2SB device is hidden. When p2sb_cache_resources() is called, check if the device is hidden and store the result in the global flag p2sb_hidden. Check the flag in p2sb_bar() and if the device is hidden, refer to the cached resources. 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> --- Changes from v1: * Put back P2SBC_HIDE flag reference code in the rescan_remove lock region * Do not cache resources when the P2SB device is not hidden * Added the Reported-by tag drivers/platform/x86/p2sb.c | 56 ++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 16 deletions(-)