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

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

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