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

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

On Thu, Dec 21, 2023 at 06:39:36PM +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.

Thank you for the update!
My comments below.

...

> +/*
> + * Cache BAR0 of P2SB device functions 0 to 7
> + * TODO: Move this definition to pci.h together with same other definitions

Missing periods at the end of sentences.

> + */
> +#define NR_P2SB_RES_CACHE 8

...

> +static bool p2sb_valid_resource(struct resource *res)
> +{
> +	return res->flags;
> +}

So, now if you look at this, there is no point in having the function.
But see below.

...

> +	if (!p2sb_valid_resource(pci_resource_n(pdev, 0)))
> +		return -ENOENT;

As per above (i.e. see below).

...

> +	ret = p2sb_scan_and_cache_devfn(bus, devfn);
> +	if (ret || PCI_FUNC(devfn) != 0)
> +		return ret;

This hides the fact that we don't scan functions if P2SB is not 0.

	if (ret)
		return ret;

	/* ...add a comment like above... */
	if (PCI_FUNC(devfn) != 0)
		return 0;


> +	/*
> +	 * When function number of the P2SB device is zero, scan other function

The first part will become the comment above. So, you may drop it from here.

> +	 * numbers. If devices are available, cache their BAR0.
> +	 */

...

> +		pci_bus_write_config_dword(p2sb_bus, devfn_p2sb, P2SBC,
> +					   P2SBC_HIDE);

Having it on one line is fine.

...

> +	if (!bus)
> +		bus = p2sb_bus;

Hmm... Maybe

static struct pci_bus *p2sb_get_bus(bus)
{
	static struct pci_bus *p2sb_bus;

	bus = bus ?: p2sb_bus;
	if (bus)
		return bus;

	p2sb_bus = ...;
	return p2sb_bus;
}

?

...

> +	if (!devfn) {

Maybe move this check to the callee?

> +		ret = p2sb_get_devfn(&devfn);
> +		if (ret)
> +			return ret;
> +	}

...

> +	cache = &p2sb_resources[PCI_FUNC(devfn)];
> +	if (!p2sb_valid_resource(&cache->res) ||
> +	    cache->bus_dev_id != bus->dev.id)
>  		return -ENODEV;

I don't remember if I mentioned in the last email(s), but I think what you want is

static int p2sb_valid_resource(struct resource *res)
{
	return res->flags ? 0 : -ENOENT;
}
	...
	cache = &p2sb_resources[PCI_FUNC(devfn)];
	if (cache->bus_dev_id != bus->dev.id)
		return -ENODEV;

	ret = p2sb_valid_resource(&cache->res);
	if (ret)
		return ret;

-- 
With Best Regards,
Andy Shevchenko