On Wed, 2024-09-04 at 17:25 +0900, Damien Le Moal wrote:
> 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.
If vfio-pci can get rid of pci_intx() alltogether, that might be a good
thing. As far as I understood Andy Shevchenko, pci_intx() is outdated.
There's only a hand full of users anyways.
Best solution would be to avoid GFP_ATOMIC and see first if setting
is_managed = false solves the reported problem for now.
Other problematic users should hopefully be found through lockdep, too.
Though I think they are unlikely to occur
>
> Either solution must be implemented regardless of the fix to set
> is_managed to
> false.
Yes
>
> So what context is allowed to call pci_intx() ? The current kdoc
> comment does
> not say...
the old pci_intx() did not allocate.
It only calls pci_read_config_word() and pci_write_config_word(). If
those cannot block etc. it should be save from any context.
Though I'd like to hear from one of the maintainers about it.
The new version allocates if pcim_enable_device() was called when it
runs for the first time. That first run would then be illegal in must-
not-sleep contexts.
P.
