[PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()

[PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()

[Yijing Wang]

Move MSI entry stuff to a new function
msi_setup_entry(), simplify msi_capability_init()
code like MSI-X do.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/msi.c |   71 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d8e431d..8e17446 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -582,6 +582,35 @@ error_attrs:
 	return ret;
 }
 
+static int msi_setup_entry(struct pci_dev *dev)
+{
+	u16 control;
+	struct msi_desc *entry;
+
+	entry = alloc_msi_entry(dev);
+	if (!entry)
+		return -ENOMEM;
+
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
+
+	entry->msi_attrib.is_msix	= 0;
+	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
+	entry->msi_attrib.entry_nr	= 0;
+	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
+	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
+	entry->msi_attrib.pos		= dev->msi_cap;
+	entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
+
+	if (control & PCI_MSI_FLAGS_64BIT)
+		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
+	else
+		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+
+
+	list_add_tail(&entry->list, &dev->msi_list);
+	return 0;
+}
+
 /**
  * msi_capability_init - configure device's MSI capability structure
  * @dev: pointer to the pci_dev data structure of MSI device function
@@ -597,51 +626,30 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 {
 	struct msi_desc *entry;
 	int ret;
-	u16 control;
 	unsigned mask;
 
 	msi_set_enable(dev, 0);	/* Disable MSI during set up */
 
-	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
 	/* MSI Entry Initialization */
-	entry = alloc_msi_entry(dev);
-	if (!entry)
-		return -ENOMEM;
-
-	entry->msi_attrib.is_msix	= 0;
-	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
-	entry->msi_attrib.entry_nr	= 0;
-	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
-	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
-	entry->msi_attrib.pos		= dev->msi_cap;
-	entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
+	ret = msi_setup_entry(dev);
+	if (ret)
+		return ret;
 
-	if (control & PCI_MSI_FLAGS_64BIT)
-		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
-	else
-		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+	entry = list_first_entry(&dev->msi_list, struct msi_desc, list);
 	/* All MSIs are unmasked by default, Mask them all */
 	if (entry->msi_attrib.maskbit)
 		pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
 	mask = msi_mask(entry->msi_attrib.multi_cap);
 	msi_mask_irq(entry, mask, mask);
 
-	list_add_tail(&entry->list, &dev->msi_list);
-
 	/* Configure MSI capability structure */
 	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSI);
-	if (ret) {
-		msi_mask_irq(entry, mask, ~mask);
-		free_msi_irqs(dev);
-		return ret;
-	}
+	if (ret)
+		goto err;
 
 	ret = populate_msi_sysfs(dev);
-	if (ret) {
-		msi_mask_irq(entry, mask, ~mask);
-		free_msi_irqs(dev);
-		return ret;
-	}
+	if (ret)
+		goto err;
 
 	/* Set MSI enabled bits	 */
 	pci_intx_for_msi(dev, 0);
@@ -650,6 +658,11 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 
 	dev->irq = entry->irq;
 	return 0;
+
+err:
+	msi_mask_irq(entry, mask, ~mask);
+	free_msi_irqs(dev);
+	return ret;
 }
 
 static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
-- 
1.7.1

Re: [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()

[Bjorn Helgaas]

On Thu, Jun 19, 2014 at 04:30:48PM +0800, Yijing Wang wrote:
> Move MSI entry stuff to a new function
> msi_setup_entry(), simplify msi_capability_init()
> code like MSI-X do.

I like this in general, but ...

> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/msi.c |   71 +++++++++++++++++++++++++++++++---------------------
>  1 files changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d8e431d..8e17446 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -582,6 +582,35 @@ error_attrs:
>  	return ret;
>  }
>  
> +static int msi_setup_entry(struct pci_dev *dev)
> +{
> +	u16 control;
> +	struct msi_desc *entry;
> +
> +	entry = alloc_msi_entry(dev);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
> +
> +	entry->msi_attrib.is_msix	= 0;
> +	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
> +	entry->msi_attrib.entry_nr	= 0;
> +	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
> +	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
> +	entry->msi_attrib.pos		= dev->msi_cap;
> +	entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
> +
> +	if (control & PCI_MSI_FLAGS_64BIT)
> +		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
> +	else
> +		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
> +
> +
> +	list_add_tail(&entry->list, &dev->msi_list);
> +	return 0;

Why don't you just return "entry" here (and NULL for the failure above)?

> +}
> +
>  /**
>   * msi_capability_init - configure device's MSI capability structure
>   * @dev: pointer to the pci_dev data structure of MSI device function
> @@ -597,51 +626,30 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>  {
>  	struct msi_desc *entry;
>  	int ret;
> -	u16 control;
>  	unsigned mask;
>  
>  	msi_set_enable(dev, 0);	/* Disable MSI during set up */
>  
> -	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
>  	/* MSI Entry Initialization */
> -	entry = alloc_msi_entry(dev);
> -	if (!entry)
> -		return -ENOMEM;
> -
> -	entry->msi_attrib.is_msix	= 0;
> -	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
> -	entry->msi_attrib.entry_nr	= 0;
> -	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
> -	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
> -	entry->msi_attrib.pos		= dev->msi_cap;
> -	entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
> +	ret = msi_setup_entry(dev);
> +	if (ret)
> +		return ret;
>  
> -	if (control & PCI_MSI_FLAGS_64BIT)
> -		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
> -	else
> -		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
> +	entry = list_first_entry(&dev->msi_list, struct msi_desc, list);
>  	/* All MSIs are unmasked by default, Mask them all */
>  	if (entry->msi_attrib.maskbit)
>  		pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
>  	mask = msi_mask(entry->msi_attrib.multi_cap);
>  	msi_mask_irq(entry, mask, mask);
>  
> -	list_add_tail(&entry->list, &dev->msi_list);

And keep the list_add_tail() here.  Then you don't have the awkwardness of
pulling the entry off the list after calling msi_setup_entry().

That also means the MSIs can be masked before putting the entry on
dev->msi_list, as they were before.  It *might* be safe to change the order
as you did, but it definitely takes some analysis to prove it, especially
since pci_enable_msi_range() only WARNs but doesn't bail out if
dev->msi_enabled already.

>  	/* Configure MSI capability structure */
>  	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSI);
> -	if (ret) {
> -		msi_mask_irq(entry, mask, ~mask);
> -		free_msi_irqs(dev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err;
>  
>  	ret = populate_msi_sysfs(dev);
> -	if (ret) {
> -		msi_mask_irq(entry, mask, ~mask);
> -		free_msi_irqs(dev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err;
>  
>  	/* Set MSI enabled bits	 */
>  	pci_intx_for_msi(dev, 0);
> @@ -650,6 +658,11 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>  
>  	dev->irq = entry->irq;
>  	return 0;
> +
> +err:
> +	msi_mask_irq(entry, mask, ~mask);
> +	free_msi_irqs(dev);
> +	return ret;
>  }
>  
>  static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
> -- 
> 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

Re: [PATCH 4/5] PCI/MSI: MSI cleanup, msi_setup_entry()

[Yijing Wang]

>> +	list_add_tail(&entry->list, &dev->msi_list);
>> +	return 0;
> 
> Why don't you just return "entry" here (and NULL for the failure above)?

Hmmm, this is a good idea, return the entry here will be better

> 
>> +}
>> +
>>  /**
>>   * msi_capability_init - configure device's MSI capability structure
>>   * @dev: pointer to the pci_dev data structure of MSI device function
>> @@ -597,51 +626,30 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>>  
>> -	if (control & PCI_MSI_FLAGS_64BIT)
>> -		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
>> -	else
>> -		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
>> +	entry = list_first_entry(&dev->msi_list, struct msi_desc, list);
>>  	/* All MSIs are unmasked by default, Mask them all */
>>  	if (entry->msi_attrib.maskbit)
>>  		pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
>>  	mask = msi_mask(entry->msi_attrib.multi_cap);
>>  	msi_mask_irq(entry, mask, mask);
>>  
>> -	list_add_tail(&entry->list, &dev->msi_list);
> 
> And keep the list_add_tail() here.  Then you don't have the awkwardness of
> pulling the entry off the list after calling msi_setup_entry().
> 
> That also means the MSIs can be masked before putting the entry on
> dev->msi_list, as they were before.  It *might* be safe to change the order
> as you did, but it definitely takes some analysis to prove it, especially
> since pci_enable_msi_range() only WARNs but doesn't bail out if
> dev->msi_enabled already.
> 

Bjorn, Thanks for your review and comments, I will update this patch and resend it, Thanks!

>>  	/* Configure MSI capability structure */
>>  	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSI);
>> -	if (ret) {
>> -		msi_mask_irq(entry, mask, ~mask);
>> -		free_msi_irqs(dev);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		goto err;
>>  
>>  	ret = populate_msi_sysfs(dev);
>> -	if (ret) {
>> -		msi_mask_irq(entry, mask, ~mask);
>> -		free_msi_irqs(dev);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		goto err;
>>  
>>  	/* Set MSI enabled bits	 */
>>  	pci_intx_for_msi(dev, 0);
>> @@ -650,6 +658,11 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>>  
>>  	dev->irq = entry->irq;
>>  	return 0;
>> +
>> +err:
>> +	msi_mask_irq(entry, mask, ~mask);
>> +	free_msi_irqs(dev);
>> +	return ret;
>>  }
>>  
>>  static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
>> -- 
>> 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
> 
> .
> 


-- 
Thanks!
Yijing