Hi, On 3/4/24 4:19 AM, Shinichiro Kawasaki wrote: > On Mar 03, 2024 / 20:35, Hans de Goede wrote: >> Hi Shinichiro, >> >> On 3/3/24 00:37, Shinichiro Kawasaki wrote: <snip> >> So I have taken a quick look at your latest patch from: >> https://bugzilla.kernel.org/show_bug.cgi?id=218531 >> >> I think that skipping the caching at fs_initcall() on goldmont >> is a good idea. >> >> But you still cache *all* the bars for goldmont on the first >> p2sb_bar(bus, 0, &res) call . >> >> If we delay caching the bars till there first use, why not >> just do that for all the bars and also drop p2sb_scan_and_cache() >> which for non goldmont is equivalent to p2sb_scan_and_cache_devfn() >> but on goldmont caches all the functions. >> >> Since you now delay caching (on goldmont) to the first p2sb_bar() >> call I think that you can just drop p2sb_scan_and_cache() >> altogether and just directly call p2sb_scan_and_cache_devfn() >> in its place. >> >> This means that on goldmont where both the p2sb devfn >> PCI_DEVFN(13, 0) and the SPI controller PCI_DEVFN(13, 2) >> are used we end up going through p2sb_cache_resources() >> twice, assuming both are actually requested at least once. >> But with your current patch this will also happen when >> PCI_DEVFN(13, 2) gets requested first because then >> p2sb_scan_and_cache() will enter the "not function 0" >> path and only cache the one resource. >> >> So I think that it would make things more KISS if >> p2sb_bar() always only caches the requested devfn bar0 >> instead of treating function0 special as it does now. > > Thank you again for looking into the patch. I agree that the "function 0" path > in p2sb_scan_and_cache() is not meaningful any more. When I prepare v3 patch, > I will modify the patch to call p2sb_scan_and_cache_devfn() in place of > p2sb_scan_and_cache(). I've seen you've posted v3, this looks good, thanks. >> Also talking about making things more KISS, how >> about completely dropping the fs_initcall and >> simply always delay the caching of a devfn until >> the first call of p2sb_bar() for that devfn ? >> >> That way fixing the issue will also actually reduce / >> simplify the code :) > > This will simplify the code more, but it has two drawabacks: > > 1) It still leaves the rare deadlock scenario. If the drivers which call > p2sb_bar() are not probed during boot, and if they are probed afterwards by > sysfs pci bus rescan, pci_rescan_remove_lock causes the deadlock. > > 2) It triggers lockdep splat for pci_rescan_remove_lock at sysfs pci bus rescan, > even for devices unrelated to p2sb (This is what I regularly observe during > kernel tests for storage sub-system.) > > I suggest to limit these drawbacks only on goldmont. I was not aware of the lockdep triggering issue. I agree that should be avoided when possible. I see you have kept the fs_initcall() for this in v3, good. Regards, Hans
