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

[Philipp Stanner <pstanner@redhat.com>]

On Tue, 2024-09-03 at 09:44 -0600, Alex Williamson wrote:
> On Thu, 25 Jul 2024 14:07:30 +0200
> Philipp Stanner <pstanner@redhat.com> 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"?

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

I suppose your solution is the better one, though.


P.

>   Thanks,
> 
> Alex
>