[PATCH v2 0/5] Some cleanup for MSI code

[PATCH v2 0/5] Some cleanup for MSI code

[Yijing Wang]

This is collection of two cleanup patchset,
http://marc.info/?l=linux-pci&m=140807095026813&w=2 and
http://marc.info/?l=linux-pci&m=140633779729162&w=2

I rebased it on 3.17-rc1, and dropped a incorrect patch.

Yijing Wang (5):
  PCI/MSI: Clean up the kobject in struct msi_desc
  PCI/MSI: Remove msi_attrib->pos in struct msi_desc
  PCI/MSI: Change msi_bus attribute to support enable/disable MSI for
    EP
  MSI: Use __get_cached_msi_msg() instead of get_cached_msi_msg()
  MSI: Use __read_msi_msg() instead of read_msi_msg()

 arch/ia64/kernel/msi_ia64.c          |    2 +-
 arch/ia64/sn/kernel/msi_sn.c         |    4 ++--
 arch/mips/pci/msi-octeon.c           |    6 +++---
 arch/powerpc/platforms/pseries/msi.c |    2 +-
 arch/x86/kernel/apic/io_apic.c       |    2 +-
 arch/x86/pci/xen.c                   |    2 +-
 drivers/pci/host/pcie-designware.c   |    2 +-
 drivers/pci/msi.c                    |   31 ++-----------------------------
 drivers/pci/pci-sysfs.c              |   12 ++++++------
 include/linux/msi.h                  |    9 ++-------
 10 files changed, 20 insertions(+), 52 deletions(-)

[PATCH v2 1/5] PCI/MSI: Clean up the kobject in struct msi_desc

[Yijing Wang]

commit 1c51b50c2995 using attributes to export MSI mode
instead of kobject. So clean up the kobject in struct
msi_desc.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/pci/msi.c   |   11 -----------
 include/linux/msi.h |    2 --
 2 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 5a40516..e2aa74e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -384,17 +384,6 @@ static void free_msi_irqs(struct pci_dev *dev)
 				iounmap(entry->mask_base);
 		}
 
-		/*
-		 * Its possible that we get into this path
-		 * When populate_msi_sysfs fails, which means the entries
-		 * were not registered with sysfs.  In that case don't
-		 * unregister them.
-		 */
-		if (entry->kobj.parent) {
-			kobject_del(&entry->kobj);
-			kobject_put(&entry->kobj);
-		}
-
 		list_del(&entry->list);
 		kfree(entry);
 	}
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8103f32..8892d41 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -47,8 +47,6 @@ struct msi_desc {
 
 	/* Last set MSI message */
 	struct msi_msg msg;
-
-	struct kobject kobj;
 };
 
 /*
-- 
1.7.1

[PATCH v2 2/5] PCI/MSI: Remove msi_attrib->pos in struct msi_desc

[Yijing Wang]

Use pci_dev->msi_cap or pci_dev->msix_cap instead of
msi_desc->msi_attrib.pos, and remove pos member.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 arch/mips/pci/msi-octeon.c         |    6 +++---
 drivers/pci/host/pcie-designware.c |    2 +-
 drivers/pci/msi.c                  |    2 --
 include/linux/msi.h                |    1 -
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/mips/pci/msi-octeon.c b/arch/mips/pci/msi-octeon.c
index ab0c5d1..c8814be 100644
--- a/arch/mips/pci/msi-octeon.c
+++ b/arch/mips/pci/msi-octeon.c
@@ -73,8 +73,8 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
 	 * wants.  Most devices only want 1, which will give
 	 * configured_private_bits and request_private_bits equal 0.
 	 */
-	pci_read_config_word(dev, desc->msi_attrib.pos + PCI_MSI_FLAGS,
-			     &control);
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
+			&control);
 
 	/*
 	 * If the number of private bits has been configured then use
@@ -176,7 +176,7 @@ msi_irq_allocated:
 	/* Update the number of IRQs the device has available to it */
 	control &= ~PCI_MSI_FLAGS_QSIZE;
 	control |= request_private_bits << 4;
-	pci_write_config_word(dev, desc->msi_attrib.pos + PCI_MSI_FLAGS,
+	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
 			      control);
 
 	irq_set_msi_desc(irq, desc);
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52bd3a1..c3706cb 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -355,7 +355,7 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 		return -EINVAL;
 	}
 
-	pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
+	pci_read_config_word(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
 				&msg_ctr);
 	msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
 	if (msgvec == 0)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e2aa74e..0d0f163 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -584,7 +584,6 @@ static struct msi_desc *msi_setup_entry(struct pci_dev *dev)
 	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)
@@ -688,7 +687,6 @@ static int msix_setup_entries(struct pci_dev *dev, void __iomem *base,
 		entry->msi_attrib.is_64		= 1;
 		entry->msi_attrib.entry_nr	= entries[i].entry;
 		entry->msi_attrib.default_irq	= dev->irq;
-		entry->msi_attrib.pos		= dev->msix_cap;
 		entry->mask_base		= base;
 
 		list_add_tail(&entry->list, &dev->msi_list);
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8892d41..fff7201 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -29,7 +29,6 @@ struct msi_desc {
 		__u8	multi_cap : 3;	/* log2 num of messages supported */
 		__u8	maskbit	: 1;	/* mask-pending bit supported ? */
 		__u8	is_64	: 1;	/* Address size: 0=32bit 1=64bit */
-		__u8	pos;		/* Location of the msi capability */
 		__u16	entry_nr;	/* specific enabled entry */
 		unsigned default_irq;	/* default pre-assigned irq */
 	} msi_attrib;
-- 
1.7.1

[PATCH v2 3/5] PCI/MSI: Change msi_bus attribute to support enable/disable MSI for EP

[Yijing Wang]

Msi_bus attribute is only valid for bridge device.
We can enable or disable MSI capability for a bus,
if we echo 1/0 > /sys/bus/pci/devices/$EP/msi_bus,
the action will be ignored. Sometime we need to
only enable/disable a EP device MSI capability,
not all devices share the same bus.

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

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9ff0a90..b199ad9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -251,11 +251,9 @@ static ssize_t msi_bus_show(struct device *dev, struct device_attribute *attr,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (!pdev->subordinate)
-		return 0;
-
-	return sprintf(buf, "%u\n",
-		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI));
+	return sprintf(buf, "%u\n", pdev->subordinate ?
+		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			   : !pdev->no_msi);
 }
 
 static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
@@ -278,8 +276,10 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
 	 * Maybe devices without subordinate buses shouldn't have this
 	 * attribute in the first place?
 	 */
-	if (!pdev->subordinate)
+	if (!pdev->subordinate) {
+		pdev->no_msi = !val;
 		return count;
+	}
 
 	/* Is the flag going to change, or keep the value it already had? */
 	if (!(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI) ^
-- 
1.7.1

[PATCH v2 4/5] MSI: Use __get_cached_msi_msg() instead of get_cached_msi_msg()

[Yijing Wang]

Get_cached_msi_msg() only be called in two places,
ia64_set_msi_irq_affinity() and sn_set_msi_irq_affinity().

The code flow is:
irq = irq_data->irq
    get_cached_msi_msg(irq)
	    irq_get_msi_desc(irq)
		    irq_get_irq_data(irq)
			    msi_desc = irq_data->desc
        __get_cached_msi_msg(msi_desc, msg)

This is crazy, we should use __get_cached_msi_msg(irq_data->desc, msg)
directly to simplify the flow, and remove get_cached_msi_msg().
Finally, rename __get_cached_msi_msg() to get_cached_msi_msg().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: Tony Luck <tony.luck@intel.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: x86@kernel.org
CC: linux-ia64@vger.kernel.org
---
 arch/ia64/kernel/msi_ia64.c    |    2 +-
 arch/ia64/sn/kernel/msi_sn.c   |    4 ++--
 arch/x86/kernel/apic/io_apic.c |    2 +-
 drivers/pci/msi.c              |    9 +--------
 include/linux/msi.h            |    3 +--
 5 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/arch/ia64/kernel/msi_ia64.c b/arch/ia64/kernel/msi_ia64.c
index c430f91..4efe748 100644
--- a/arch/ia64/kernel/msi_ia64.c
+++ b/arch/ia64/kernel/msi_ia64.c
@@ -23,7 +23,7 @@ static int ia64_set_msi_irq_affinity(struct irq_data *idata,
 	if (irq_prepare_move(irq, cpu))
 		return -1;
 
-	get_cached_msi_msg(irq, &msg);
+	get_cached_msi_msg(idata->msi_desc, &msg);
 
 	addr = msg.address_lo;
 	addr &= MSI_ADDR_DEST_ID_MASK;
diff --git a/arch/ia64/sn/kernel/msi_sn.c b/arch/ia64/sn/kernel/msi_sn.c
index afc58d2..8bec242 100644
--- a/arch/ia64/sn/kernel/msi_sn.c
+++ b/arch/ia64/sn/kernel/msi_sn.c
@@ -175,8 +175,8 @@ static int sn_set_msi_irq_affinity(struct irq_data *data,
 	 * Release XIO resources for the old MSI PCI address
 	 */
 
-	get_cached_msi_msg(irq, &msg);
-        sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
+	get_cached_msi_msg(data->msi_desc, &msg);
+	sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
 	pdev = sn_pdev->pdi_linux_pcidev;
 	provider = SN_PCIDEV_BUSPROVIDER(pdev);
 
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 29290f5..e877cfb 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3145,7 +3145,7 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 	if (ret)
 		return ret;
 
-	__get_cached_msi_msg(data->msi_desc, &msg);
+	get_cached_msi_msg(data->msi_desc, &msg);
 
 	msg.data &= ~MSI_DATA_VECTOR_MASK;
 	msg.data |= MSI_DATA_VECTOR(cfg->vector);
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0d0f163..8746ecd 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -296,7 +296,7 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
 	__read_msi_msg(entry, msg);
 }
 
-void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
+void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	/* Assert that the cache is valid, assuming that
 	 * valid messages are not all-zeroes. */
@@ -306,13 +306,6 @@ void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 	*msg = entry->msg;
 }
 
-void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
-{
-	struct msi_desc *entry = irq_get_msi_desc(irq);
-
-	__get_cached_msi_msg(entry, msg);
-}
-
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	if (entry->dev->current_state != PCI_D0) {
diff --git a/include/linux/msi.h b/include/linux/msi.h
index fff7201..329ec73 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -16,10 +16,9 @@ struct msi_desc;
 void mask_msi_irq(struct irq_data *data);
 void unmask_msi_irq(struct irq_data *data);
 void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
-void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
+void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void read_msi_msg(unsigned int irq, struct msi_msg *msg);
-void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
 void write_msi_msg(unsigned int irq, struct msi_msg *msg);
 
 struct msi_desc {
-- 
1.7.1

[PATCH v2 5/5] MSI: Use __read_msi_msg() instead of read_msi_msg()

[Yijing Wang]

Read_msi_msg() only be called in rtas_setup_msi_irqs(),
use __read_msi_msg() instead of read_msi_msg for
simplification. And rename __read_msi_msg() to
read_msi_msg().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/platforms/pseries/msi.c |    2 +-
 arch/x86/pci/xen.c                   |    2 +-
 drivers/pci/msi.c                    |    9 +--------
 include/linux/msi.h                  |    3 +--
 4 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 18ff462..7e87878 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -485,7 +485,7 @@ again:
 		irq_set_msi_desc(virq, entry);
 
 		/* Read config space back so we can restore after reset */
-		read_msi_msg(virq, &msg);
+		read_msi_msg(entry, &msg);
 		entry->msg = msg;
 	}
 
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 093f5f4..ad03739 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -229,7 +229,7 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		return 1;
 
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		__read_msi_msg(msidesc, &msg);
+		read_msi_msg(msidesc, &msg);
 		pirq = MSI_ADDR_EXT_DEST_ID(msg.address_hi) |
 			((msg.address_lo >> MSI_ADDR_DEST_ID_SHIFT) & 0xff);
 		if (msg.data != XEN_PIRQ_MSI_DATA ||
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 8746ecd..aff384a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -259,7 +259,7 @@ void default_restore_msi_irqs(struct pci_dev *dev)
 	}
 }
 
-void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
+void read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	BUG_ON(entry->dev->current_state != PCI_D0);
 
@@ -289,13 +289,6 @@ void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 	}
 }
 
-void read_msi_msg(unsigned int irq, struct msi_msg *msg)
-{
-	struct msi_desc *entry = irq_get_msi_desc(irq);
-
-	__read_msi_msg(entry, msg);
-}
-
 void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	/* Assert that the cache is valid, assuming that
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 329ec73..6be66f4 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -15,10 +15,9 @@ struct irq_data;
 struct msi_desc;
 void mask_msi_irq(struct irq_data *data);
 void unmask_msi_irq(struct irq_data *data);
-void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
+void read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
-void read_msi_msg(unsigned int irq, struct msi_msg *msg);
 void write_msi_msg(unsigned int irq, struct msi_msg *msg);
 
 struct msi_desc {
-- 
1.7.1

Re: [PATCH v2 5/5] MSI: Use __read_msi_msg() instead of read_msi_msg()

[Michael Ellerman]

On Fri, 2014-08-22 at 16:08 +0800, Yijing Wang wrote:
> Read_msi_msg() only be called in rtas_setup_msi_irqs(),
> use __read_msi_msg() instead of read_msi_msg for
> simplification. And rename __read_msi_msg() to
> read_msi_msg().
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: linuxppc-dev@lists.ozlabs.org

LGTM.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

Re: [PATCH v2 5/5] MSI: Use __read_msi_msg() instead of read_msi_msg()

[Yijing Wang]

On 2014/9/16 14:34, Michael Ellerman wrote:
> On Fri, 2014-08-22 at 16:08 +0800, Yijing Wang wrote:
>> Read_msi_msg() only be called in rtas_setup_msi_irqs(),
>> use __read_msi_msg() instead of read_msi_msg for
>> simplification. And rename __read_msi_msg() to
>> read_msi_msg().
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: linuxppc-dev@lists.ozlabs.org
> 
> LGTM.
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

Thanks!

> 
> cheers
> 
> 
> 
> 


-- 
Thanks!
Yijing

Re: [PATCH v2 3/5] PCI/MSI: Change msi_bus attribute to support enable/disable MSI for EP

[Bjorn Helgaas]

On Fri, Aug 22, 2014 at 04:07:58PM +0800, Yijing Wang wrote:
> Msi_bus attribute is only valid for bridge device.
> We can enable or disable MSI capability for a bus,
> if we echo 1/0 > /sys/bus/pci/devices/$EP/msi_bus,
> the action will be ignored. Sometime we need to
> only enable/disable a EP device MSI capability,
> not all devices share the same bus.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

What's the purpose of this?  Is this just for debugging?  I assume that if
you have an endpoint that requires MSI to be disabled, you'd have a quirk
to do that, not a sysfs interface.

This should probably be mentioned in
Documentation/ABI/testing/sysfs-bus-pci while you're at it.  I know it's
not there yet, but this seems like a good time to rectify that omission.

> ---
>  drivers/pci/pci-sysfs.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9ff0a90..b199ad9 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -251,11 +251,9 @@ static ssize_t msi_bus_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  
> -	if (!pdev->subordinate)
> -		return 0;
> -
> -	return sprintf(buf, "%u\n",
> -		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI));
> +	return sprintf(buf, "%u\n", pdev->subordinate ?
> +		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> +			   : !pdev->no_msi);
>  }
>  
>  static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
> @@ -278,8 +276,10 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
>  	 * Maybe devices without subordinate buses shouldn't have this
>  	 * attribute in the first place?
>  	 */
> -	if (!pdev->subordinate)
> +	if (!pdev->subordinate) {
> +		pdev->no_msi = !val;
>  		return count;
> +	}
>  
>  	/* Is the flag going to change, or keep the value it already had? */
>  	if (!(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI) ^
> -- 
> 1.7.1
> 

Re: [PATCH v2 4/5] MSI: Use __get_cached_msi_msg() instead of get_cached_msi_msg()

[Bjorn Helgaas]

On Fri, Aug 22, 2014 at 04:07:59PM +0800, Yijing Wang wrote:
> Get_cached_msi_msg() only be called in two places,
> ia64_set_msi_irq_affinity() and sn_set_msi_irq_affinity().
> 
> The code flow is:
> irq = irq_data->irq
>     get_cached_msi_msg(irq)
> 	    irq_get_msi_desc(irq)
> 		    irq_get_irq_data(irq)
> 			    msi_desc = irq_data->desc
>         __get_cached_msi_msg(msi_desc, msg)
> 
> This is crazy, we should use __get_cached_msi_msg(irq_data->desc, msg)
> directly to simplify the flow, and remove get_cached_msi_msg().
> Finally, rename __get_cached_msi_msg() to get_cached_msi_msg().

This looks like a nice cleanup.  It'd be easier to review if this were two
patches:

  1) Convert all callers to use __get_cached_msi_msg() rather than
     get_cached_msi_msg().
  2) Remove the unused get_cached_msi_msg() and rename
     __get_cached_msi_msg() to get_cached_msi_msg().

Maybe even the second patch should be split into remove and rename patches.

> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: x86@kernel.org
> CC: linux-ia64@vger.kernel.org
> ---
>  arch/ia64/kernel/msi_ia64.c    |    2 +-
>  arch/ia64/sn/kernel/msi_sn.c   |    4 ++--
>  arch/x86/kernel/apic/io_apic.c |    2 +-
>  drivers/pci/msi.c              |    9 +--------
>  include/linux/msi.h            |    3 +--
>  5 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/ia64/kernel/msi_ia64.c b/arch/ia64/kernel/msi_ia64.c
> index c430f91..4efe748 100644
> --- a/arch/ia64/kernel/msi_ia64.c
> +++ b/arch/ia64/kernel/msi_ia64.c
> @@ -23,7 +23,7 @@ static int ia64_set_msi_irq_affinity(struct irq_data *idata,
>  	if (irq_prepare_move(irq, cpu))
>  		return -1;
>  
> -	get_cached_msi_msg(irq, &msg);
> +	get_cached_msi_msg(idata->msi_desc, &msg);
>  
>  	addr = msg.address_lo;
>  	addr &= MSI_ADDR_DEST_ID_MASK;
> diff --git a/arch/ia64/sn/kernel/msi_sn.c b/arch/ia64/sn/kernel/msi_sn.c
> index afc58d2..8bec242 100644
> --- a/arch/ia64/sn/kernel/msi_sn.c
> +++ b/arch/ia64/sn/kernel/msi_sn.c
> @@ -175,8 +175,8 @@ static int sn_set_msi_irq_affinity(struct irq_data *data,
>  	 * Release XIO resources for the old MSI PCI address
>  	 */
>  
> -	get_cached_msi_msg(irq, &msg);
> -        sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
> +	get_cached_msi_msg(data->msi_desc, &msg);
> +	sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
>  	pdev = sn_pdev->pdi_linux_pcidev;
>  	provider = SN_PCIDEV_BUSPROVIDER(pdev);
>  
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 29290f5..e877cfb 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -3145,7 +3145,7 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
>  	if (ret)
>  		return ret;
>  
> -	__get_cached_msi_msg(data->msi_desc, &msg);
> +	get_cached_msi_msg(data->msi_desc, &msg);
>  
>  	msg.data &= ~MSI_DATA_VECTOR_MASK;
>  	msg.data |= MSI_DATA_VECTOR(cfg->vector);
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 0d0f163..8746ecd 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -296,7 +296,7 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>  	__read_msi_msg(entry, msg);
>  }
>  
> -void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> +void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
>  	/* Assert that the cache is valid, assuming that
>  	 * valid messages are not all-zeroes. */
> @@ -306,13 +306,6 @@ void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  	*msg = entry->msg;
>  }
>  
> -void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
> -{
> -	struct msi_desc *entry = irq_get_msi_desc(irq);
> -
> -	__get_cached_msi_msg(entry, msg);
> -}
> -
>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
>  	if (entry->dev->current_state != PCI_D0) {
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index fff7201..329ec73 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -16,10 +16,9 @@ struct msi_desc;
>  void mask_msi_irq(struct irq_data *data);
>  void unmask_msi_irq(struct irq_data *data);
>  void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
> -void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
> +void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  void read_msi_msg(unsigned int irq, struct msi_msg *msg);
> -void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>  void write_msi_msg(unsigned int irq, struct msi_msg *msg);
>  
>  struct msi_desc {
> -- 
> 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 v2 3/5] PCI/MSI: Change msi_bus attribute to support enable/disable MSI for EP

[Yijing Wang]

On 2014/9/23 7:03, Bjorn Helgaas wrote:
> On Fri, Aug 22, 2014 at 04:07:58PM +0800, Yijing Wang wrote:
>> Msi_bus attribute is only valid for bridge device.
>> We can enable or disable MSI capability for a bus,
>> if we echo 1/0 > /sys/bus/pci/devices/$EP/msi_bus,
>> the action will be ignored. Sometime we need to
>> only enable/disable a EP device MSI capability,
>> not all devices share the same bus.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> 
> What's the purpose of this?  Is this just for debugging?  I assume that if
> you have an endpoint that requires MSI to be disabled, you'd have a quirk
> to do that, not a sysfs interface.

Hi Bjorn, yes, the purpose of doing this is to debug and provide a interface to control
device MSI capability. Sometimes we only want to force a EP device to use legacy interrupt,
Eg. in x86, there are limit vector resources, and now most device driver uses MSI to
deliver interrupts, so the vector resources will be easy to be exhausted. Implement it for
EP will let system administrator have more ability to optimize MSI interrupts.

> 
> This should probably be mentioned in
> Documentation/ABI/testing/sysfs-bus-pci while you're at it.  I know it's
> not there yet, but this seems like a good time to rectify that omission.

OK, I will do it.

Thanks!
Yijing.

> 
>> ---
>>  drivers/pci/pci-sysfs.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 9ff0a90..b199ad9 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -251,11 +251,9 @@ static ssize_t msi_bus_show(struct device *dev, struct device_attribute *attr,
>>  {
>>  	struct pci_dev *pdev = to_pci_dev(dev);
>>  
>> -	if (!pdev->subordinate)
>> -		return 0;
>> -
>> -	return sprintf(buf, "%u\n",
>> -		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI));
>> +	return sprintf(buf, "%u\n", pdev->subordinate ?
>> +		       !(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>> +			   : !pdev->no_msi);
>>  }
>>  
>>  static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
>> @@ -278,8 +276,10 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
>>  	 * Maybe devices without subordinate buses shouldn't have this
>>  	 * attribute in the first place?
>>  	 */
>> -	if (!pdev->subordinate)
>> +	if (!pdev->subordinate) {
>> +		pdev->no_msi = !val;
>>  		return count;
>> +	}
>>  
>>  	/* Is the flag going to change, or keep the value it already had? */
>>  	if (!(pdev->subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI) ^
>> -- 
>> 1.7.1
>>
> 
> 


-- 
Thanks!
Yijing

Re: [PATCH v2 4/5] MSI: Use __get_cached_msi_msg() instead of get_cached_msi_msg()

[Yijing Wang]

On 2014/9/23 7:08, Bjorn Helgaas wrote:
> On Fri, Aug 22, 2014 at 04:07:59PM +0800, Yijing Wang wrote:
>> Get_cached_msi_msg() only be called in two places,
>> ia64_set_msi_irq_affinity() and sn_set_msi_irq_affinity().
>>
>> The code flow is:
>> irq = irq_data->irq
>>     get_cached_msi_msg(irq)
>> 	    irq_get_msi_desc(irq)
>> 		    irq_get_irq_data(irq)
>> 			    msi_desc = irq_data->desc
>>         __get_cached_msi_msg(msi_desc, msg)
>>
>> This is crazy, we should use __get_cached_msi_msg(irq_data->desc, msg)
>> directly to simplify the flow, and remove get_cached_msi_msg().
>> Finally, rename __get_cached_msi_msg() to get_cached_msi_msg().
> 
> This looks like a nice cleanup.  It'd be easier to review if this were two
> patches:
> 
>   1) Convert all callers to use __get_cached_msi_msg() rather than
>      get_cached_msi_msg().
>   2) Remove the unused get_cached_msi_msg() and rename
>      __get_cached_msi_msg() to get_cached_msi_msg().
> 
> Maybe even the second patch should be split into remove and rename patches.

OK.

Thanks!
Yijing.

> 
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> CC: Tony Luck <tony.luck@intel.com>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: x86@kernel.org
>> CC: linux-ia64@vger.kernel.org
>> ---
>>  arch/ia64/kernel/msi_ia64.c    |    2 +-
>>  arch/ia64/sn/kernel/msi_sn.c   |    4 ++--
>>  arch/x86/kernel/apic/io_apic.c |    2 +-
>>  drivers/pci/msi.c              |    9 +--------
>>  include/linux/msi.h            |    3 +--
>>  5 files changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/ia64/kernel/msi_ia64.c b/arch/ia64/kernel/msi_ia64.c
>> index c430f91..4efe748 100644
>> --- a/arch/ia64/kernel/msi_ia64.c
>> +++ b/arch/ia64/kernel/msi_ia64.c
>> @@ -23,7 +23,7 @@ static int ia64_set_msi_irq_affinity(struct irq_data *idata,
>>  	if (irq_prepare_move(irq, cpu))
>>  		return -1;
>>  
>> -	get_cached_msi_msg(irq, &msg);
>> +	get_cached_msi_msg(idata->msi_desc, &msg);
>>  
>>  	addr = msg.address_lo;
>>  	addr &= MSI_ADDR_DEST_ID_MASK;
>> diff --git a/arch/ia64/sn/kernel/msi_sn.c b/arch/ia64/sn/kernel/msi_sn.c
>> index afc58d2..8bec242 100644
>> --- a/arch/ia64/sn/kernel/msi_sn.c
>> +++ b/arch/ia64/sn/kernel/msi_sn.c
>> @@ -175,8 +175,8 @@ static int sn_set_msi_irq_affinity(struct irq_data *data,
>>  	 * Release XIO resources for the old MSI PCI address
>>  	 */
>>  
>> -	get_cached_msi_msg(irq, &msg);
>> -        sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
>> +	get_cached_msi_msg(data->msi_desc, &msg);
>> +	sn_pdev = (struct pcidev_info *)sn_irq_info->irq_pciioinfo;
>>  	pdev = sn_pdev->pdi_linux_pcidev;
>>  	provider = SN_PCIDEV_BUSPROVIDER(pdev);
>>  
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index 29290f5..e877cfb 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -3145,7 +3145,7 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
>>  	if (ret)
>>  		return ret;
>>  
>> -	__get_cached_msi_msg(data->msi_desc, &msg);
>> +	get_cached_msi_msg(data->msi_desc, &msg);
>>  
>>  	msg.data &= ~MSI_DATA_VECTOR_MASK;
>>  	msg.data |= MSI_DATA_VECTOR(cfg->vector);
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 0d0f163..8746ecd 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -296,7 +296,7 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>>  	__read_msi_msg(entry, msg);
>>  }
>>  
>> -void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>> +void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>>  {
>>  	/* Assert that the cache is valid, assuming that
>>  	 * valid messages are not all-zeroes. */
>> @@ -306,13 +306,6 @@ void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>>  	*msg = entry->msg;
>>  }
>>  
>> -void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>> -{
>> -	struct msi_desc *entry = irq_get_msi_desc(irq);
>> -
>> -	__get_cached_msi_msg(entry, msg);
>> -}
>> -
>>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>>  {
>>  	if (entry->dev->current_state != PCI_D0) {
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index fff7201..329ec73 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -16,10 +16,9 @@ struct msi_desc;
>>  void mask_msi_irq(struct irq_data *data);
>>  void unmask_msi_irq(struct irq_data *data);
>>  void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>> -void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>> +void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>>  void read_msi_msg(unsigned int irq, struct msi_msg *msg);
>> -void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>>  void write_msi_msg(unsigned int irq, struct msi_msg *msg);
>>  
>>  struct msi_desc {
>> -- 
>> 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