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
