Re: [PATCH v2] platform/x86: p2sb: Defer P2SB device scan when P2SB device has func 0

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
> On Mar 04, 2024 / 12:04, Hans de Goede wrote:
> ...
>> 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.
> 
> Thanks for sharing the insights.

Looking closer at the actual unhiding I don't think accessing func 0
is the problem. The unhiding is always done on function 0 even when
retreiving the bar for function 2 (the SPI function).

So taking that into account, as mentioned in the bugzilla, I think
the problem is probing the other functions (1, 3-7) by calling
pci_scan_single_device() on them.

I have prepared an alternative fix based on this and asked
Danilrybakov to test that in the bugzilla.

(and also posted it as a RFC to the list)

>> 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).
> 
> Oh, this sounds a great idea.
> 
>>
>> 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.
> 
> Thanks a lot. Looking forward to the results.

Since I already had the follow-up patch to your v3 implementing
this ready I've also send this to the list as RFC.

I hope the patch to only cache 13,0 + 13,2 at fs_initcall()
time works. Then we still avoid all possible deadlock /
lockdep scenarios which would be nice.

Regards,

Hans