Re: platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

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

Hi,

On 18-Nov-24 2:24 PM, Andy Shevchenko wrote:
> On Mon, Nov 18, 2024 at 12:40:16PM +0000, Daniel Walker (danielwa) wrote:
>> On Mon, Nov 18, 2024 at 11:30:59AM +0000, Shinichiro Kawasaki wrote:
>>>
>>> Thank you. Here I quote the relevant part of the debug log.
>>>
>>> --------------------------------------------------------------------------------
>>> ...
>>> pci 0000:00:1f.0: [8086:19dc] type 00 class 0x060100 conventional PCI endpoint
>>> pci 0000:00:1f.1: [8086:19dd] type 00 class 0x058000 conventional PCI endpoint ... [A]
>>> pci 0000:00:1f.1: BAR 0 [mem 0xfd000000-0xfdffffff 64bit]
>>> pci 0000:00:1f.2: [8086:19de] type 00 class 0x058000 conventional PCI endpoint
>>> pci 0000:00:1f.2: BAR 0 [mem 0x88c00000-0x88c03fff]
>>> ...
>>> PCI: Using ACPI for IRQ routing
>>> pci 0000:00:1f.1: BAR 0 [mem 0xfd000000-0xfdffffff 64bit]: can't claim; no compatible bridge window ... [B]
>>> hpet0: at MMIO 0xfed00000, IRQs 2, 8, 0, 0, 0, 0, 0, 0
>>> ...
>>> NET: Registered PF_XDP protocol family
>>> pci 0000:00:1f.1: BAR 0 [mem 0x280000000-0x280ffffff 64bit]: assigned ... [C]
>>> pci 0000:00:09.0: PCI bridge to [bus 01-06]
>>> ...
>>> PCI: CLS 64 bytes, default 64
>>> p2sb_cache_resources
>>> p2sb_cache_resources: P2SBC_HIDE=0  ... [D]
>>> p2sb_scan_and_cache_devfn: devfn=1f.1
>>> p2sb_scan_and_cache_devfn: 280000000-280ffffff: 140204 ... [E]
>>> PCI-DMA: Using software bounce buffering for IO (SWIOTLB)
>>> ...
>>> --------------------------------------------------------------------------------
>>>
>>> Also, here I list my observations.
>>>
>>> [A] The P2SB device was detected with DEVFN 1f.1, and device id 8086:19dd
>>> [B] Failed to claim its resource
>>> [C] Assigned new resource
>>> [D] p2sb_cache_resource() was called after the new resource assignment.
>>>     P2SBC_HIDE bit is not set.
>>> [E] The new resource was cached. IORESOURCE flags: MEM_64,SIZE_ALIGN,MEM.
>>>
>>> So it was confirmed that the p2sb_cache_resource() was called after the new
>>> resource assignment, but Hans and Andy discuss that this order is not the
>>> problem cause, probably.
>>>
>>> One thing I noticed is that p2sb_bar() call is not recorded in the log. My
>>> understanding is that all device drivers which use P2SB resource shouled call
>>> p2sb_bar(). Daniel, you noted that "a custom gpio device" disappeared. Does its
>>> device driver call p2sb_bar()?
>>
>> No calls to p2sb_bar(). It maybe an Intel GPIO device actually. 0x8086 is Intel's vendor code.
>> I suspect it's something standard on Denverton.
> 
> Are you referring to LPC GPIO?
> 
>>> On the other hand, Daniel noted that,
>>>
>>>   "The vendor and device details for the pci device are 8086:19dd."
>>>
>>> I think 8086:19dd is the P2SB device itself. When p2sb_cache_resource() is
>>> called, pci_stop_and_remove_bus_device() is called for it, so I guess it is
>>> expected the device 8086:19dd disappears. Before applying the commit
>>> 5913320eb0b3, this pci_stop_and_remove_bus_device() call happened when
>>> p2sb_bar() was called. So, my mere guess is that Daniel's system's drivers do
>>> not call p2sb_bar() during the boot process, then the 8086:19dd P2SB device was
>>> still visible after boot.
>>
>> The method I used for the bisect was to check lspci for 19dd. I find it odd that
>> the device which does exist was not in lspci. Also pci_get_device() fails for
>> this device and gpio operations can't happen.
> 
> So, it seems like we have avoid removing PCI devices if the P2SB has been
> unhidden during the cache call. Still wondering if we simply unhide it early
> (if it was hidden) and assume the p2sb device is always present. In this case
> p2sb_bar() will become almost an equivalent to the pci_resource_start().

That would basically open it up for normal PCI drivers to bind to. Which is not
necessarily a problem, but I wonder if we already have such drivers which then
all of a sudden will start binding to it ?

Note that e.g. the GPIO code in lpc_ich.c checks if the GPIO controller is not
ACPI enumerated before calling p2sb_bar(). Unconditionally unhiding the P2SB
would not mean that at least the GPIO part will be enumerated twice, once
through ACPI and once through PCI. I'm not sure if that is a good idea.

My vote would go to fix the problem of the PCI device being removed by
the p2sb.c code when it was not hidden.

Regards,

Hans