Re: [PATCH] PCI: iproc: restrict multi-MSI to single core CPUs

Re: [PATCH] PCI: iproc: restrict multi-MSI to single core CPUs

[Bjorn Helgaas]

[+cc Lorenzo, linux-pci]

You can use this to find the appropriate cc list:

  ./scripts/get_maintainer.pl -f drivers/pci/controller/pcie-iproc-msi.c

I added Lorenzo and linux-pci for you.

Please update the subject line to:

  PCI: iproc: Support multi-MSI only on uniprocessor kernel

On Sat, Jun 05, 2021 at 07:17:36PM +0200, Sandor Bodo-Merle wrote:
> Commit fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
> introduced multi-MSI support with a broken allocation mechanism (it failed to
> reserve the proper number of bits from the inner domain).  Natural alignment of
> the base vector number was also not guaranteed.

This sounds like it's fixing *two* problems: the bitmap allocation
problem above, and the multi-MSI restriction problem below.  Please
split this into two separate patches if possible.

> The interrupt affinity scheme used by this driver is incompatible with
> multi-MSI as implies moving the doorbell address to that of another MSI group.
> This isn't possible for Multi-MSI, as all the MSIs must have the same doorbell
> address. As such it is restricted to systems with single CPU core.

Please rewrap the commit log to fit in 75 columns, so it still fits
in 80 when "git log" indents it.

s/as implies/as it implies/
s/Multi-MSI/multi-MSI/ (or capitalize them all; just be consistent)
s/with single CPU core/with a single CPU/

Using "core" here ("single core CPUs" or "single CPU core") suggests
that this has something to do with single-core CPUs vs multi-core
CPUs, but I don't think that's the case.

The patch says the important thing is whether the kernel supports one
CPU or several CPUs.  Whether they're in a single package or not is
irrelevant.  And apparently multi-MSI even works fine when you boot a
uniprocessor kernel (CONFIG_NR_CPUS=1) on a multi-processor machine.

> Fixes: fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
> Reported-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Sandor Bodo-Merle <sbodomerle@gmail.com>
> ---
>  drivers/pci/controller/pcie-iproc-msi.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git drivers/pci/controller/pcie-iproc-msi.c drivers/pci/controller/pcie-iproc-msi.c

Patch is incorrectly generated and lacks a path element, so doesn't
apply cleanly.  I don't know how you did this, but it should look like
this (note the leading "a/" and "b/"):

  diff --git a/drivers/pci/controller/pcie-iproc-msi.c b/drivers/pci/controller/pcie-iproc-msi.c

> index eede4e8f3f75..2e42c460b626 100644
> --- drivers/pci/controller/pcie-iproc-msi.c
> +++ drivers/pci/controller/pcie-iproc-msi.c
> @@ -171,7 +171,7 @@ static struct irq_chip iproc_msi_irq_chip = {
>  
>  static struct msi_domain_info iproc_msi_domain_info = {
>  	.flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> -		MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX,
> +		MSI_FLAG_PCI_MSIX,
>  	.chip = &iproc_msi_irq_chip,
>  };
>  
> @@ -252,18 +252,15 @@ static int iproc_msi_irq_domain_alloc(struct irq_domain *domain,
>  
>  	mutex_lock(&msi->bitmap_lock);
>  
> -	/* Allocate 'nr_cpus' number of MSI vectors each time */
> -	hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
> -					   msi->nr_cpus, 0);
> -	if (hwirq < msi->nr_msi_vecs) {
> -		bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
> -	} else {
> -		mutex_unlock(&msi->bitmap_lock);
> -		return -ENOSPC;
> -	}
> +	/* Allocate 'nr_irqs' multiplied by 'nr_cpus' number of MSI vectors each time */

Can you wrap this comment so it fits in 80 columns, please?  The rest
of the file is formatted for 80 columns, so it will be nice if this
matches.

> +	hwirq = bitmap_find_free_region(msi->bitmap, msi->nr_msi_vecs,
> +					order_base_2(msi->nr_cpus * nr_irqs));
>  
>  	mutex_unlock(&msi->bitmap_lock);
>  
> +	if (hwirq < 0)
> +		return -ENOSPC;
> +
>  	for (i = 0; i < nr_irqs; i++) {
>  		irq_domain_set_info(domain, virq + i, hwirq + i,
>  				    &iproc_msi_bottom_irq_chip,
> @@ -284,7 +281,8 @@ static void iproc_msi_irq_domain_free(struct irq_domain *domain,
>  	mutex_lock(&msi->bitmap_lock);
>  
>  	hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
> -	bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
> +	bitmap_release_region(msi->bitmap, hwirq,
> +			      order_base_2(msi->nr_cpus * nr_irqs));
>  
>  	mutex_unlock(&msi->bitmap_lock);
>  
> @@ -539,6 +537,9 @@ int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node)
>  	mutex_init(&msi->bitmap_lock);
>  	msi->nr_cpus = num_possible_cpus();
>  
> +	if (msi->nr_cpus == 1)
> +		iproc_msi_domain_info.flags |=  MSI_FLAG_MULTI_PCI_MSI;
> +
>  	msi->nr_irqs = of_irq_count(node);
>  	if (!msi->nr_irqs) {
>  		dev_err(pcie->dev, "found no MSI GIC interrupt\n");
> -- 
> 2.31.0
>