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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 
> 




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux