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"?
> 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
>
