On Thu, Nov 30, 2023 at 07:30:56AM +0000, Shinichiro Kawasaki wrote:
> On Nov 29, 2023 / 15:53, Andy Shevchenko wrote:
> > On Wed, Nov 29, 2023 at 03:50:21PM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 29, 2023 at 12:17:39PM +0100, Lukas Wunner wrote:
> > > > Your patch uses a list to store a multitude of struct resource.
> > > > Is that actually necessary? I thought there can only be a single
> > > > P2SB device in the system?
>
> Yes, the list might be too much. I was not sure what is the expected
> number of P2SB resources to be cached. I found drivers/mfd/lpc_ich.c
> calls p2sb_bar() at two places for devfn=0 and devfn=(13,2), so at
> least two resources look required. Not sure about the future.
> If two static resources are sufficient, the code will be simpler.
About that p2sb_bar() call in lpc_ich.c for PCI_DEVFN(13, 2):
It's in a switch-case statement for INTEL_SPI_BXT. BXT means Broxton,
which is an Atom Goldmont based architecture.
If you look at p2sb_cpu_ids[], you'll notice the P2SB is located at
PCI_DEVFN(13, 0) on Goldmont.
PCI functions with function number > 0 are not enumerable unless there is
a PCI function with function number 0.
So p2sb_bar() first unhides the P2SB at PCI_DEVFN(13, 0), then the
SPI function at PCI_DEVFN(13, 2) becomes enumerable and p2sb_bar()
retrieves the BAR address of that function.
Unfortunately this is a little byzantine.
For the caching approach I guess it means you can assume there is only
a single P2SB device in the system but you need to cache not just the
P2SB BAR, but also the BARs of functions 1 .. 7 of the P2SB device
if the P2SB's function number is 0. I don't know if each of those
upper functions only ever has a single BAR, but probably that's the case.
> Lukas, thank you for the idea. If I understand the comment by Andy
> correctly, P2SB should not be unhidden between arch_initcall and
> fs_initcall. Hmm.
>
> This made me think: how about to unhide and hide P2SB just during
> fs_initcall to cache the P2SB resources? To try it, I added a function
> below on top of the previous trial patch. The added function calls
> p2sb_bar() for devfn=0 at fs_initcall so that the resource is cached
> before probe of i2c-i801. This worked
> good on my system. It avoided the deadlock as well as the lockdep WARN :)
This may work if i2c-i801 is compiled as a module, but may not work
if it's builtin: It would try to access the cached P2SB BAR when
it's not yet been cached. So you'd have to return -EPROBE_DEFER
from p2sb_bar() if it hasn't been cached yet. And you'd have to
make sure that all the callers can cope with that return value.
Another approach would be to cache the BARs very early at boot in
arch/x86/kernel/early-quirks.c. That would obviate the need to
defer probing if the BAR hasn't been cached yet.
Looking through past discussions archived in lore, I've found an
important issue raised by Bjorn:
"Apparently this normally hidden device is consuming
PCI address space. The PCI core needs to know about this. If it
doesn't, the PCI core may assign this space to another device."
https://lore.kernel.org/all/20210308185212.GA1790506@bjorn-Precision-5520/
arch/x86/kernel/early-quirks.c already reserves "stolen" memory used
by Intel GPUs with unified-memory architecture. It adjusts the e820
map to achieve that. I guess the same method could be used to reserve
the memory used by P2SB (as well as "upper" functions if P2SB has
function number 0).
An early version of p2sb_bar() (which wasn't mainlined) duplicated
__pci_read_base(). I suggested to instead unhide and temporarily
enumerate the device, retrieve the BAR, then destroy the pci_dev
and hide the P2SB again:
https://lore.kernel.org/all/20220505145503.GA25423@xxxxxxxxx/
That resulted in a significant reduction in LoC. But if the BAR
caching happens in arch/x86/kernel/early-quirks.c, it may be
necessary to duplicate at least portions of __pci_read_base() again.
Or maybe the code can be simplified for this specific use case,
I don't know.
Thanks,
Lukas
