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

[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 <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

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

[Bjorn Helgaas]

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
> 

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

[Yijing Wang]

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


-- 
Thanks!
Yijing