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