>> + list_add_tail(&entry->list, &dev->msi_list);
>> + return 0;
>
> Why don't you just return "entry" here (and NULL for the failure above)?
Hmmm, this is a good idea, return the entry here will be better
>
>> +}
>> +
>> /**
>> * msi_capability_init - configure device's MSI capability structure
>> * @dev: pointer to the pci_dev data structure of MSI device function
>> @@ -597,51 +626,30 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>>
>> - if (control & PCI_MSI_FLAGS_64BIT)
>> - entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
>> - else
>> - entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
>> + entry = list_first_entry(&dev->msi_list, struct msi_desc, list);
>> /* All MSIs are unmasked by default, Mask them all */
>> if (entry->msi_attrib.maskbit)
>> pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
>> mask = msi_mask(entry->msi_attrib.multi_cap);
>> msi_mask_irq(entry, mask, mask);
>>
>> - list_add_tail(&entry->list, &dev->msi_list);
>
> And keep the list_add_tail() here. Then you don't have the awkwardness of
> pulling the entry off the list after calling msi_setup_entry().
>
> That also means the MSIs can be masked before putting the entry on
> dev->msi_list, as they were before. It *might* be safe to change the order
> as you did, but it definitely takes some analysis to prove it, especially
> since pci_enable_msi_range() only WARNs but doesn't bail out if
> dev->msi_enabled already.
>
Bjorn, Thanks for your review and comments, I will update this patch and resend it, Thanks!
>> /* Configure MSI capability structure */
>> ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSI);
>> - if (ret) {
>> - msi_mask_irq(entry, mask, ~mask);
>> - free_msi_irqs(dev);
>> - return ret;
>> - }
>> + if (ret)
>> + goto err;
>>
>> ret = populate_msi_sysfs(dev);
>> - if (ret) {
>> - msi_mask_irq(entry, mask, ~mask);
>> - free_msi_irqs(dev);
>> - return ret;
>> - }
>> + if (ret)
>> + goto err;
>>
>> /* Set MSI enabled bits */
>> pci_intx_for_msi(dev, 0);
>> @@ -650,6 +658,11 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>>
>> dev->irq = entry->irq;
>> return 0;
>> +
>> +err:
>> + msi_mask_irq(entry, mask, ~mask);
>> + free_msi_irqs(dev);
>> + return ret;
>> }
>>
>> static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
>> --
>> 1.7.1
>>
>>
>> --
>> 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
>
> .
>
--
Thanks!
Yijing
--
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
