Re: [PATCH] PCI: Fix devres regression in pci_intx()

[Damien Le Moal]

On 2024/09/04 16:06, Philipp Stanner wrote:
> On Tue, 2024-09-03 at 09:44 -0600, Alex Williamson wrote:
>> On Thu, 25 Jul 2024 14:07:30 +0200
>> Philipp Stanner <pstanner@xxxxxxxxxx> wrote:
>>
>>> pci_intx() is a function that becomes managed if
>>> pcim_enable_device()
>>> has been called in advance. Commit 25216afc9db5 ("PCI: Add managed
>>> pcim_intx()") changed this behavior so that pci_intx() always leads
>>> to
>>> creation of a separate device resource for itself, whereas earlier,
>>> a
>>> shared resource was used for all PCI devres operations.
>>>
>>> Unfortunately, pci_intx() seems to be used in some drivers'
>>> remove()
>>> paths; in the managed case this causes a device resource to be
>>> created
>>> on driver detach.
>>>
>>> Fix the regression by only redirecting pci_intx() to its managed
>>> twin
>>> pcim_intx() if the pci_command changes.
>>>
>>> Fixes: 25216afc9db5 ("PCI: Add managed pcim_intx()")
>>
>> I'm seeing another issue from this, which is maybe a more general
>> problem with managed mode.  In my case I'm using vfio-pci to assign
>> an
>> ahci controller to a VM.
> 
> "In my case" doesn't mean OOT, does it? I can't fully follow.
> 
>>   ahci_init_one() calls pcim_enable_device()
>> which sets is_managed = true.  I notice that nothing ever sets
>> is_managed to false.  Therefore now when I call pci_intx() from vfio-
>> pci
>> under spinlock, I get a lockdep warning
> 
> I suppose you see the lockdep warning because the new pcim_intx() can 
> now allocate, whereas before 25216afc9db5 it was pcim_enable_device()
> which allocated *everything* related to PCI devres.
> 
>>  as I no go through pcim_intx()
>> code after 25216afc9db5 
> 
> You alwas went through pcim_intx()'s logic. The issue seems to be that
> the allocation step was moved.
> 
>> since the previous driver was managed.
> 
> what do you mean by "previous driver"?

The AHCI driver... When attaching a PCI dev to vfio to e.g. passthrough to a VM,
the device driver must first be unbound and the device bound to vfio-pci. So we
switch from ahci/libata driver to vfio. When vfio tries to enable intx with
is_managed still true from the use of the device by ahci, problem happen.

> 
>>   It seems
>> like we should be setting is_managed to false is the driver release
>> path, right?
> 
> So the issue seems to be that the same struct pci_dev can be used by
> different drivers, is that correct?
> 
> If so, I think that can be addressed trough having
> pcim_disable_device() set is_managed to false as you suggest.
> 
> Another solution can could at least consider would be to use a
> GFP_ATOMIC for allocation in get_or_create_intx_devres().

If it is allowed to call pci_intx() under a spin_lock, then we need GFP_ATOMIC.
If not, then vfio-pci needs to move the call out of the spinlock.

Either solution must be implemented regardless of the fix to set is_managed to
false.

So what context is allowed to call pci_intx() ? The current kdoc comment does
not say...


-- 
Damien Le Moal
Western Digital Research