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@xxxxxxxxxx>
>> ---
>> 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
>>
>
>
--
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
