On Fri, Dec 22, 2023 at 04:20:54PM +0900, Shin'ichiro Kawasaki wrote:
> p2sb_bar() unhides P2SB device to get resources from the device. It
> guards the operation by locking pci_rescan_remove_lock so that parallel
> rescans do not find the P2SB device. However, this lock causes deadlock
> when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan
> locks pci_rescan_remove_lock and probes PCI devices. When PCI devices
> call p2sb_bar() during probe, it locks pci_rescan_remove_lock again.
> Hence the deadlock.
>
> To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar().
> Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources()
> for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(),
> refer the cache and return to the caller.
Thanks for this version!
My (mostly cosmetic) comments below. I believe next version will be good
enough to go with.
...
> +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> +{
> + unsigned int slot, fn;
> + int ret;
> +
> + /* Scan the P2SB device and cache its BAR0 */
> + ret = p2sb_scan_and_cache_devfn(bus, devfn);
> + if (ret)
> + return ret;
> +
> + /*
> + * When function number of the P2SB device is zero, scan other function
> + * numbers.
> + */
> + if (PCI_FUNC(devfn) != 0)
> + return 0;
> +
> + /* If devices are available, cache their BAR0 */
> + slot = PCI_SLOT(devfn);
> + for (fn = 1; fn < NR_P2SB_RES_CACHE; fn++)
> + p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn));
> +
> + return 0;
> +}
So, we can actually do the for-loop from index 0.
if (PCI_FUNC(devfn) == 0) {
/*
* When function number of the P2SB device is zero, scan it
* and other function numbers, and if devices are available,
* cache their BAR0s.
*/
slot = PCI_SLOT(devfn);
// This will allow us to have something like pci_dev_for_each_func() in the future.
for (fn = 0; fn < NR_P2SB_RES_CACHE; fn++)
p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn));
} else {
/* Scan the P2SB device and cache its BAR0 */
ret = p2sb_scan_and_cache_devfn(bus, devfn);
if (ret)
return ret;
}
...
> + static struct pci_bus *bus;
The "static" word is not needed.
> + bus = p2sb_get_bus(NULL);
> + if (!bus)
> + return -ENODEV;
...
> /* Hide the P2SB device, if it was hidden */
> if (value & P2SBC_HIDE)
> pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
> -
Stray change.
> pci_unlock_rescan_remove();
--
With Best Regards,
Andy Shevchenko
