Re: [PATCH V2 01/11] cxl/pci: Add generic MSI-X/MSI irq support

[Dan Williams]

Ira Weiny wrote:
> On Thu, Dec 01, 2022 at 04:23:21PM -0800, Dan Williams wrote:
> > ira.weiny@ wrote:
> > > From: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > > 
> > > Currently the only CXL features targeted for irq support require their
> > > message numbers to be within the first 16 entries.  The device may
> > > however support less than 16 entries depending on the support it
> > > provides.
> > > 
> > > Attempt to allocate these 16 irq vectors.  If the device supports less
> > > then the PCI infrastructure will allocate that number.
> > 
> > What happens if the device supports 16, but irq-core allocates less? I
> > believe the answer is with the first user, but this patch does not
> > include a user.
> > 
> > > Store the number of vectors actually allocated in the device state for
> > > later use by individual functions.
> > 
> > The patch does not do that.
> 
> Sorry missed updating this message.
> 
> > 
> > I know this patch has gone through a lot of discussion, but this
> > mismatch shows it should really be squashed with the first user because
> > it does not stand on its own anymore.
> 
> It is separate because it was Davidlohr's to begin with.
> 
> I'll squash it back.
> 
> > 
> > > Upon successful allocation, users can plug in their respective isr at
> > > any point thereafter, for example, if the irq setup is not done in the
> > > PCI driver, such as the case of the CXL-PMU.
> > > 
> > > Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > Signed-off-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > > 
> > > ---
> > > Changes from V1:
> > > 	Jonathan
> > > 		pci_alloc_irq_vectors() cleans up the vectors automatically
> > > 		use msi_enabled rather than nr_irq_vecs
> > > 
> > > Changes from Ira
> > > 	Remove reviews
> > > 	Allocate up to a static 16 vectors.
> > > 	Change cover letter
> > > ---
> > >  drivers/cxl/cxlmem.h |  3 +++
> > >  drivers/cxl/cxlpci.h |  6 ++++++
> > >  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
> > >  3 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index 88e3a8e54b6a..cd35f43fedd4 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
> > >   * @info: Cached DVSEC information about the device.
> > >   * @serial: PCIe Device Serial Number
> > >   * @doe_mbs: PCI DOE mailbox array
> > > + * @msi_enabled: MSI-X/MSI has been enabled
> > >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> > >   *
> > >   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > > @@ -247,6 +248,8 @@ struct cxl_dev_state {
> > >  
> > >  	struct xarray doe_mbs;
> > >  
> > > +	bool msi_enabled;
> > > +
> > 
> > This goes unused in this patch and it also duplicates what the core
> > offers with pdev->{msi,msix}_enabled.
> 
> I tried to argue that with Jonathan and lost.  What I had in V1 was to store
> the number actually allocated.  Then if a function reports something higher
> later it can't be used.

A successful pci_alloc_irq_vectors() call assigns a vector number to all
interrupt sources on the device regardless of how many interrupt sources
there are. If the device has 32 interrupt sources and 16 irqs are returned
from pci_alloc_irq_vectors() then each interrupt source will be sharing
a vector with one or more other vectors. All PCI IRQ vectors are shared.

So I do not see the point of this msi_enabled flag cxl_dev_state. If
pci_alloc_irq_vectors() returns at least 1 then you are good to go.