[PATCH v2 1/2] PCI/MSI: Simplify default_restore_msi_irq()

[Yijing Wang]

Both MSI and MSI-X irq will be associated to msi_desc
in arch MSI code, e.g. in x86
arch_setup_msi_irqs()
native_setup_msi_irqs()
setup_msi_irq()
irq_set_msi_desc_off()

Use irq_get_msi_desc() to get the MSI-X msi_desc for
simplification. Also use __write_msi_msg() instead
of write_msi_msg() to avoid the redundant calls.

Signed-off-by: Yijing Wang <***@huawei.com>
---
drivers/pci/msi.c | 13 ++-----------
1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d077749..e0916ad 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -109,18 +109,9 @@ static void default_restore_msi_irq(struct pci_dev *dev, int irq)
{
struct msi_desc *entry;

- entry = NULL;
- if (dev->msix_enabled) {
- list_for_each_entry(entry, &dev->msi_list, list) {
- if (irq == entry->irq)
- break;
- }
- } else if (dev->msi_enabled) {
- entry = irq_get_msi_desc(irq);
- }
-
+ entry = irq_get_msi_desc(irq);
if (entry)
- write_msi_msg(irq, &entry->msg);
+ __write_msi_msg(entry, &entry->msg);
}

void __weak arch_restore_msi_irqs(struct pci_dev *dev)

[Bjorn Helgaas]

Post by Yijing Wang
Both MSI and MSI-X irq will be associated to msi_desc
in arch MSI code, e.g. in x86
arch_setup_msi_irqs()
native_setup_msi_irqs()
setup_msi_irq()
irq_set_msi_desc_off()
Use irq_get_msi_desc() to get the MSI-X msi_desc for
simplification.

Holy cow, this is a mess (not your patch, but the existing split between
common code and arch code). You showed one path above, but how am I
supposed to know that *all* the paths make this association correctly? If
all the paths do it, why isn't the association done in some common code to
begin with? Is this telling us that the arch interface is designed wrong?

No doubt there's some implicit knowledge, like "there's no way MSI can work
at all unless the arch code makes this association," but I don't know
enough about MSI to have that knowledge, and consequently I really can't
convince myself that this patch is safe for everybody.

Post by Yijing Wang
Also use __write_msi_msg() instead
of write_msi_msg() to avoid the redundant calls.

I applied this part to pci/msi for v3.18 since it's unrelated.

Post by Yijing Wang
---
drivers/pci/msi.c | 13 ++-----------
1 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d077749..e0916ad 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -109,18 +109,9 @@ static void default_restore_msi_irq(struct pci_dev *dev, int irq)
{
struct msi_desc *entry;
- entry = NULL;
- if (dev->msix_enabled) {
- list_for_each_entry(entry, &dev->msi_list, list) {
- if (irq == entry->irq)
- break;
- }
- } else if (dev->msi_enabled) {
- entry = irq_get_msi_desc(irq);
- }
-
+ entry = irq_get_msi_desc(irq);
if (entry)
- write_msi_msg(irq, &entry->msg);
+ __write_msi_msg(entry, &entry->msg);
}
void __weak arch_restore_msi_irqs(struct pci_dev *dev)
--
1.7.1

[Yijing Wang]

Post by Bjorn Helgaas

Post by Yijing Wang
Both MSI and MSI-X irq will be associated to msi_desc
in arch MSI code, e.g. in x86
arch_setup_msi_irqs()
native_setup_msi_irqs()
setup_msi_irq()
irq_set_msi_desc_off()
Use irq_get_msi_desc() to get the MSI-X msi_desc for
simplification.

Holy cow, this is a mess (not your patch, but the existing split between
common code and arch code). You showed one path above, but how am I
supposed to know that *all* the paths make this association correctly? If
all the paths do it, why isn't the association done in some common code to
begin with? Is this telling us that the arch interface is designed wrong?

Hi Bjorn, sorry for the late reply, in vocation last week.
Yes, I think this a mess, there is no similar association in MSI type common code,
only in MSI-X code, but MSI arch code allocates irq and process it in the same way for
both MSI and MSI-X irq. But I can't find why in git log now.

Post by Bjorn Helgaas
No doubt there's some implicit knowledge, like "there's no way MSI can work
at all unless the arch code makes this association," but I don't know
enough about MSI to have that knowledge, and consequently I really can't
convince myself that this patch is safe for everybody.

Ok, just keep it now.

Post by Bjorn Helgaas

Post by Yijing Wang
Also use __write_msi_msg() instead
of write_msi_msg() to avoid the redundant calls.

I applied this part to pci/msi for v3.18 since it's unrelated.

Thanks very much!

Post by Bjorn Helgaas

Post by Yijing Wang
---
drivers/pci/msi.c | 13 ++-----------
1 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d077749..e0916ad 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -109,18 +109,9 @@ static void default_restore_msi_irq(struct pci_dev *dev, int irq)
{
struct msi_desc *entry;
- entry = NULL;
- if (dev->msix_enabled) {
- list_for_each_entry(entry, &dev->msi_list, list) {
- if (irq == entry->irq)
- break;
- }
- } else if (dev->msi_enabled) {
- entry = irq_get_msi_desc(irq);
- }
-
+ entry = irq_get_msi_desc(irq);
if (entry)
- write_msi_msg(irq, &entry->msg);
+ __write_msi_msg(entry, &entry->msg);
}
void __weak arch_restore_msi_irqs(struct pci_dev *dev)
--
1.7.1