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

Andy Shevchenko
2023-12-14T16:38:47Z

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