Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation

[Marc Zyngier <maz@kernel.org>]

On Tue, 03 Jun 2025 14:29:58 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Jun 03, 2025 at 01:50:47PM +0100, Marc Zyngier wrote:
> > Hi Zenghui,
> > 
> > On Tue, 03 Jun 2025 09:22:47 +0100,
> > Zenghui Yu <yuzenghui@huawei.com> wrote:
> > > 
> > > > +	domain->dev = dev;
> > > > +	dev->msi.data->__domains[domid].domain = domain;
> > > > +
> > > > +	if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {
> > > 
> > > Does it work for MSI? hwsize is 1 in the MSI case, without taking
> > > pci_msi_vec_count() into account.
> > >
> > > bool pci_setup_msi_device_domain(struct pci_dev *pdev)
> > > {
> > > 	[...]
> > > 
> > > 	return pci_create_device_domain(pdev, &pci_msi_template, 1);
> > 
> > Well spotted.
> > 
> > This looks like a PCI bug ignoring Multi-MSI. Can you give the
> > following a go and let people know whether that fixes your issue?
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> > index d7ba8795d60f..89677a21d525 100644
> > --- a/drivers/pci/msi/irqdomain.c
> > +++ b/drivers/pci/msi/irqdomain.c
> > @@ -287,7 +287,7 @@ static bool pci_create_device_domain(struct pci_dev *pdev, const struct msi_doma
> >   *	- The device is removed
> >   *	- MSI is disabled and a MSI-X domain is created
> >   */
> > -bool pci_setup_msi_device_domain(struct pci_dev *pdev)
> > +bool pci_setup_msi_device_domain(struct pci_dev *pdev, unsigned int hwsize)
> >  {
> >  	if (WARN_ON_ONCE(pdev->msix_enabled))
> >  		return false;
> > @@ -297,7 +297,7 @@ bool pci_setup_msi_device_domain(struct pci_dev *pdev)
> >  	if (pci_match_device_domain(pdev, DOMAIN_BUS_PCI_DEVICE_MSIX))
> >  		msi_remove_device_irq_domain(&pdev->dev, MSI_DEFAULT_DOMAIN);
> >  
> > -	return pci_create_device_domain(pdev, &pci_msi_template, 1);
> > +	return pci_create_device_domain(pdev, &pci_msi_template, hwsize);
> >  }
> >  
> >  /**
> > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > index 8b8848788618..81891701840a 100644
> > --- a/drivers/pci/msi/msi.c
> > +++ b/drivers/pci/msi/msi.c
> > @@ -449,7 +449,7 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
> >  	if (rc)
> >  		return rc;
> >  
> > -	if (!pci_setup_msi_device_domain(dev))
> > +	if (!pci_setup_msi_device_domain(dev, nvec))
> 
> If pci_msi_vec_count(dev) > maxvec we would cap nvec and size the
> domain with the capped value.
> 
> In __pci_enable_msix_range() we are sizing the device according to
> pci_msix_vec_count(dev) regardless of maxvec, if I read the code correctly.
> 
> While fixing it it would be good to make them consistent unless there is
> a reason why they should not.

This is indeed odd, but that'd be a separate fix. Something like:

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 2090eef64b14..6ede55a7c5e6 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -439,9 +439,6 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (nvec < minvec)
 		return -ENOSPC;
 
-	if (nvec > maxvec)
-		nvec = maxvec;
-
 	rc = pci_setup_msi_context(dev);
 	if (rc)
 		return rc;
@@ -449,6 +446,9 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (!pci_setup_msi_device_domain(dev, nvec))
 		return -ENODEV;
 
+	if (nvec > maxvec)
+		nvec = maxvec;
+
 	for (;;) {
 		if (affd) {
 			nvec = irq_calc_affinity_vectors(minvec, nvec, affd);

-- 
Jazz isn't dead. It just smells funny.