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 ...
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);
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
