| Message ID | 1411979733-28523-1-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Sep 29, 2014 at 04:35:33PM +0800, Yijing Wang wrote: > 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. > 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. > Signed-off-by: Yijing Wang <wangyijing@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) > -- > 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
On 2014/10/1 5:06, Bjorn Helgaas wrote: > On Mon, Sep 29, 2014 at 04:35:33PM +0800, Yijing Wang wrote: >> 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. > > 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. > >> 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! > >> Signed-off-by: Yijing Wang <wangyijing@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) >> -- >> 1.7.1 >> > >
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)
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 <wangyijing@huawei.com> --- drivers/pci/msi.c | 13 ++----------- 1 files changed, 2 insertions(+), 11 deletions(-)