[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-pci
Subject:    Re: [PATCH v2 1/2] PCI/MSI: Simplify default_restore_msi_irq()
From:       Bjorn Helgaas <bhelgaas () google ! com>
Date:       2014-09-30 21:06:24
Message-ID: 20140930210624.GE5625 () google ! com
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]