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

[Philipp Stanner <pstanner@redhat.com>]

On Thu, 2024-09-05 at 09:33 +0900, Damien Le Moal wrote:
> On 2024/09/05 6:10, Alex Williamson wrote:
> > On Wed, 4 Sep 2024 23:24:53 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > > Wed, Sep 04, 2024 at 12:07:21PM -0600, Alex Williamson kirjoitti:
> > > > On Wed, 04 Sep 2024 15:37:25 +0200
> > > > Philipp Stanner <pstanner@redhat.com> wrote:  
> > > > > On Wed, 2024-09-04 at 17:25 +0900, Damien Le Moal wrote:  
> > > 
> > > ...
> > > 
> > > > > 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?  
> > > 
> > > From API perspective the pci_alloc_irq_vectors() & Co should be
> > > used.
> > 
> > We can't replace a device level INTx control with a vector
> > allocation
> > function.
> >  
> > > > 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,  
> > > 
> > > I briefly read the discussion and if I understand it correctly
> > > the problem here
> > > is in the flow: when the above mentioned API is being called.
> > > Hence it's design
> > > (or architectural) level of issue and changing call from foo() to
> > > bar() won't
> > > magically make problem go away. But I might be mistaken.
> > 
> > Certainly from a vector allocation standpoint we can change to
> > whatever
> > is preferred, but the direct INTx manipulation functions are a
> > different thing entirely and afaik there's nothing else that can
> > replace them at a low level, nor can we just get rid of our calls
> > to
> > pci_intx().  Thanks,
> 
> But can these calls be moved out of the spinlock context ? If not,
> then we need
> to clarify that pci_intx() can be called from any context, which will
> require
> changing to a GFP_ATOMIC for the resource allocation, even if the use
> case
> cannot trigger the allocation. This is needed to ensure the
> correctness of the
> pci_intx() function use.

We could do that I guess. As I keep saying, it's not intended to have
pci_intx() allocate _permanently_. This is a temporary situation.
pci_intx() should have neither devres nor allocation.

> Frankly, I am surprised that the might sleep splat you
> got was not already reported before (fuzzying, static analyzers might
> eventually
> catch that though).

It's a super rare situation:
 * pci_intx() has very few callers
 * It only allocates if pcim_enable_device() instead of
   pci_enable_device() ran.
 * It only allocates when it's called for the FIRST TIME
 * All of the above is only a problem while you hold a lock

> 
> The other solution would be a version of pci_intx() that has a gfp
> flags
> argument to allow callers to use the right gfp flags for the call
> context.

I don't think that's a good idea. As I said, I want to clean up all
that in the mid term.

As a matter of fact, there is already __pcim_intx() in pci/devres.c as
a pure unmanaged pci_intx() as a means to split and then cleanup the
APIs.

One path towards getting the hybrid behavior out of pci_intx() could be
to rename __pcim_intx() to pci_intx_unmanaged() and port everyone who
uses pci_enable_device() + pci_intx() to that version. That would be
better than to have a third version with a gfp_t argument.


P.

> 
> 
> > 
> > Alex
> > 
>