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

[Alex Williamson]

On Wed, 04 Sep 2024 15:37:25 +0200
Philipp Stanner <pstanner@xxxxxxxxxx> wrote:

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

What's the alternative?  vfio-pci has a potentially unique requirement
here, we don't know how to handle the device interrupt, we only forward
it to the userspace driver.  As a level triggered interrupt, INTx will
continue to assert until that userspace driver handles the device.
That's obviously unacceptable from a host perspective, so INTx is
masked at the device via pci_intx() where available, or at the
interrupt controller otherwise.  The API with the userspace driver
requires that driver to unmask the interrupt, again resulting in a call
to pci_intx() or unmasking the interrupt controller, in order to receive
further interrupts from the device.  Thanks,

Alex