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

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

[Shin'ichiro Kawasaki]

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.

Link: https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
This patch reflects discussions held at the Link tag. I confirmed this patch
fixes the problem using a system with i2c_i801 device, building i2c_i801
module as both built-in and loadable. Reviews will be appreicated.

Changes from v1:
* Reflected review comments by Andy

Changes from RFC v2:
* Reflected review comments on the list
* Removed RFC prefix

Changes from RFC v1:
* Fixed a build warning poitned out in llvm list by kernel test robot

 drivers/platform/x86/p2sb.c | 172 +++++++++++++++++++++++++++---------
 1 file changed, 131 insertions(+), 41 deletions(-)

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 1cf2471d54dd..9693ed065782 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -26,6 +26,19 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = {
 	{}
 };
 
+/*
+ * Cache BAR0 of P2SB device functions 0 to 7.
+ * TODO: Move this definition to pci.h together with same other definitions.
+ */
+#define NR_P2SB_RES_CACHE 8
+
+struct p2sb_res_cache {
+	u32 bus_dev_id;
+	struct resource res;
+};
+
+static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE];
+
 static int p2sb_get_devfn(unsigned int *devfn)
 {
 	unsigned int fn = P2SB_DEVFN_DEFAULT;
@@ -39,10 +52,15 @@ static int p2sb_get_devfn(unsigned int *devfn)
 	return 0;
 }
 
+static int p2sb_valid_resource(struct resource *res)
+{
+	return res->flags ? 0 : -ENOENT;
+}
+
 /* Copy resource from the first BAR of the device in question */
-static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
+static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
 {
-	struct resource *bar0 = &pdev->resource[0];
+	struct resource *bar0 = pci_resource_n(pdev, 0);
 
 	/* Make sure we have no dangling pointers in the output */
 	memset(mem, 0, sizeof(*mem));
@@ -56,12 +74,11 @@ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
 	mem->end = bar0->end;
 	mem->flags = bar0->flags;
 	mem->desc = bar0->desc;
-
-	return 0;
 }
 
-static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+static int p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn)
 {
+	struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)];
 	struct pci_dev *pdev;
 	int ret;
 
@@ -69,34 +86,58 @@ static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct re
 	if (!pdev)
 		return -ENODEV;
 
-	ret = p2sb_read_bar0(pdev, mem);
+	ret = p2sb_valid_resource(pci_resource_n(pdev, 0));
+	if (ret)
+		return ret;
+
+	p2sb_read_bar0(pdev, &cache->res);
+	cache->bus_dev_id = bus->dev.id;
 
 	pci_stop_and_remove_bus_device(pdev);
-	return ret;
+	return 0;
 }
 
-/**
- * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
- * @bus: PCI bus to communicate with
- * @devfn: PCI slot and function to communicate with
- * @mem: memory resource to be filled in
- *
- * 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.
- *
- * if @bus is NULL, the bus 0 in domain 0 will be used.
- * If @devfn is 0, it will be replaced by devfn of the P2SB device.
- *
- * Caller must provide a valid pointer to @mem.
- *
- * Locking is handled by pci_rescan_remove_lock mutex.
- *
- * Return:
- * 0 on success or appropriate errno value on error.
- */
-int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+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;
+}
+
+static struct pci_bus *p2sb_get_bus(struct pci_bus *bus)
+{
+	static struct pci_bus *p2sb_bus;
+
+	bus = bus ?: p2sb_bus;
+	if (bus)
+		return bus;
+
+	/* Assume P2SB is on the bus 0 in domain 0 */
+	p2sb_bus = pci_find_bus(0, 0);
+	return p2sb_bus;
+}
+
+static int p2sb_cache_resources(void)
 {
-	struct pci_dev *pdev_p2sb;
+	static struct pci_bus *bus;
 	unsigned int devfn_p2sb;
 	u32 value = P2SBC_HIDE;
 	int ret;
@@ -106,8 +147,9 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 	if (ret)
 		return ret;
 
-	/* if @bus is NULL, use bus 0 in domain 0 */
-	bus = bus ?: pci_find_bus(0, 0);
+	bus = p2sb_get_bus(NULL);
+	if (!bus)
+		return -ENODEV;
 
 	/*
 	 * Prevent concurrent PCI bus scan from seeing the P2SB device and
@@ -115,30 +157,78 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 	 */
 	pci_lock_rescan_remove();
 
-	/* Unhide the P2SB device, if needed */
+	/*
+	 * 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);
 
-	pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb);
-	if (devfn)
-		ret = p2sb_scan_and_read(bus, devfn, mem);
-	else
-		ret = p2sb_read_bar0(pdev_p2sb, mem);
-	pci_stop_and_remove_bus_device(pdev_p2sb);
+	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_unlock_rescan_remove();
 
-	if (ret)
-		return ret;
+	return ret;
+}
+
+/**
+ * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
+ * @bus: PCI bus to communicate with
+ * @devfn: PCI slot and function to communicate with
+ * @mem: memory resource to be filled in
+ *
+ * If @bus is NULL, the bus 0 in domain 0 will be used.
+ * If @devfn is 0, it will be replaced by devfn of the P2SB device.
+ *
+ * Caller must provide a valid pointer to @mem.
+ *
+ * Return:
+ * 0 on success or appropriate errno value on error.
+ */
+int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+{
+	struct p2sb_res_cache *cache;
+	int ret;
+
+	bus = p2sb_get_bus(bus);
+	if (!bus)
+		return -ENODEV;
 
-	if (mem->flags == 0)
+	if (!devfn) {
+		ret = p2sb_get_devfn(&devfn);
+		if (ret)
+			return ret;
+	}
+
+	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;
+
+	memcpy(mem, &cache->res, sizeof(*mem));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(p2sb_bar);
+
+static int __init p2sb_fs_init(void)
+{
+	p2sb_cache_resources();
+	return 0;
+}
+
+/*
+ * pci_rescan_remove_lock avoids access to unhidden P2SB devices, but it causes
+ * deadlock with sysfs pci bus rescan. To avoid the deadlock, access to P2SB
+ * devices at an early step in kernel initialization and cache required
+ * resources. This should happen after subsys_initcall which initializes PCI
+ * subsystem and before device_initcall which requires P2SB resources.
+ */
+fs_initcall(p2sb_fs_init);
-- 
2.43.0

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 04:25:10PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 22, 2023 at 04:20:54PM +0900, Shin'ichiro Kawasaki wrote:

...

> > +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;
> 	}

Even going further

	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);
		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 */
		p2sb_scan_and_cache_devfn(bus, devfn);
	}

	return p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)]);

// With this it's maybe p2sb_scan_and_cache_devfn() can be void.

-- 
With Best Regards,
Andy Shevchenko

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

[Shinichiro Kawasaki]

On Dec 22, 2023 / 16:36, Andy Shevchenko wrote:
> On Fri, Dec 22, 2023 at 04:25:10PM +0200, Andy Shevchenko wrote:
> > On Fri, Dec 22, 2023 at 04:20:54PM +0900, Shin'ichiro Kawasaki wrote:
> 
> ...
> 
> > > +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;
> > 	}
> 
> Even going further
> 
> 	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);
> 		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 */
> 		p2sb_scan_and_cache_devfn(bus, devfn);
> 	}
> 
> 	return p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)]);
> 
> // With this it's maybe p2sb_scan_and_cache_devfn() can be void.

Thanks. This will make p2sb_scan_and_cache() simpler. Will reflect it to v3
together with other your comments. I will also simplify
p2sb_scan_and_cache_devfn() to return void and skip p2sb_valid_resource() call.