Andy, thank you for resending the comments. I will reflect all of your comments
to the next version. Please find a couple of comments below in line.
On Dec 14, 2023 / 18:38, Andy Shevchenko wrote:
> 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.)
I'm not sure where are other definitions. I guess PCI_CONF1_FUNC_SHIFT in
drivers/pci/pci.h is one of them. As you suggested in another mail, I'll add a
TODO comment and note that the NR_P2SB_RES_CACHE should be refactored later.
>
> ...
>
> > +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?
I will add a global variable "static struct pci_bus *p2sb_bus". It will keep the
first call return value and allow to skip the second call.
>
> ...
>
> > + /*
> > + * 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
>
>
