Re: [PATCH v2] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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