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@xxxxxxx>
> >
> > 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)
