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

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

On Tue, Dec 12, 2023 at 08:47:46PM +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.

...

> +/* Cache BAR0 of P2SB device from function 0 ot 7 */
> +#define NR_P2SB_RES_CACHE 8

This is fifth or so definition for the same, isn't it a good time to create
a treewide definition in pci.h?

See also below.

(In previous mail I even found all cases and listed, a bit lazy to repeat.)

...

> +static bool p2sb_invalid_resource(struct resource *res)

The naming is better to be p2sb_is_resource_valid().

...

>  	struct resource *bar0 = &pdev->resource[0];

This and in new code can use pci_resource_n() macro.

...

>  	pdev = pci_scan_single_device(bus, devfn);
> -	if (!pdev)
> +	if (!pdev || p2sb_invalid_resource(&pdev->resource[0]))
>  		return -ENODEV;

I prefer to split and have different error code for the second one:
-ENOENT / -EINVAL / etc.

...

> +	struct pci_bus *bus;
> +	unsigned int devfn_p2sb, slot_p2sb, fn;

Please, preserve reversed xmas tree ordering.

>  	u32 value = P2SBC_HIDE;
>  	int ret;

...

> -	/* if @bus is NULL, use bus 0 in domain 0 */
> -	bus = bus ?: pci_find_bus(0, 0);
> +	/* Assume P2SB is on the bus 0 in domain 0 */
> +	bus = pci_find_bus(0, 0);

The pci_find_bus() is called in two places now. Can we avoid doing
this duplication?

...

> +	/*
> +	 * When function number of the P2SB device is zero, scan other function
> +	 * numbers. If devices are available, cache their BAR0.
> +	 */
> +	if (!PCI_FUNC(devfn_p2sb)) {

I prefer to see '== 0' to make it clear that 0 has the same semantics as other
numbers here. It's not special like NULL.

> +		slot_p2sb = PCI_SLOT(devfn_p2sb);
> +		for (fn = 1; fn < 8; fn++)

As per above, use a definition for 8

> +			p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn));
> +	}
> +
> +out:

Can it be split the above to the previous call or a separate helper?

...

> +static int __init p2sb_fs_init(void)
> +{
> +	p2sb_cache_resources();
> +	return 0;
> +}

Please, add a comment justifying fs_initcall().

> +fs_initcall(p2sb_fs_init);

-- 
With Best Regards,
Andy Shevchenko