incremental fix for NIU MSI-X problem

Commit Message

Matthew Wilcox May 14, 2009, 1:45 p.m. UTC

On Thu, May 14, 2009 at 07:44:34AM -0600, Matthew Wilcox wrote:
> I think the real problem is that msix_capability_init() is 90 lines
> of code with 11 local variables.  The fix for this, however, is not
> to encapsulate the function control elsewhere, but to create more
> subfunctions.  I shrunk it to 45 lines by doing that ... I'll send
> that patch as a followup (note I didn't even boot the result, but it
> does compile).

As promised ...

Comments

Hidetoshi Seto May 15, 2009, 12:49 a.m. UTC | #1

It is definitely cleanup, not for NIU MSI-X problem, and not for .30

One point...

Matthew Wilcox wrote:
> +	nr_entries = msix_setup_entries(dev, pos, entries, nvec);
>  
>  	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
> -	if (ret < 0) {
> -		/* If we had some success report the number of irqs
> -		 * we succeeded in setting up. */
> -		int avail = 0;
> -		list_for_each_entry(entry, &dev->msi_list, list) {
> -			if (entry->irq != 0) {
> -				avail++;
> -			}
> -		}
> -
> -		if (avail != 0)
> -			ret = avail;
> -	}
> +	if (ret < 0 && nr_entries > 0)
> +		ret = nr_entries;
>  
>  	if (ret) {
>  		msi_free_irqs(dev);

I think this changes the logic badly.
nr_entries is number of allocated entry, while avail is number of irq
successfully allocated.  I suppose usually nr_entries > avail.


Thanks,
H.Seto

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Ellerman May 15, 2009, 1:38 a.m. UTC | #2

On Fri, 2009-05-15 at 09:49 +0900, Hidetoshi Seto wrote:
> It is definitely cleanup, not for NIU MSI-X problem, and not for .30
> 
> One point...
> 
> Matthew Wilcox wrote:
> > +	nr_entries = msix_setup_entries(dev, pos, entries, nvec);
> >  
> >  	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
> > -	if (ret < 0) {
> > -		/* If we had some success report the number of irqs
> > -		 * we succeeded in setting up. */
> > -		int avail = 0;
> > -		list_for_each_entry(entry, &dev->msi_list, list) {
> > -			if (entry->irq != 0) {
> > -				avail++;
> > -			}
> > -		}
> > -
> > -		if (avail != 0)
> > -			ret = avail;
> > -	}
> > +	if (ret < 0 && nr_entries > 0)
> > +		ret = nr_entries;
> >  
> >  	if (ret) {
> >  		msi_free_irqs(dev);
> 
> I think this changes the logic badly.
> nr_entries is number of allocated entry, while avail is number of irq
> successfully allocated.  I suppose usually nr_entries > avail.

Yeah that bit's broken. If the arch routine fails (< 0) then we'll
return nr_entries to the driver, which will then try again with nvec =
nr_entries.

And you're passing nvec to the arch routine even though
msix_setup_entries() might not have allocated that many entries - which
means the value of nvec and the contents of the entry list don't match.

The overall structure of the split looks good though.

cheers

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6faff6a..73e2dbd 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -404,6 +404,62 @@  static int msi_capability_init(struct pci_dev *dev, int nvec)
 	return 0;
 }
 
+static void __iomem *msix_map_region(struct pci_dev *dev, unsigned pos,
+							unsigned nr_entries)
+{
+	unsigned long phys_addr;
+	u32 table_offset;
+	u8 bir;
+
+ 	pci_read_config_dword(dev, msix_table_offset_reg(pos), &table_offset);
+	bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
+	table_offset &= ~PCI_MSIX_FLAGS_BIRMASK;
+	phys_addr = pci_resource_start(dev, bir) + table_offset;
+	return ioremap_nocache(phys_addr, nr_entries * PCI_MSIX_ENTRY_SIZE);
+}
+
+static int msix_setup_entries(struct pci_dev *dev, unsigned pos,
+				struct msix_entry *entries, int nvec)
+{
+	struct msi_desc *entry;
+	int i, j;
+
+	for (i = 0; i < nvec; i++) {
+		entry = alloc_msi_entry(dev);
+		if (!entry)
+			return i;
+
+ 		j = entries[i].entry;
+		entry->msi_attrib.is_msix = 1;
+		entry->msi_attrib.is_64 = 1;
+		entry->msi_attrib.entry_nr = j;
+		entry->msi_attrib.default_irq = dev->irq;
+		entry->msi_attrib.pos = pos;
+
+		list_add_tail(&entry->list, &dev->msi_list);
+	}
+
+	return nvec;
+}
+
+static void msix_program_entries(struct pci_dev *dev, void __iomem *base,
+					struct msix_entry *entries)
+{
+	struct msi_desc *entry;
+	int i = 0;
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		int j = entries[i].entry;
+		entries[i].vector = entry->irq;
+		entry->mask_base = base;
+		set_irq_msi(entry->irq, entry);
+		entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE +
+					PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+		msix_mask_irq(entry, 1);
+		i++;
+	}
+}
+
 /**
  * msix_capability_init - configure device's MSI-X capability
  * @dev: pointer to the pci_dev data structure of MSI-X device function
@@ -417,12 +473,8 @@  static int msi_capability_init(struct pci_dev *dev, int nvec)
 static int msix_capability_init(struct pci_dev *dev,
 				struct msix_entry *entries, int nvec)
 {
-	struct msi_desc *entry;
-	int pos, i, j, nr_entries, ret;
-	unsigned long phys_addr;
-	u32 table_offset;
+	int pos, nr_entries, ret;
  	u16 control;
-	u8 bir;
 	void __iomem *base;
 
    	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
@@ -432,47 +484,15 @@  static int msix_capability_init(struct pci_dev *dev,
 	control &= ~PCI_MSIX_FLAGS_ENABLE;
 	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
 
-	/* Request & Map MSI-X table region */
-	nr_entries = multi_msix_capable(control);
-
- 	pci_read_config_dword(dev, msix_table_offset_reg(pos), &table_offset);
-	bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
-	table_offset &= ~PCI_MSIX_FLAGS_BIRMASK;
-	phys_addr = pci_resource_start (dev, bir) + table_offset;
-	base = ioremap_nocache(phys_addr, nr_entries * PCI_MSIX_ENTRY_SIZE);
+	base = msix_map_region(dev, pos, multi_msix_capable(control));
 	if (base == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < nvec; i++) {
-		entry = alloc_msi_entry(dev);
-		if (!entry)
-			break;
-
- 		j = entries[i].entry;
-		entry->msi_attrib.is_msix = 1;
-		entry->msi_attrib.is_64 = 1;
-		entry->msi_attrib.entry_nr = j;
-		entry->msi_attrib.default_irq = dev->irq;
-		entry->msi_attrib.pos = pos;
-		entry->mask_base = base;
-
-		list_add_tail(&entry->list, &dev->msi_list);
-	}
+	nr_entries = msix_setup_entries(dev, pos, entries, nvec);
 
 	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
-	if (ret < 0) {
-		/* If we had some success report the number of irqs
-		 * we succeeded in setting up. */
-		int avail = 0;
-		list_for_each_entry(entry, &dev->msi_list, list) {
-			if (entry->irq != 0) {
-				avail++;
-			}
-		}
-
-		if (avail != 0)
-			ret = avail;
-	}
+	if (ret < 0 && nr_entries > 0)
+		ret = nr_entries;
 
 	if (ret) {
 		msi_free_irqs(dev);
@@ -487,16 +507,7 @@  static int msix_capability_init(struct pci_dev *dev,
 	control |= PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE;
 	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
 
-	i = 0;
-	list_for_each_entry(entry, &dev->msi_list, list) {
-		entries[i].vector = entry->irq;
-		set_irq_msi(entry->irq, entry);
-		j = entries[i].entry;
-		entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE +
-					PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
-		msix_mask_irq(entry, 1);
-		i++;
-	}
+	msix_program_entries(dev, base, entries);
 
 	/* Set MSI-X enabled bits and unmask the function */
 	pci_intx_for_msi(dev, 0);