Hi Shinichiro, On 3/4/24 10:55 AM, Hans de Goede wrote: > Hi, > > On 3/4/24 4:19 AM, Shinichiro Kawasaki wrote: >> On Mar 03, 2024 / 20:35, Hans de Goede wrote: <snip> >>> 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. I have just taken a second look at my analysis of when p2sb_bar(devfn=0) is called on Goldmont platforms and my analysis for: 1) edac/pnd2_edac devfn = 0 was wrong, p2sb_bar(devfn=0) is only used on Denverton (aka "Goldmont D") which uses the default P2SB_DEVFN_DEFAULT = PCI_DEVFN(31, 1) devfn not the special Goldmont devfn. So the p2sb_bar(devfn=0) call actually only happens on Goldmont from: 3) mfd/lpc_ich for pinctrl devfn = 0 and then only when the ACPI tables fail to properly describe the GPIO controllers as ACPI devices, if the GPIO controllers are described in ACPI, which they are on the ASUS VivoBook D540NV-GQ065T then that call gets skipped. So on the ASUS VivoBook D540NV-GQ065T p2sb_bar(devfn=0) gets never called. Which explains why not caching it fixes things. I assume that this laptop just seems not likes the P2SB is touched at all and by not caching the BAR for the P2SB it ends up not getting touched at all. This also means that likely the P2SB devfn itself generally speaking is often not touched on Goldmont platforms. So we can avoid the lockdep issue on PCI bus rescan there by caching the SPI controller PCI_DEVFN(13, 2) devfn from fs_initcall(), since that will be the only devfn for which p2sb_bar() will get called (except on hw with the GPIO controller missing from the ACPI tables which should be rare). I have prepared a follow up patch to your v3 to cache the SPI controller devfn instead of the P2SB devfn at fs_initcall() time. I'll post this shortly and I'll also ask the bug reporter to test the combination of our 2 patches. Regards, Hans
