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

The commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
during PCI device probe") triggered repeated ACPI errors on ASUS
VivoBook D540NV-GQ065T [1]. It was confirmed that the P2SB device scan
and remove at the fs_initcall stage triggered the errors.

To avoid the error, defer the P2SB device scan on the concerned device.
The error was observed on the system with Pentium N4200 in Goldmont micro-
architecture, and on which P2SB has function 0. Then refer to the P2SB
function to decide whether to defer or not.

When the device scan is deferred, do the scan later when p2sb_bar() is
called for the first time. If this first scan is triggered by sysfs
pci bus rescan, deadlock happens. In most cases, the scan happens during
system boot process, then there is no chance of deadlock.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=218531 [1]
Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
Changes from v1:
* Removed unnecessary p2sb_resource_cached()
* Reflected other review comments

 drivers/platform/x86/p2sb.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

On Mar 02, 2024 / 10:28, Shin'ichiro Kawasaki wrote:
> The commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
> during PCI device probe") triggered repeated ACPI errors on ASUS
> VivoBook D540NV-GQ065T [1]. It was confirmed that the P2SB device scan
> and remove at the fs_initcall stage triggered the errors.
> 
> To avoid the error, defer the P2SB device scan on the concerned device.
> The error was observed on the system with Pentium N4200 in Goldmont micro-
> architecture, and on which P2SB has function 0. Then refer to the P2SB
> function to decide whether to defer or not.
> 
> When the device scan is deferred, do the scan later when p2sb_bar() is
> called for the first time. If this first scan is triggered by sysfs
> pci bus rescan, deadlock happens. In most cases, the scan happens during
> system boot process, then there is no chance of deadlock.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218531 [1]
> Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Let me drop this patch. danilrybakov found that the ACPI errors are still
reported even with this patch. Will try another fix approach.

Hi Shinichiro,

Thank you for your work on this.

On 3/2/24 08:28, Shinichiro Kawasaki wrote:
> On Mar 02, 2024 / 10:28, Shin'ichiro Kawasaki wrote:
>> The commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
>> during PCI device probe") triggered repeated ACPI errors on ASUS
>> VivoBook D540NV-GQ065T [1]. It was confirmed that the P2SB device scan
>> and remove at the fs_initcall stage triggered the errors.
>>
>> To avoid the error, defer the P2SB device scan on the concerned device.
>> The error was observed on the system with Pentium N4200 in Goldmont micro-
>> architecture, and on which P2SB has function 0. Then refer to the P2SB
>> function to decide whether to defer or not.
>>
>> When the device scan is deferred, do the scan later when p2sb_bar() is
>> called for the first time. If this first scan is triggered by sysfs
>> pci bus rescan, deadlock happens. In most cases, the scan happens during
>> system boot process, then there is no chance of deadlock.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218531 [1]
>> Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> 
> Let me drop this patch. danilrybakov found that the ACPI errors are still
> reported even with this patch. Will try another fix approach.

Can we not simply just skip scanning function 0 all together when
on Goldmont? I don't think any drivers actually ask for the bar
of function 0 on Goldmont ?

This is likely also why we never had the issue with the old p2sb_bar()
code, because that never touched function 0.

I think this is actually what you did in one of your first test
patches in the bugzilla, right ?

So maybe audit all the callers of p2sb_bar() and see if any
caller asks for function 0 on goldmont ?

Let me know if you need help with this audit.

Regards,

Hans

On Mar 02, 2024 / 12:43, Hans de Goede wrote:
> Hi Shinichiro,
> 
> Thank you for your work on this.
> 
> On 3/2/24 08:28, Shinichiro Kawasaki wrote:
> > On Mar 02, 2024 / 10:28, Shin'ichiro Kawasaki wrote:
> >> The commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
> >> during PCI device probe") triggered repeated ACPI errors on ASUS
> >> VivoBook D540NV-GQ065T [1]. It was confirmed that the P2SB device scan
> >> and remove at the fs_initcall stage triggered the errors.
> >>
> >> To avoid the error, defer the P2SB device scan on the concerned device.
> >> The error was observed on the system with Pentium N4200 in Goldmont micro-
> >> architecture, and on which P2SB has function 0. Then refer to the P2SB
> >> function to decide whether to defer or not.
> >>
> >> When the device scan is deferred, do the scan later when p2sb_bar() is
> >> called for the first time. If this first scan is triggered by sysfs
> >> pci bus rescan, deadlock happens. In most cases, the scan happens during
> >> system boot process, then there is no chance of deadlock.
> >>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218531 [1]
> >> Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
> >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > 
> > Let me drop this patch. danilrybakov found that the ACPI errors are still
> > reported even with this patch. Will try another fix approach.
> 
> Can we not simply just skip scanning function 0 all together when
> on Goldmont? I don't think any drivers actually ask for the bar
> of function 0 on Goldmont ?

Hi Hans, thank you for the idea. If we can take this appraoch, the fix patch
will be simpler.

> 
> This is likely also why we never had the issue with the old p2sb_bar()
> code, because that never touched function 0.
> 
> I think this is actually what you did in one of your first test
> patches in the bugzilla, right ?

To be precise, the first test patch did P2SB scan only for the function 2.
To make sure your idea works, it's the better to test to scan all the function
numbers except 0, from 1 to 7.

> 
> So maybe audit all the callers of p2sb_bar() and see if any
> caller asks for function 0 on goldmont ?
> 
> Let me know if you need help with this audit.

Help for the audit will be appreciated.

With the quick grep for p2sb_bar() [2], there are five p2sb_bar() callers:

 1) edac/pnd2_edac             devfn = 0
 2) i2c/busses/i2c-i801        devfn = 0
 3) mfd/lpc_ich for pinctrl    devfn = 0
 4) mfd/lpc_ich for spi        devfn = PCI_DEVFN(13, 2)
 5) watchdog/simatic-ipc-wdt   devfn = 0

Four out of the five callers specify devfn = 0, which means devfn is the P2SB
default PCI_DEVFN(13, 0) on Goldmont. So the question is if the four drivers are
used on Goldmont platform or not. And I have no clue...

[2]

$ git grep p2sb_bar
drivers/edac/pnd2_edac.c:                       ret = p2sb_bar(NULL, 0, &r);
drivers/i2c/busses/i2c-i801.c:  ret = p2sb_bar(pci_dev->bus, 0, res);
drivers/mfd/lpc_ich.c:  ret = p2sb_bar(dev->bus, 0, &base);
drivers/mfd/lpc_ich.c:          ret = p2sb_bar(dev->bus, PCI_DEVFN(13, 2), res);
drivers/platform/x86/p2sb.c: * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
drivers/platform/x86/p2sb.c:int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
drivers/platform/x86/p2sb.c:EXPORT_SYMBOL_GPL(p2sb_bar);
drivers/watchdog/simatic-ipc-wdt.c:             ret = p2sb_bar(NULL, 0, res);
include/linux/platform_data/x86/p2sb.h:int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem);
include/linux/platform_data/x86/p2sb.h:static inline int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)

Hi Shinichiro,

On 3/3/24 00:37, Shinichiro Kawasaki wrote:
> On Mar 02, 2024 / 12:43, Hans de Goede wrote:

<snip>

>> Can we not simply just skip scanning function 0 all together when
>> on Goldmont? I don't think any drivers actually ask for the bar
>> of function 0 on Goldmont ?
> 
> Hi Hans, thank you for the idea. If we can take this appraoch, the fix patch
> will be simpler.
> 
>>
>> This is likely also why we never had the issue with the old p2sb_bar()
>> code, because that never touched function 0.
>>
>> I think this is actually what you did in one of your first test
>> patches in the bugzilla, right ?
> 
> To be precise, the first test patch did P2SB scan only for the function 2.
> To make sure your idea works, it's the better to test to scan all the function
> numbers except 0, from 1 to 7.
> 
>>
>> So maybe audit all the callers of p2sb_bar() and see if any
>> caller asks for function 0 on goldmont ?
>>
>> Let me know if you need help with this audit.
> 
> Help for the audit will be appreciated.
> 
> With the quick grep for p2sb_bar() [2], there are five p2sb_bar() callers:

Ack, I have found the same 5 callers, let go over them one by one:

>  1) edac/pnd2_edac             devfn = 0

Hmm, ok so this one binds based on CPU-ids:

static const struct x86_cpu_id pnd2_cpuids[] = {
        X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,       &apl_ops),
        X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_D,     &dnv_ops),
        { }
};
MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids);

And the 0 passed here will get replaced by PCI_DEVFN(13, 0),
so there goes my theory of p2sb() never being called for
function 0.

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.

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

Regards,

Hans


p.s.

FWIW here is a quick analysis of the other callers:

>  2) i2c/busses/i2c-i801        devfn = 0

Goldmont platforms use PCI_DEVICE_ID_INTEL_BROXTON_SMBUS for the i801
i2c adapter and that does not have FEATURE_TCO_* set in its
feature flags so the p2sb_bar() call there is never made
on broxton.

>  3) mfd/lpc_ich for pinctrl    devfn = 0

This one is made on Apollo Lake devices, which use goldmont
CPU cores, so this is another case where function 0 is
actually queried through p2sb_bar()...

>  4) mfd/lpc_ich for spi        devfn = PCI_DEVFN(13, 2)

Not function 0.

>  5) watchdog/simatic-ipc-wdt   devfn = 0

This is actually a similar usage to the mfd/lpc_ich to get
to the GPIO controller when its not exported through ACPI,
but before the code switched to the p2sb_bar() helper it
was looking at PCI_DEVFN(31, 1), so this does not run
on goldmont.

Regards,

Hans




> 
> [2]
> 
> $ git grep p2sb_bar
> drivers/edac/pnd2_edac.c:                       ret = p2sb_bar(NULL, 0, &r);
> drivers/i2c/busses/i2c-i801.c:  ret = p2sb_bar(pci_dev->bus, 0, res);
> drivers/mfd/lpc_ich.c:  ret = p2sb_bar(dev->bus, 0, &base);
> drivers/mfd/lpc_ich.c:          ret = p2sb_bar(dev->bus, PCI_DEVFN(13, 2), res);
> drivers/platform/x86/p2sb.c: * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
> drivers/platform/x86/p2sb.c:int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
> drivers/platform/x86/p2sb.c:EXPORT_SYMBOL_GPL(p2sb_bar);
> drivers/watchdog/simatic-ipc-wdt.c:             ret = p2sb_bar(NULL, 0, res);
> include/linux/platform_data/x86/p2sb.h:int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem);
> include/linux/platform_data/x86/p2sb.h:static inline int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
>

On Mar 03, 2024 / 20:35, Hans de Goede wrote:
> Hi Shinichiro,
> 
> On 3/3/24 00:37, Shinichiro Kawasaki wrote:
> > On Mar 02, 2024 / 12:43, Hans de Goede wrote:
> 
> <snip>
> 
> >> Can we not simply just skip scanning function 0 all together when
> >> on Goldmont? I don't think any drivers actually ask for the bar
> >> of function 0 on Goldmont ?
> > 
> > Hi Hans, thank you for the idea. If we can take this appraoch, the fix patch
> > will be simpler.
> > 
> >>
> >> This is likely also why we never had the issue with the old p2sb_bar()
> >> code, because that never touched function 0.
> >>
> >> I think this is actually what you did in one of your first test
> >> patches in the bugzilla, right ?
> > 
> > To be precise, the first test patch did P2SB scan only for the function 2.
> > To make sure your idea works, it's the better to test to scan all the function
> > numbers except 0, from 1 to 7.
> > 
> >>
> >> So maybe audit all the callers of p2sb_bar() and see if any
> >> caller asks for function 0 on goldmont ?
> >>
> >> Let me know if you need help with this audit.
> > 
> > Help for the audit will be appreciated.
> > 
> > With the quick grep for p2sb_bar() [2], there are five p2sb_bar() callers:
> 
> Ack, I have found the same 5 callers, let go over them one by one:
> 
> >  1) edac/pnd2_edac             devfn = 0
> 
> Hmm, ok so this one binds based on CPU-ids:
> 
> static const struct x86_cpu_id pnd2_cpuids[] = {
>         X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,       &apl_ops),
>         X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_D,     &dnv_ops),
>         { }
> };
> MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids);
> 
> And the 0 passed here will get replaced by PCI_DEVFN(13, 0),
> so there goes my theory of p2sb() never being called for
> function 0.

Thank you very much fot the audit. So, it was clarified that the p2sb_bar()
can be called for function 0 on goldmont.

> 
> 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().

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

On the other hand, it means that the drawbacks will be still left on goldmont.
I would like to follow your call. If you think the "no fs_initcall() and always
delay the caching" is the best, I'm willing to prepare the patch for it.

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

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

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.

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

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

On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
> On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
> > On Mar 04, 2024 / 12:04, Hans de Goede wrote:

...

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

So, why we can't simply call pci_dev_present() on the function in a loop?

Will be even simpler fix, no?

Hi Andy,

On 3/4/24 3:09 PM, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
>> On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
>>> On Mar 04, 2024 / 12:04, Hans de Goede wrote:
> 
> ...
> 
>>> 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.
> 
> So, why we can't simply call pci_dev_present() on the function in a loop?
> 
> Will be even simpler fix, no?

pci_dev_present takes a set of ids and then looks for those in already
detected devices. That will not work for devices which we have just unhidden.

Also it is unclear what exactly is tripping the hw up. We already have
a separate code-path for Goldmont, on other platforms only the P2SB
itself is scanned instead of all functions on the PCI slot.

This patch makes the Goldmont code closer to the other platforms by
only scanning the one extra function instead of scanning all functions.

As such this patch also mostly removes code :)

Regards,

Hans

On Mon, Mar 04, 2024 at 04:09:49PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
> > On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
> > > On Mar 04, 2024 / 12:04, Hans de Goede wrote:

...

> > > 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.
> 
> So, why we can't simply call pci_dev_present() on the function in a loop?

pci_device_is_present()

> Will be even simpler fix, no?

On Mon, Mar 04, 2024 at 04:17:04PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 04:09:49PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
> > > On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
> > > > On Mar 04, 2024 / 12:04, Hans de Goede wrote:

...

> > > > 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.
> > 
> > So, why we can't simply call pci_dev_present() on the function in a loop?
> 
> pci_device_is_present()
> 
> > Will be even simpler fix, no?

Oh, it requires a PCI device structure which is probably is not there until
we call the pci_scan_single_device()...

Hi,

On 3/4/24 3:17 PM, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 04:09:49PM +0200, Andy Shevchenko wrote:
>> On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
>>> On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
>>>> On Mar 04, 2024 / 12:04, Hans de Goede wrote:
> 
> ...
> 
>>>> 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.
>>
>> So, why we can't simply call pci_dev_present() on the function in a loop?
> 
> pci_device_is_present()

That takes a pci_dev as argument which we only have after calling
pci_scan_single_device() and the calling of pci_scan_single_device()
on some of the other functions is what is suspected of maybe causing
the issue.

Also it is likely that if pci_scan_single_device() is actually
a problem that then what is a problem is running it on an actual
present device. Devices which are not there also should not get confused
by trying to probe them ...

Regards,

Hans

On Mon, Mar 04, 2024 at 03:24:58PM +0100, Hans de Goede wrote:
> On 3/4/24 3:17 PM, Andy Shevchenko wrote:
> > On Mon, Mar 04, 2024 at 04:09:49PM +0200, Andy Shevchenko wrote:
> >> On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
> >>> On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
> >>>> On Mar 04, 2024 / 12:04, Hans de Goede wrote:

...

> >>>> 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.
> >>
> >> So, why we can't simply call pci_dev_present() on the function in a loop?
> > 
> > pci_device_is_present()
> 
> That takes a pci_dev as argument which we only have after calling
> pci_scan_single_device() and the calling of pci_scan_single_device()
> on some of the other functions is what is suspected of maybe causing
> the issue.
> 
> Also it is likely that if pci_scan_single_device() is actually
> a problem that then what is a problem is running it on an actual
> present device.

Which is weird. But okay, let's work around first the real issue, then
if I have time I will look into datasheet to see if there is anything
special about P2SB device(s).

> Devices which are not there also should not get confused
> by trying to probe them ...

Hi,

On 3/4/24 3:35 PM, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 03:24:58PM +0100, Hans de Goede wrote:
>> On 3/4/24 3:17 PM, Andy Shevchenko wrote:
>>> On Mon, Mar 04, 2024 at 04:09:49PM +0200, Andy Shevchenko wrote:
>>>> On Mon, Mar 04, 2024 at 02:59:57PM +0100, Hans de Goede wrote:
>>>>> On 3/4/24 1:13 PM, Shinichiro Kawasaki wrote:
>>>>>> On Mar 04, 2024 / 12:04, Hans de Goede wrote:
> 
> ...
> 
>>>>>> 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.
>>>>
>>>> So, why we can't simply call pci_dev_present() on the function in a loop?
>>>
>>> pci_device_is_present()
>>
>> That takes a pci_dev as argument which we only have after calling
>> pci_scan_single_device() and the calling of pci_scan_single_device()
>> on some of the other functions is what is suspected of maybe causing
>> the issue.
>>
>> Also it is likely that if pci_scan_single_device() is actually
>> a problem that then what is a problem is running it on an actual
>> present device.
> 
> Which is weird. But okay, let's work around first the real issue, then
> if I have time I will look into datasheet to see if there is anything
> special about P2SB device(s).

Note atm pci_scan_single_device() on one of the "other" being
the problem is still only a theory!

It is also possible that pci_scan_single_device() on the P2SB
itself is somehow an issue on this laptop, since on this laptop
p2sb_bar() is only called for the SPI controller, so before
the caching to avoid the deadlock issue pci_scan_single_device()
was not called on the P2SB devfn itself.

Regards,

Hans