* [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()
@ 2014-06-19 8:30 Yijing Wang
2014-07-03 23:39 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Yijing Wang @ 2014-06-19 8:30 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Wuyun, Xinwei Hu, Yijing Wang
Move MSI entry stuff to a new function
msi_setup_entry(), simplify msi_capability_init()
code like MSI-X do.
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
drivers/pci/msi.c | 71 +++++++++++++++++++++++++++++++---------------------
1 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d8e431d..8e17446 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -582,6 +582,35 @@ error_attrs:
return ret;
}
+static int msi_setup_entry(struct pci_dev *dev)
+{
+ u16 control;
+ struct msi_desc *entry;
+
+ entry = alloc_msi_entry(dev);
+ if (!entry)
+ return -ENOMEM;
+
+ pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
+
+ entry->msi_attrib.is_msix = 0;
+ entry->msi_attrib.is_64 = !!(control & PCI_MSI_FLAGS_64BIT);
+ entry->msi_attrib.entry_nr = 0;
+ entry->msi_attrib.maskbit = !!(control & PCI_MSI_FLAGS_MASKBIT);
+ entry->msi_attrib.default_irq = dev->irq; /* Save IOAPIC IRQ */
+ entry->msi_attrib.pos = dev->msi_cap;
+ entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
+
+ 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;
+
+
+ list_add_tail(&entry->list, &dev->msi_list);
+ return 0;
+}
+
/**
* 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)
{
struct msi_desc *entry;
int ret;
- u16 control;
unsigned mask;
msi_set_enable(dev, 0); /* Disable MSI during set up */
- pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
/* MSI Entry Initialization */
- entry = alloc_msi_entry(dev);
- if (!entry)
- return -ENOMEM;
-
- entry->msi_attrib.is_msix = 0;
- entry->msi_attrib.is_64 = !!(control & PCI_MSI_FLAGS_64BIT);
- entry->msi_attrib.entry_nr = 0;
- entry->msi_attrib.maskbit = !!(control & PCI_MSI_FLAGS_MASKBIT);
- entry->msi_attrib.default_irq = dev->irq; /* Save IOAPIC IRQ */
- entry->msi_attrib.pos = dev->msi_cap;
- entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
+ ret = msi_setup_entry(dev);
+ if (ret)
+ return ret;
- 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);
-
/* 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()
2014-06-19 8:30 [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry() Yijing Wang
@ 2014-07-03 23:39 ` Bjorn Helgaas
2014-07-04 1:57 ` Yijing Wang
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2014-07-03 23:39 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Wuyun, Xinwei Hu
On Thu, Jun 19, 2014 at 04:30:48PM +0800, Yijing Wang wrote:
> Move MSI entry stuff to a new function
> msi_setup_entry(), simplify msi_capability_init()
> code like MSI-X do.
I like this in general, but ...
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
> drivers/pci/msi.c | 71 +++++++++++++++++++++++++++++++---------------------
> 1 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d8e431d..8e17446 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -582,6 +582,35 @@ error_attrs:
> return ret;
> }
>
> +static int msi_setup_entry(struct pci_dev *dev)
> +{
> + u16 control;
> + struct msi_desc *entry;
> +
> + entry = alloc_msi_entry(dev);
> + if (!entry)
> + return -ENOMEM;
> +
> + pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
> +
> + entry->msi_attrib.is_msix = 0;
> + entry->msi_attrib.is_64 = !!(control & PCI_MSI_FLAGS_64BIT);
> + entry->msi_attrib.entry_nr = 0;
> + entry->msi_attrib.maskbit = !!(control & PCI_MSI_FLAGS_MASKBIT);
> + entry->msi_attrib.default_irq = dev->irq; /* Save IOAPIC IRQ */
> + entry->msi_attrib.pos = dev->msi_cap;
> + entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
> +
> + 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;
> +
> +
> + list_add_tail(&entry->list, &dev->msi_list);
> + return 0;
Why don't you just return "entry" here (and NULL for the failure above)?
> +}
> +
> /**
> * 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)
> {
> struct msi_desc *entry;
> int ret;
> - u16 control;
> unsigned mask;
>
> msi_set_enable(dev, 0); /* Disable MSI during set up */
>
> - pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
> /* MSI Entry Initialization */
> - entry = alloc_msi_entry(dev);
> - if (!entry)
> - return -ENOMEM;
> -
> - entry->msi_attrib.is_msix = 0;
> - entry->msi_attrib.is_64 = !!(control & PCI_MSI_FLAGS_64BIT);
> - entry->msi_attrib.entry_nr = 0;
> - entry->msi_attrib.maskbit = !!(control & PCI_MSI_FLAGS_MASKBIT);
> - entry->msi_attrib.default_irq = dev->irq; /* Save IOAPIC IRQ */
> - entry->msi_attrib.pos = dev->msi_cap;
> - entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
> + ret = msi_setup_entry(dev);
> + if (ret)
> + return ret;
>
> - 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.
> /* 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()
2014-07-03 23:39 ` Bjorn Helgaas
@ 2014-07-04 1:57 ` Yijing Wang
0 siblings, 0 replies; 3+ messages in thread
From: Yijing Wang @ 2014-07-04 1:57 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Wuyun, Xinwei Hu
>> + 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-04 1:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-19 8:30 [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry() Yijing Wang
2014-07-03 23:39 ` Bjorn Helgaas
2014-07-04 1:57 ` Yijing Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.