Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries

[Raj, Ashok]

Hi Thomas

On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:

[snip]

> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -691,6 +691,7 @@ static int msix_setup_entries(struct pci
>  {
>  	struct irq_affinity_desc *curmsk, *masks = NULL;
>  	struct msi_desc *entry;
> +	void __iomem *addr;
>  	int ret, i;
>  	int vec_count = pci_msix_vec_count(dev);
>  
> @@ -711,6 +712,7 @@ static int msix_setup_entries(struct pci
>  
>  		entry->msi_attrib.is_msix	= 1;
>  		entry->msi_attrib.is_64		= 1;
> +
>  		if (entries)
>  			entry->msi_attrib.entry_nr = entries[i].entry;
>  		else
> @@ -722,6 +724,10 @@ static int msix_setup_entries(struct pci
>  		entry->msi_attrib.default_irq	= dev->irq;
>  		entry->mask_base		= base;
>  
> +		addr = pci_msix_desc_addr(entry);
> +		if (addr)
> +			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);

Silly question:
Do we have to read what the HW has to set this entry->masked? Shouldn't
this be all masked before we start the setup?

> +
>  		list_add_tail(&entry->list, dev_to_msi_list(&dev->dev));
>  		if (masks)
>  			curmsk++;
> @@ -732,26 +738,25 @@ static int msix_setup_entries(struct pci
>  	return ret;
>  }
>  
> -static void msix_program_entries(struct pci_dev *dev,
> -				 struct msix_entry *entries)
> +static void msix_update_entries(struct pci_dev *dev, struct msix_entry *entries)
>  {
>  	struct msi_desc *entry;
> -	int i = 0;
> -	void __iomem *desc_addr;
>  
>  	for_each_pci_msi_entry(entry, dev) {
> -		if (entries)
> -			entries[i++].vector = entry->irq;
> +		if (entries) {
> +			entries->vector = entry->irq;
> +			entries++;
> +		}
> +	}
> +}
>  
> -		desc_addr = pci_msix_desc_addr(entry);
> -		if (desc_addr)
> -			entry->masked = readl(desc_addr +
> -					      PCI_MSIX_ENTRY_VECTOR_CTRL);
> -		else
> -			entry->masked = 0;
> +static void msix_mask_all(void __iomem *base, int tsize)
> +{
> +	u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +	int i;
>  
> -		msix_mask_irq(entry, 1);
> -	}
> +	for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
> +		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);

shouldn't we initialize entry->masked here?

>  }
>  
>  /**
> @@ -768,9 +773,9 @@ static void msix_program_entries(struct
>  static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
>  				int nvec, struct irq_affinity *affd)
>  {
> -	int ret;
> -	u16 control;
>  	void __iomem *base;
> +	int ret, tsize;
> +	u16 control;
>  
>  	/*
>  	 * Some devices require MSI-X to be enabled before the MSI-X
> @@ -782,12 +787,16 @@ static int msix_capability_init(struct p
>  
>  	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
>  	/* Request & Map MSI-X table region */
> -	base = msix_map_region(dev, msix_table_size(control));
> +	tsize = msix_table_size(control);
> +	base = msix_map_region(dev, tsize);
>  	if (!base) {
>  		ret = -ENOMEM;
>  		goto out_disable;
>  	}
>  
> +	/* Ensure that all table entries are masked. */
> +	msix_mask_all(base, tsize);
> +
>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);
>  	if (ret)
>  		goto out_disable;
> @@ -801,7 +810,7 @@ static int msix_capability_init(struct p
>  	if (ret)
>  		goto out_free;
>  
> -	msix_program_entries(dev, entries);
> +	msix_update_entries(dev, entries);
>  
>  	ret = populate_msi_sysfs(dev);
>  	if (ret)
> 

-- 
Cheers,
Ashok

[Forgiveness is the attribute of the STRONG - Gandhi]