[PATCH] PCI: export MSI mode using attributes, not kobjects

[PATCH] PCI: export MSI mode using attributes, not kobjects

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

The PCI MSI sysfs code is a mess with kobjects for things that don't
really need to be kobjects.  This patch creates attributes dynamically
for the MSI interrupts instead of using kobjects.

Note, this removes a directory from the current MSI interrupt sysfs
code:

old MSI kobjects:
pci_device
   └── msi_irqs
       └── 40
           └── mode

new MSI attributes:
pci_device
   └── msi_irqs
       └── 40

As there was only one file "mode" with the kobject model, the interrupt
number is now a file that returns the "mode" of the interrupt (msi vs.
msix).

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---

Neil, can you test this patch to see if irqbalance works as-is or not?

thanks,

greg k-h

 drivers/pci/msi.c   |  152 +++++++++++++++++++++++++---------------------------
 include/linux/pci.h |    2 
 2 files changed, 76 insertions(+), 78 deletions(-)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -363,6 +363,9 @@ void write_msi_msg(unsigned int irq, str
 static void free_msi_irqs(struct pci_dev *dev)
 {
 	struct msi_desc *entry, *tmp;
+	struct attribute **msi_attrs;
+	struct device_attribute *dev_attr;
+	int count = 0;
 
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		int i, nvec;
@@ -398,6 +401,22 @@ static void free_msi_irqs(struct pci_dev
 		list_del(&entry->list);
 		kfree(entry);
 	}
+
+	if (dev->msi_irq_groups) {
+		sysfs_remove_groups(&dev->dev.kobj, dev->msi_irq_groups);
+		msi_attrs = dev->msi_irq_groups[0]->attrs;
+		list_for_each_entry(entry, &dev->msi_list, list) {
+			dev_attr = container_of(msi_attrs[count],
+						struct device_attribute, attr);
+			kfree(dev_attr->attr.name);
+			kfree(dev_attr);
+			++count;
+		}
+		kfree(msi_attrs);
+		kfree(dev->msi_irq_groups[0]);
+		kfree(dev->msi_irq_groups);
+		dev->msi_irq_groups = NULL;
+	}
 }
 
 static struct msi_desc *alloc_msi_entry(struct pci_dev *dev)
@@ -471,96 +490,79 @@ void pci_restore_msi_state(struct pci_de
 }
 EXPORT_SYMBOL_GPL(pci_restore_msi_state);
 
-
-#define to_msi_attr(obj) container_of(obj, struct msi_attribute, attr)
-#define to_msi_desc(obj) container_of(obj, struct msi_desc, kobj)
-
-struct msi_attribute {
-	struct attribute        attr;
-	ssize_t (*show)(struct msi_desc *entry, struct msi_attribute *attr,
-			char *buf);
-	ssize_t (*store)(struct msi_desc *entry, struct msi_attribute *attr,
-			 const char *buf, size_t count);
-};
-
-static ssize_t show_msi_mode(struct msi_desc *entry, struct msi_attribute *atr,
+static ssize_t msi_mode_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%s\n", entry->msi_attrib.is_msix ? "msix" : "msi");
-}
-
-static ssize_t msi_irq_attr_show(struct kobject *kobj,
-				 struct attribute *attr, char *buf)
-{
-	struct msi_attribute *attribute = to_msi_attr(attr);
-	struct msi_desc *entry = to_msi_desc(kobj);
-
-	if (!attribute->show)
-		return -EIO;
-
-	return attribute->show(entry, attribute, buf);
-}
-
-static const struct sysfs_ops msi_irq_sysfs_ops = {
-	.show = msi_irq_attr_show,
-};
-
-static struct msi_attribute mode_attribute =
-	__ATTR(mode, S_IRUGO, show_msi_mode, NULL);
-
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct msi_desc *entry;
+	unsigned long irq;
+	int retval;
 
-static struct attribute *msi_irq_default_attrs[] = {
-	&mode_attribute.attr,
-	NULL
-};
+	retval = kstrtoul(attr->attr.name, 10, &irq);
+	if (retval)
+		return retval;
 
-static void msi_kobj_release(struct kobject *kobj)
-{
-	struct msi_desc *entry = to_msi_desc(kobj);
-
-	pci_dev_put(entry->dev);
+	list_for_each_entry(entry, &pdev->msi_list, list) {
+		if (entry->irq == irq) {
+			return sprintf(buf, "%s\n",
+				       entry->msi_attrib.is_msix ? "msix" : "msi");
+		}
+	}
+	return -ENODEV;
 }
 
-static struct kobj_type msi_irq_ktype = {
-	.release = msi_kobj_release,
-	.sysfs_ops = &msi_irq_sysfs_ops,
-	.default_attrs = msi_irq_default_attrs,
-};
-
 static int populate_msi_sysfs(struct pci_dev *pdev)
 {
+	struct attribute **msi_attrs;
+	struct device_attribute *msi_dev_attr;
+	struct attribute_group *msi_irq_group;
+	const struct attribute_group **msi_irq_groups;
 	struct msi_desc *entry;
-	struct kobject *kobj;
 	int ret;
+	int num_msi = 0;
 	int count = 0;
 
-	pdev->msi_kset = kset_create_and_add("msi_irqs", NULL, &pdev->dev.kobj);
-	if (!pdev->msi_kset)
-		return -ENOMEM;
-
+	/* Determine how many msi entries we have */
 	list_for_each_entry(entry, &pdev->msi_list, list) {
-		kobj = &entry->kobj;
-		kobj->kset = pdev->msi_kset;
-		pci_dev_get(pdev);
-		ret = kobject_init_and_add(kobj, &msi_irq_ktype, NULL,
-				     "%u", entry->irq);
-		if (ret)
-			goto out_unroll;
-
-		count++;
+		++num_msi;
 	}
+	if (!num_msi)
+		return 0;
 
-	return 0;
-
-out_unroll:
+	/* Dynamically create the MSI attributes for the PCI device */
+	msi_attrs = kzalloc(sizeof(void *) * (num_msi + 1), GFP_KERNEL);
+	if (!msi_attrs)
+		return -ENOMEM;
 	list_for_each_entry(entry, &pdev->msi_list, list) {
-		if (!count)
-			break;
-		kobject_del(&entry->kobj);
-		kobject_put(&entry->kobj);
-		count--;
+		char *name = kmalloc(20, GFP_KERNEL);
+		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
+		if (!msi_dev_attr)
+			return -ENOMEM;
+		sprintf(name, "%d", entry->irq);
+		msi_dev_attr->attr.name = name;
+		msi_dev_attr->attr.mode = S_IRUGO;
+		msi_dev_attr->show = msi_mode_show;
+		msi_attrs[count] = &msi_dev_attr->attr;
+		++count;
 	}
-	return ret;
+
+	msi_irq_group = kzalloc(sizeof(*msi_irq_group), GFP_KERNEL);
+	if (!msi_irq_group)
+		return -ENOMEM;
+	msi_irq_group->name = "msi_irqs";
+	msi_irq_group->attrs = msi_attrs;
+
+	msi_irq_groups = kzalloc(sizeof(void *) * 2, GFP_KERNEL);
+	if (!msi_irq_groups)
+		return -ENOMEM;
+	msi_irq_groups[0] = msi_irq_group;
+
+	ret = sysfs_create_groups(&pdev->dev.kobj, msi_irq_groups);
+	if (ret)
+		return ret;
+	pdev->msi_irq_groups = msi_irq_groups;
+
+	return 0;
 }
 
 /**
@@ -925,8 +927,6 @@ void pci_disable_msi(struct pci_dev *dev
 
 	pci_msi_shutdown(dev);
 	free_msi_irqs(dev);
-	kset_unregister(dev->msi_kset);
-	dev->msi_kset = NULL;
 }
 EXPORT_SYMBOL(pci_disable_msi);
 
@@ -1023,8 +1023,6 @@ void pci_disable_msix(struct pci_dev *de
 
 	pci_msix_shutdown(dev);
 	free_msi_irqs(dev);
-	kset_unregister(dev->msi_kset);
-	dev->msi_kset = NULL;
 }
 EXPORT_SYMBOL(pci_disable_msix);
 
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -351,7 +351,7 @@ struct pci_dev {
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
 #ifdef CONFIG_PCI_MSI
 	struct list_head msi_list;
-	struct kset *msi_kset;
+	const struct attribute_group **msi_irq_groups;
 #endif
 	struct pci_vpd *vpd;
 #ifdef CONFIG_PCI_ATS

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Neil Horman]

On Wed, Nov 27, 2013 at 10:46:52AM -0800, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> The PCI MSI sysfs code is a mess with kobjects for things that don't
> really need to be kobjects.  This patch creates attributes dynamically
> for the MSI interrupts instead of using kobjects.
> 
> Note, this removes a directory from the current MSI interrupt sysfs
> code:
> 
> old MSI kobjects:
> pci_device
>    └── msi_irqs
>        └── 40
>            └── mode
> 
> new MSI attributes:
> pci_device
>    └── msi_irqs
>        └── 40
> 
> As there was only one file "mode" with the kobject model, the interrupt
> number is now a file that returns the "mode" of the interrupt (msi vs.
> msix).
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
> 
> Neil, can you test this patch to see if irqbalance works as-is or not?
> 
ACK, Testing with git-head irqbalance on F19 with this patch and it works fine.

Thanks!
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Veaceslav Falico]

On Wed, Nov 27, 2013 at 10:46:52AM -0800, Greg Kroah-Hartman wrote:
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
>The PCI MSI sysfs code is a mess with kobjects for things that don't
>really need to be kobjects.  This patch creates attributes dynamically
>for the MSI interrupts instead of using kobjects.
>
>Note, this removes a directory from the current MSI interrupt sysfs
>code:
>
>old MSI kobjects:
>pci_device
>   └── msi_irqs
>       └── 40
>           └── mode
>
>new MSI attributes:
>pci_device
>   └── msi_irqs
>       └── 40
>
>As there was only one file "mode" with the kobject model, the interrupt
>number is now a file that returns the "mode" of the interrupt (msi vs.
>msix).
>
>Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Works like a charm for me.

FWIW,

Reviewed-by: Veaceslav Falico <vfalico@redhat.com>

>
>---
>
>Neil, can you test this patch to see if irqbalance works as-is or not?
>
>thanks,
>
>greg k-h
>
> drivers/pci/msi.c   |  152 +++++++++++++++++++++++++---------------------------
> include/linux/pci.h |    2
> 2 files changed, 76 insertions(+), 78 deletions(-)
>
>--- a/drivers/pci/msi.c
>+++ b/drivers/pci/msi.c
>@@ -363,6 +363,9 @@ void write_msi_msg(unsigned int irq, str
> static void free_msi_irqs(struct pci_dev *dev)
> {
> 	struct msi_desc *entry, *tmp;
>+	struct attribute **msi_attrs;
>+	struct device_attribute *dev_attr;
>+	int count = 0;
>
> 	list_for_each_entry(entry, &dev->msi_list, list) {
> 		int i, nvec;
>@@ -398,6 +401,22 @@ static void free_msi_irqs(struct pci_dev
> 		list_del(&entry->list);
> 		kfree(entry);
> 	}
>+
>+	if (dev->msi_irq_groups) {
>+		sysfs_remove_groups(&dev->dev.kobj, dev->msi_irq_groups);
>+		msi_attrs = dev->msi_irq_groups[0]->attrs;
>+		list_for_each_entry(entry, &dev->msi_list, list) {
>+			dev_attr = container_of(msi_attrs[count],
>+						struct device_attribute, attr);
>+			kfree(dev_attr->attr.name);
>+			kfree(dev_attr);
>+			++count;
>+		}
>+		kfree(msi_attrs);
>+		kfree(dev->msi_irq_groups[0]);
>+		kfree(dev->msi_irq_groups);
>+		dev->msi_irq_groups = NULL;
>+	}
> }
>
> static struct msi_desc *alloc_msi_entry(struct pci_dev *dev)
>@@ -471,96 +490,79 @@ void pci_restore_msi_state(struct pci_de
> }
> EXPORT_SYMBOL_GPL(pci_restore_msi_state);
>
>-
>-#define to_msi_attr(obj) container_of(obj, struct msi_attribute, attr)
>-#define to_msi_desc(obj) container_of(obj, struct msi_desc, kobj)
>-
>-struct msi_attribute {
>-	struct attribute        attr;
>-	ssize_t (*show)(struct msi_desc *entry, struct msi_attribute *attr,
>-			char *buf);
>-	ssize_t (*store)(struct msi_desc *entry, struct msi_attribute *attr,
>-			 const char *buf, size_t count);
>-};
>-
>-static ssize_t show_msi_mode(struct msi_desc *entry, struct msi_attribute *atr,
>+static ssize_t msi_mode_show(struct device *dev, struct device_attribute *attr,
> 			     char *buf)
> {
>-	return sprintf(buf, "%s\n", entry->msi_attrib.is_msix ? "msix" : "msi");
>-}
>-
>-static ssize_t msi_irq_attr_show(struct kobject *kobj,
>-				 struct attribute *attr, char *buf)
>-{
>-	struct msi_attribute *attribute = to_msi_attr(attr);
>-	struct msi_desc *entry = to_msi_desc(kobj);
>-
>-	if (!attribute->show)
>-		return -EIO;
>-
>-	return attribute->show(entry, attribute, buf);
>-}
>-
>-static const struct sysfs_ops msi_irq_sysfs_ops = {
>-	.show = msi_irq_attr_show,
>-};
>-
>-static struct msi_attribute mode_attribute =
>-	__ATTR(mode, S_IRUGO, show_msi_mode, NULL);
>-
>+	struct pci_dev *pdev = to_pci_dev(dev);
>+	struct msi_desc *entry;
>+	unsigned long irq;
>+	int retval;
>
>-static struct attribute *msi_irq_default_attrs[] = {
>-	&mode_attribute.attr,
>-	NULL
>-};
>+	retval = kstrtoul(attr->attr.name, 10, &irq);
>+	if (retval)
>+		return retval;
>
>-static void msi_kobj_release(struct kobject *kobj)
>-{
>-	struct msi_desc *entry = to_msi_desc(kobj);
>-
>-	pci_dev_put(entry->dev);
>+	list_for_each_entry(entry, &pdev->msi_list, list) {
>+		if (entry->irq == irq) {
>+			return sprintf(buf, "%s\n",
>+				       entry->msi_attrib.is_msix ? "msix" : "msi");
>+		}
>+	}
>+	return -ENODEV;
> }
>
>-static struct kobj_type msi_irq_ktype = {
>-	.release = msi_kobj_release,
>-	.sysfs_ops = &msi_irq_sysfs_ops,
>-	.default_attrs = msi_irq_default_attrs,
>-};
>-
> static int populate_msi_sysfs(struct pci_dev *pdev)
> {
>+	struct attribute **msi_attrs;
>+	struct device_attribute *msi_dev_attr;
>+	struct attribute_group *msi_irq_group;
>+	const struct attribute_group **msi_irq_groups;
> 	struct msi_desc *entry;
>-	struct kobject *kobj;
> 	int ret;
>+	int num_msi = 0;
> 	int count = 0;
>
>-	pdev->msi_kset = kset_create_and_add("msi_irqs", NULL, &pdev->dev.kobj);
>-	if (!pdev->msi_kset)
>-		return -ENOMEM;
>-
>+	/* Determine how many msi entries we have */
> 	list_for_each_entry(entry, &pdev->msi_list, list) {
>-		kobj = &entry->kobj;
>-		kobj->kset = pdev->msi_kset;
>-		pci_dev_get(pdev);
>-		ret = kobject_init_and_add(kobj, &msi_irq_ktype, NULL,
>-				     "%u", entry->irq);
>-		if (ret)
>-			goto out_unroll;
>-
>-		count++;
>+		++num_msi;
> 	}
>+	if (!num_msi)
>+		return 0;
>
>-	return 0;
>-
>-out_unroll:
>+	/* Dynamically create the MSI attributes for the PCI device */
>+	msi_attrs = kzalloc(sizeof(void *) * (num_msi + 1), GFP_KERNEL);
>+	if (!msi_attrs)
>+		return -ENOMEM;
> 	list_for_each_entry(entry, &pdev->msi_list, list) {
>-		if (!count)
>-			break;
>-		kobject_del(&entry->kobj);
>-		kobject_put(&entry->kobj);
>-		count--;
>+		char *name = kmalloc(20, GFP_KERNEL);
>+		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
>+		if (!msi_dev_attr)
>+			return -ENOMEM;
>+		sprintf(name, "%d", entry->irq);
>+		msi_dev_attr->attr.name = name;
>+		msi_dev_attr->attr.mode = S_IRUGO;
>+		msi_dev_attr->show = msi_mode_show;
>+		msi_attrs[count] = &msi_dev_attr->attr;
>+		++count;
> 	}
>-	return ret;
>+
>+	msi_irq_group = kzalloc(sizeof(*msi_irq_group), GFP_KERNEL);
>+	if (!msi_irq_group)
>+		return -ENOMEM;
>+	msi_irq_group->name = "msi_irqs";
>+	msi_irq_group->attrs = msi_attrs;
>+
>+	msi_irq_groups = kzalloc(sizeof(void *) * 2, GFP_KERNEL);
>+	if (!msi_irq_groups)
>+		return -ENOMEM;
>+	msi_irq_groups[0] = msi_irq_group;
>+
>+	ret = sysfs_create_groups(&pdev->dev.kobj, msi_irq_groups);
>+	if (ret)
>+		return ret;
>+	pdev->msi_irq_groups = msi_irq_groups;
>+
>+	return 0;
> }
>
> /**
>@@ -925,8 +927,6 @@ void pci_disable_msi(struct pci_dev *dev
>
> 	pci_msi_shutdown(dev);
> 	free_msi_irqs(dev);
>-	kset_unregister(dev->msi_kset);
>-	dev->msi_kset = NULL;
> }
> EXPORT_SYMBOL(pci_disable_msi);
>
>@@ -1023,8 +1023,6 @@ void pci_disable_msix(struct pci_dev *de
>
> 	pci_msix_shutdown(dev);
> 	free_msi_irqs(dev);
>-	kset_unregister(dev->msi_kset);
>-	dev->msi_kset = NULL;
> }
> EXPORT_SYMBOL(pci_disable_msix);
>
>--- a/include/linux/pci.h
>+++ b/include/linux/pci.h
>@@ -351,7 +351,7 @@ struct pci_dev {
> 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
> #ifdef CONFIG_PCI_MSI
> 	struct list_head msi_list;
>-	struct kset *msi_kset;
>+	const struct attribute_group **msi_irq_groups;
> #endif
> 	struct pci_vpd *vpd;
> #ifdef CONFIG_PCI_ATS

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Greg KH]

On Fri, Nov 29, 2013 at 10:41:37AM +0100, Veaceslav Falico wrote:
> On Wed, Nov 27, 2013 at 10:46:52AM -0800, Greg Kroah-Hartman wrote:
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> >The PCI MSI sysfs code is a mess with kobjects for things that don't
> >really need to be kobjects.  This patch creates attributes dynamically
> >for the MSI interrupts instead of using kobjects.
> >
> >Note, this removes a directory from the current MSI interrupt sysfs
> >code:
> >
> >old MSI kobjects:
> >pci_device
> >   └── msi_irqs
> >       └── 40
> >           └── mode
> >
> >new MSI attributes:
> >pci_device
> >   └── msi_irqs
> >       └── 40
> >
> >As there was only one file "mode" with the kobject model, the interrupt
> >number is now a file that returns the "mode" of the interrupt (msi vs.
> >msix).
> >
> >Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Works like a charm for me.
> 
> FWIW,
> 
> Reviewed-by: Veaceslav Falico <vfalico@redhat.com>

On Thu, Nov 28, 2013 at 10:14:19PM -0500, Neil Horman wrote:
> ACK, Testing with git-head irqbalance on F19 with this patch and it works fine.
> 
> Thanks!
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Great, thanks for both of you testing this.

Bjorn, any objection to take this through your tree for 3.14-rc1?

thanks,

gre k-h

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Bjorn Helgaas]

On Fri, Nov 29, 2013 at 10:53 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Nov 29, 2013 at 10:41:37AM +0100, Veaceslav Falico wrote:
>> On Wed, Nov 27, 2013 at 10:46:52AM -0800, Greg Kroah-Hartman wrote:
>> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >
>> >The PCI MSI sysfs code is a mess with kobjects for things that don't
>> >really need to be kobjects.  This patch creates attributes dynamically
>> >for the MSI interrupts instead of using kobjects.
>> >
>> >Note, this removes a directory from the current MSI interrupt sysfs
>> >code:
>> >
>> >old MSI kobjects:
>> >pci_device
>> >   └── msi_irqs
>> >       └── 40
>> >           └── mode
>> >
>> >new MSI attributes:
>> >pci_device
>> >   └── msi_irqs
>> >       └── 40
>> >
>> >As there was only one file "mode" with the kobject model, the interrupt
>> >number is now a file that returns the "mode" of the interrupt (msi vs.
>> >msix).
>> >
>> >Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> Works like a charm for me.
>>
>> FWIW,
>>
>> Reviewed-by: Veaceslav Falico <vfalico@redhat.com>
>
> On Thu, Nov 28, 2013 at 10:14:19PM -0500, Neil Horman wrote:
>> ACK, Testing with git-head irqbalance on F19 with this patch and it works fine.
>>
>> Thanks!
>> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>
> Great, thanks for both of you testing this.
>
> Bjorn, any objection to take this through your tree for 3.14-rc1?

Nope, I don't object at all.  We probably should update
Documentation/ABI/testing/sysfs-bus-pci at the same time, though.

Bjorn

[PATCH] PCI: update MSI attribute file documentation

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Now that the MSI modes are now attribute files in sysfs, not
subdirectories, update the documentation to reflect this.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 5210a51c90fd..a3c5a6685036 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -70,18 +70,15 @@ Date:		September, 2011
 Contact:	Neil Horman <nhorman@tuxdriver.com>
 Description:
 		The /sys/devices/.../msi_irqs directory contains a variable set
-		of sub-directories, with each sub-directory being named after a
-		corresponding msi irq vector allocated to that device.  Each
-		numbered sub-directory N contains attributes of that irq.
-		Note that this directory is not created for device drivers which
-		do not support msi irqs
+		of files, with each file being named after a corresponding msi
+		irq vector allocated to that device.
 
-What:		/sys/bus/pci/devices/.../msi_irqs/<N>/mode
+What:		/sys/bus/pci/devices/.../msi_irqs/<N>
 Date:		September 2011
 Contact:	Neil Horman <nhorman@tuxdriver.com>
 Description:
 		This attribute indicates the mode that the irq vector named by
-		the parent directory is in (msi vs. msix)
+		the file is in (msi vs. msix)
 
 What:		/sys/bus/pci/devices/.../remove
 Date:		January 2009

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Greg KH]

On Mon, Dec 02, 2013 at 06:24:06PM -0700, Bjorn Helgaas wrote:
> On Fri, Nov 29, 2013 at 10:53 AM, Greg KH <greg@kroah.com> wrote:
> > On Fri, Nov 29, 2013 at 10:41:37AM +0100, Veaceslav Falico wrote:
> >> On Wed, Nov 27, 2013 at 10:46:52AM -0800, Greg Kroah-Hartman wrote:
> >> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >
> >> >The PCI MSI sysfs code is a mess with kobjects for things that don't
> >> >really need to be kobjects.  This patch creates attributes dynamically
> >> >for the MSI interrupts instead of using kobjects.
> >> >
> >> >Note, this removes a directory from the current MSI interrupt sysfs
> >> >code:
> >> >
> >> >old MSI kobjects:
> >> >pci_device
> >> >   └── msi_irqs
> >> >       └── 40
> >> >           └── mode
> >> >
> >> >new MSI attributes:
> >> >pci_device
> >> >   └── msi_irqs
> >> >       └── 40
> >> >
> >> >As there was only one file "mode" with the kobject model, the interrupt
> >> >number is now a file that returns the "mode" of the interrupt (msi vs.
> >> >msix).
> >> >
> >> >Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>
> >> Works like a charm for me.
> >>
> >> FWIW,
> >>
> >> Reviewed-by: Veaceslav Falico <vfalico@redhat.com>
> >
> > On Thu, Nov 28, 2013 at 10:14:19PM -0500, Neil Horman wrote:
> >> ACK, Testing with git-head irqbalance on F19 with this patch and it works fine.
> >>
> >> Thanks!
> >> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> > Great, thanks for both of you testing this.
> >
> > Bjorn, any objection to take this through your tree for 3.14-rc1?
> 
> Nope, I don't object at all.  We probably should update
> Documentation/ABI/testing/sysfs-bus-pci at the same time, though.

Now sent.

thanks,

greg k-h

Re: [PATCH] PCI: update MSI attribute file documentation

[Bjorn Helgaas]

On Mon, Dec 2, 2013 at 8:37 PM, Greg KH <greg@kroah.com> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Now that the MSI modes are now attribute files in sysfs, not
> subdirectories, update the documentation to reflect this.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks, I'll fold this into the patch that makes the code change, if
you don't mind.

Bjorn

> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 5210a51c90fd..a3c5a6685036 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -70,18 +70,15 @@ Date:               September, 2011
>  Contact:       Neil Horman <nhorman@tuxdriver.com>
>  Description:
>                 The /sys/devices/.../msi_irqs directory contains a variable set
> -               of sub-directories, with each sub-directory being named after a
> -               corresponding msi irq vector allocated to that device.  Each
> -               numbered sub-directory N contains attributes of that irq.
> -               Note that this directory is not created for device drivers which
> -               do not support msi irqs
> +               of files, with each file being named after a corresponding msi
> +               irq vector allocated to that device.
>
> -What:          /sys/bus/pci/devices/.../msi_irqs/<N>/mode
> +What:          /sys/bus/pci/devices/.../msi_irqs/<N>
>  Date:          September 2011
>  Contact:       Neil Horman <nhorman@tuxdriver.com>
>  Description:
>                 This attribute indicates the mode that the irq vector named by
> -               the parent directory is in (msi vs. msix)
> +               the file is in (msi vs. msix)
>
>  What:          /sys/bus/pci/devices/.../remove
>  Date:          January 2009

Re: [PATCH] PCI: update MSI attribute file documentation

[Greg KH]

On Tue, Dec 03, 2013 at 11:23:51AM -0700, Bjorn Helgaas wrote:
> On Mon, Dec 2, 2013 at 8:37 PM, Greg KH <greg@kroah.com> wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Now that the MSI modes are now attribute files in sysfs, not
> > subdirectories, update the documentation to reflect this.
> >
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks, I'll fold this into the patch that makes the code change, if
> you don't mind.

I don't mind at all, thanks.

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Bjorn Helgaas]

On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The PCI MSI sysfs code is a mess with kobjects for things that don't
> really need to be kobjects.  This patch creates attributes dynamically
> for the MSI interrupts instead of using kobjects.
>
> Note, this removes a directory from the current MSI interrupt sysfs
> code:
>
> old MSI kobjects:
> pci_device
>    └── msi_irqs
>        └── 40
>            └── mode
>
> new MSI attributes:
> pci_device
>    └── msi_irqs
>        └── 40
>
> As there was only one file "mode" with the kobject model, the interrupt
> number is now a file that returns the "mode" of the interrupt (msi vs.
> msix).
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I added the acks from Neil and Veaceslav, folded in the
Documentation/ABI update, and applied the whole thing to my pci/misc
branch for v3.14.  Thanks!

Bjorn

> ---
>
> Neil, can you test this patch to see if irqbalance works as-is or not?
>
> thanks,
>
> greg k-h
>
>  drivers/pci/msi.c   |  152 +++++++++++++++++++++++++---------------------------
>  include/linux/pci.h |    2
>  2 files changed, 76 insertions(+), 78 deletions(-)
>
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -363,6 +363,9 @@ void write_msi_msg(unsigned int irq, str
>  static void free_msi_irqs(struct pci_dev *dev)
>  {
>         struct msi_desc *entry, *tmp;
> +       struct attribute **msi_attrs;
> +       struct device_attribute *dev_attr;
> +       int count = 0;
>
>         list_for_each_entry(entry, &dev->msi_list, list) {
>                 int i, nvec;
> @@ -398,6 +401,22 @@ static void free_msi_irqs(struct pci_dev
>                 list_del(&entry->list);
>                 kfree(entry);
>         }
> +
> +       if (dev->msi_irq_groups) {
> +               sysfs_remove_groups(&dev->dev.kobj, dev->msi_irq_groups);
> +               msi_attrs = dev->msi_irq_groups[0]->attrs;
> +               list_for_each_entry(entry, &dev->msi_list, list) {
> +                       dev_attr = container_of(msi_attrs[count],
> +                                               struct device_attribute, attr);
> +                       kfree(dev_attr->attr.name);
> +                       kfree(dev_attr);
> +                       ++count;
> +               }
> +               kfree(msi_attrs);
> +               kfree(dev->msi_irq_groups[0]);
> +               kfree(dev->msi_irq_groups);
> +               dev->msi_irq_groups = NULL;
> +       }
>  }
>
>  static struct msi_desc *alloc_msi_entry(struct pci_dev *dev)
> @@ -471,96 +490,79 @@ void pci_restore_msi_state(struct pci_de
>  }
>  EXPORT_SYMBOL_GPL(pci_restore_msi_state);
>
> -
> -#define to_msi_attr(obj) container_of(obj, struct msi_attribute, attr)
> -#define to_msi_desc(obj) container_of(obj, struct msi_desc, kobj)
> -
> -struct msi_attribute {
> -       struct attribute        attr;
> -       ssize_t (*show)(struct msi_desc *entry, struct msi_attribute *attr,
> -                       char *buf);
> -       ssize_t (*store)(struct msi_desc *entry, struct msi_attribute *attr,
> -                        const char *buf, size_t count);
> -};
> -
> -static ssize_t show_msi_mode(struct msi_desc *entry, struct msi_attribute *atr,
> +static ssize_t msi_mode_show(struct device *dev, struct device_attribute *attr,
>                              char *buf)
>  {
> -       return sprintf(buf, "%s\n", entry->msi_attrib.is_msix ? "msix" : "msi");
> -}
> -
> -static ssize_t msi_irq_attr_show(struct kobject *kobj,
> -                                struct attribute *attr, char *buf)
> -{
> -       struct msi_attribute *attribute = to_msi_attr(attr);
> -       struct msi_desc *entry = to_msi_desc(kobj);
> -
> -       if (!attribute->show)
> -               return -EIO;
> -
> -       return attribute->show(entry, attribute, buf);
> -}
> -
> -static const struct sysfs_ops msi_irq_sysfs_ops = {
> -       .show = msi_irq_attr_show,
> -};
> -
> -static struct msi_attribute mode_attribute =
> -       __ATTR(mode, S_IRUGO, show_msi_mode, NULL);
> -
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       struct msi_desc *entry;
> +       unsigned long irq;
> +       int retval;
>
> -static struct attribute *msi_irq_default_attrs[] = {
> -       &mode_attribute.attr,
> -       NULL
> -};
> +       retval = kstrtoul(attr->attr.name, 10, &irq);
> +       if (retval)
> +               return retval;
>
> -static void msi_kobj_release(struct kobject *kobj)
> -{
> -       struct msi_desc *entry = to_msi_desc(kobj);
> -
> -       pci_dev_put(entry->dev);
> +       list_for_each_entry(entry, &pdev->msi_list, list) {
> +               if (entry->irq == irq) {
> +                       return sprintf(buf, "%s\n",
> +                                      entry->msi_attrib.is_msix ? "msix" : "msi");
> +               }
> +       }
> +       return -ENODEV;
>  }
>
> -static struct kobj_type msi_irq_ktype = {
> -       .release = msi_kobj_release,
> -       .sysfs_ops = &msi_irq_sysfs_ops,
> -       .default_attrs = msi_irq_default_attrs,
> -};
> -
>  static int populate_msi_sysfs(struct pci_dev *pdev)
>  {
> +       struct attribute **msi_attrs;
> +       struct device_attribute *msi_dev_attr;
> +       struct attribute_group *msi_irq_group;
> +       const struct attribute_group **msi_irq_groups;
>         struct msi_desc *entry;
> -       struct kobject *kobj;
>         int ret;
> +       int num_msi = 0;
>         int count = 0;
>
> -       pdev->msi_kset = kset_create_and_add("msi_irqs", NULL, &pdev->dev.kobj);
> -       if (!pdev->msi_kset)
> -               return -ENOMEM;
> -
> +       /* Determine how many msi entries we have */
>         list_for_each_entry(entry, &pdev->msi_list, list) {
> -               kobj = &entry->kobj;
> -               kobj->kset = pdev->msi_kset;
> -               pci_dev_get(pdev);
> -               ret = kobject_init_and_add(kobj, &msi_irq_ktype, NULL,
> -                                    "%u", entry->irq);
> -               if (ret)
> -                       goto out_unroll;
> -
> -               count++;
> +               ++num_msi;
>         }
> +       if (!num_msi)
> +               return 0;
>
> -       return 0;
> -
> -out_unroll:
> +       /* Dynamically create the MSI attributes for the PCI device */
> +       msi_attrs = kzalloc(sizeof(void *) * (num_msi + 1), GFP_KERNEL);
> +       if (!msi_attrs)
> +               return -ENOMEM;
>         list_for_each_entry(entry, &pdev->msi_list, list) {
> -               if (!count)
> -                       break;
> -               kobject_del(&entry->kobj);
> -               kobject_put(&entry->kobj);
> -               count--;
> +               char *name = kmalloc(20, GFP_KERNEL);
> +               msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
> +               if (!msi_dev_attr)
> +                       return -ENOMEM;
> +               sprintf(name, "%d", entry->irq);
> +               msi_dev_attr->attr.name = name;
> +               msi_dev_attr->attr.mode = S_IRUGO;
> +               msi_dev_attr->show = msi_mode_show;
> +               msi_attrs[count] = &msi_dev_attr->attr;
> +               ++count;
>         }
> -       return ret;
> +
> +       msi_irq_group = kzalloc(sizeof(*msi_irq_group), GFP_KERNEL);
> +       if (!msi_irq_group)
> +               return -ENOMEM;
> +       msi_irq_group->name = "msi_irqs";
> +       msi_irq_group->attrs = msi_attrs;
> +
> +       msi_irq_groups = kzalloc(sizeof(void *) * 2, GFP_KERNEL);
> +       if (!msi_irq_groups)
> +               return -ENOMEM;
> +       msi_irq_groups[0] = msi_irq_group;
> +
> +       ret = sysfs_create_groups(&pdev->dev.kobj, msi_irq_groups);
> +       if (ret)
> +               return ret;
> +       pdev->msi_irq_groups = msi_irq_groups;
> +
> +       return 0;
>  }
>
>  /**
> @@ -925,8 +927,6 @@ void pci_disable_msi(struct pci_dev *dev
>
>         pci_msi_shutdown(dev);
>         free_msi_irqs(dev);
> -       kset_unregister(dev->msi_kset);
> -       dev->msi_kset = NULL;
>  }
>  EXPORT_SYMBOL(pci_disable_msi);
>
> @@ -1023,8 +1023,6 @@ void pci_disable_msix(struct pci_dev *de
>
>         pci_msix_shutdown(dev);
>         free_msi_irqs(dev);
> -       kset_unregister(dev->msi_kset);
> -       dev->msi_kset = NULL;
>  }
>  EXPORT_SYMBOL(pci_disable_msix);
>
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -351,7 +351,7 @@ struct pci_dev {
>         struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
>  #ifdef CONFIG_PCI_MSI
>         struct list_head msi_list;
> -       struct kset *msi_kset;
> +       const struct attribute_group **msi_irq_groups;
>  #endif
>         struct pci_vpd *vpd;
>  #ifdef CONFIG_PCI_ATS

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

On Sat, Dec 7, 2013 at 12:41 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> The PCI MSI sysfs code is a mess with kobjects for things that don't
>> really need to be kobjects.  This patch creates attributes dynamically
>> for the MSI interrupts instead of using kobjects.
>>
>> Note, this removes a directory from the current MSI interrupt sysfs
>> code:
>>
>> old MSI kobjects:
>> pci_device
>>    └── msi_irqs
>>        └── 40
>>            └── mode
>>
>> new MSI attributes:
>> pci_device
>>    └── msi_irqs
>>        └── 40
>>
>> As there was only one file "mode" with the kobject model, the interrupt
>> number is now a file that returns the "mode" of the interrupt (msi vs.
>> msix).
>>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> I added the acks from Neil and Veaceslav, folded in the
> Documentation/ABI update, and applied the whole thing to my pci/misc
> branch for v3.14.  Thanks!

got:
[   71.429735] BUG: key ffff887fcf082a58 not in .data!
[   71.429737] ------------[ cut here ]------------
[   71.429742] WARNING: CPU: 0 PID: 4 at kernel/locking/lockdep.c:2987 lockdep_i
nit_map+0x127/0x5b0()
[   71.429743] DEBUG_LOCKS_WARN_ON(1)
[   71.429744] Modules linked in:
[   71.429747] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G          I  3.13.0-rc3
-yh-01187-ge0e4e4e-dirty #45
[   71.429757] Workqueue: events work_for_cpu_fn
[   71.429792]  0000000000000009 ffff881fcf95da58 ffffffff82020475 ffff881fcf95d
aa0
[   71.429800]  ffff881fcf95da90 ffffffff81097c3d ffff887fcf082a58 ffff88dfcec1a
d28
[   71.429809]  0000000000000000 0000000000000000 ffff889fcf2eecd0 ffff881fcf95d
af0
[   71.429809] Call Trace:
[   71.429814]  [<ffffffff82020475>] dump_stack+0x45/0x56
[   71.429818]  [<ffffffff81097c3d>] warn_slowpath_common+0x7d/0xa0
[   71.429822]  [<ffffffff81097cac>] warn_slowpath_fmt+0x4c/0x50
[   71.429826]  [<ffffffff810e3d97>] lockdep_init_map+0x127/0x5b0
[   71.429835]  [<ffffffff81242fe8>] ? sysfs_new_dirent+0x98/0x140
[   71.429839]  [<ffffffff81242033>] sysfs_add_file_mode_ns+0x63/0xc0
[   71.429843]  [<ffffffff8124491d>] internal_create_group+0x18d/0x260
[   71.429853]  [<ffffffff81556345>] ? populate_msi_sysfs+0x185/0x1d0
[   71.429857]  [<ffffffff81244a52>] sysfs_create_groups+0x42/0xa0
[   71.429861]  [<ffffffff81556367>] populate_msi_sysfs+0x1a7/0x1d0
[   71.429865]  [<ffffffff81556b67>] pci_enable_msi_block+0x1f7/0x2a0
[   71.429870]  [<ffffffff81549795>] pcie_port_device_register+0x335/0x520
[   71.429874]  [<ffffffff81549f08>] pcie_portdrv_probe+0x68/0xa0
[   71.429883]  [<ffffffff8153cf75>] local_pci_probe+0x45/0xa0
[   71.429887]  [<ffffffff810b0af4>] work_for_cpu_fn+0x14/0x20
[   71.429891]  [<ffffffff810b431b>] process_one_work+0x28b/0x4a0
[   71.429895]  [<ffffffff810b4292>] ? process_one_work+0x202/0x4a0
[   71.429899]  [<ffffffff810b55db>] worker_thread+0x26b/0x3a0
[   71.429903]  [<ffffffff810e262d>] ? trace_hardirqs_on+0xd/0x10
[   71.429906]  [<ffffffff810b5370>] ? manage_workers.isra.17+0x340/0x340
[   71.429912]  [<ffffffff810bc031>] kthread+0x111/0x120
[   71.429919]  [<ffffffff810cddab>] ? local_clock+0x2b/0x40
[   71.429923]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
[   71.429927]  [<ffffffff8203842c>] ret_from_fork+0x7c/0xb0
[   71.429931]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
[   71.429933] ---[ end trace c511e3d74efea94e ]---

looks like Greg forgot adding attr init.

Can you fold attached patch into Greg's patch ?

Index: linux-2.6/drivers/pci/msi.c
===================================================================
--- linux-2.6.orig/drivers/pci/msi.c
+++ linux-2.6/drivers/pci/msi.c
@@ -547,6 +547,7 @@ static int populate_msi_sysfs(struct pci
                msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
                if (!msi_dev_attr)
                        return -ENOMEM;
+               sysfs_attr_init(&msi_dev_attr->attr);
                sprintf(name, "%d", entry->irq);
                msi_dev_attr->attr.name = name;
                msi_dev_attr->attr.mode = S_IRUGO;

[-- Attachment #2: fix_key_data.patch --]
[-- Type: text/x-patch, Size: 552 bytes --]

---
 drivers/pci/msi.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6/drivers/pci/msi.c
===================================================================
--- linux-2.6.orig/drivers/pci/msi.c
+++ linux-2.6/drivers/pci/msi.c
@@ -547,6 +547,7 @@ static int populate_msi_sysfs(struct pci
 		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
 		if (!msi_dev_attr)
 			return -ENOMEM;
+		sysfs_attr_init(&msi_dev_attr->attr);
 		sprintf(name, "%d", entry->irq);
 		msi_dev_attr->attr.name = name;
 		msi_dev_attr->attr.mode = S_IRUGO;

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Greg Kroah-Hartman]

On Thu, Dec 12, 2013 at 03:17:50PM -0800, Yinghai Lu wrote:
> On Sat, Dec 7, 2013 at 12:41 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>
> >> The PCI MSI sysfs code is a mess with kobjects for things that don't
> >> really need to be kobjects.  This patch creates attributes dynamically
> >> for the MSI interrupts instead of using kobjects.
> >>
> >> Note, this removes a directory from the current MSI interrupt sysfs
> >> code:
> >>
> >> old MSI kobjects:
> >> pci_device
> >>    └── msi_irqs
> >>        └── 40
> >>            └── mode
> >>
> >> new MSI attributes:
> >> pci_device
> >>    └── msi_irqs
> >>        └── 40
> >>
> >> As there was only one file "mode" with the kobject model, the interrupt
> >> number is now a file that returns the "mode" of the interrupt (msi vs.
> >> msix).
> >>
> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > I added the acks from Neil and Veaceslav, folded in the
> > Documentation/ABI update, and applied the whole thing to my pci/misc
> > branch for v3.14.  Thanks!
> 
> got:
> [   71.429735] BUG: key ffff887fcf082a58 not in .data!
> [   71.429737] ------------[ cut here ]------------
> [   71.429742] WARNING: CPU: 0 PID: 4 at kernel/locking/lockdep.c:2987 lockdep_i
> nit_map+0x127/0x5b0()
> [   71.429743] DEBUG_LOCKS_WARN_ON(1)
> [   71.429744] Modules linked in:
> [   71.429747] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G          I  3.13.0-rc3
> -yh-01187-ge0e4e4e-dirty #45
> [   71.429757] Workqueue: events work_for_cpu_fn
> [   71.429792]  0000000000000009 ffff881fcf95da58 ffffffff82020475 ffff881fcf95d
> aa0
> [   71.429800]  ffff881fcf95da90 ffffffff81097c3d ffff887fcf082a58 ffff88dfcec1a
> d28
> [   71.429809]  0000000000000000 0000000000000000 ffff889fcf2eecd0 ffff881fcf95d
> af0
> [   71.429809] Call Trace:
> [   71.429814]  [<ffffffff82020475>] dump_stack+0x45/0x56
> [   71.429818]  [<ffffffff81097c3d>] warn_slowpath_common+0x7d/0xa0
> [   71.429822]  [<ffffffff81097cac>] warn_slowpath_fmt+0x4c/0x50
> [   71.429826]  [<ffffffff810e3d97>] lockdep_init_map+0x127/0x5b0
> [   71.429835]  [<ffffffff81242fe8>] ? sysfs_new_dirent+0x98/0x140
> [   71.429839]  [<ffffffff81242033>] sysfs_add_file_mode_ns+0x63/0xc0
> [   71.429843]  [<ffffffff8124491d>] internal_create_group+0x18d/0x260
> [   71.429853]  [<ffffffff81556345>] ? populate_msi_sysfs+0x185/0x1d0
> [   71.429857]  [<ffffffff81244a52>] sysfs_create_groups+0x42/0xa0
> [   71.429861]  [<ffffffff81556367>] populate_msi_sysfs+0x1a7/0x1d0
> [   71.429865]  [<ffffffff81556b67>] pci_enable_msi_block+0x1f7/0x2a0
> [   71.429870]  [<ffffffff81549795>] pcie_port_device_register+0x335/0x520
> [   71.429874]  [<ffffffff81549f08>] pcie_portdrv_probe+0x68/0xa0
> [   71.429883]  [<ffffffff8153cf75>] local_pci_probe+0x45/0xa0
> [   71.429887]  [<ffffffff810b0af4>] work_for_cpu_fn+0x14/0x20
> [   71.429891]  [<ffffffff810b431b>] process_one_work+0x28b/0x4a0
> [   71.429895]  [<ffffffff810b4292>] ? process_one_work+0x202/0x4a0
> [   71.429899]  [<ffffffff810b55db>] worker_thread+0x26b/0x3a0
> [   71.429903]  [<ffffffff810e262d>] ? trace_hardirqs_on+0xd/0x10
> [   71.429906]  [<ffffffff810b5370>] ? manage_workers.isra.17+0x340/0x340
> [   71.429912]  [<ffffffff810bc031>] kthread+0x111/0x120
> [   71.429919]  [<ffffffff810cddab>] ? local_clock+0x2b/0x40
> [   71.429923]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> [   71.429927]  [<ffffffff8203842c>] ret_from_fork+0x7c/0xb0
> [   71.429931]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> [   71.429933] ---[ end trace c511e3d74efea94e ]---
> 
> looks like Greg forgot adding attr init.
> 
> Can you fold attached patch into Greg's patch ?
> 
> Index: linux-2.6/drivers/pci/msi.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/msi.c
> +++ linux-2.6/drivers/pci/msi.c
> @@ -547,6 +547,7 @@ static int populate_msi_sysfs(struct pci
>                 msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
>                 if (!msi_dev_attr)
>                         return -ENOMEM;
> +               sysfs_attr_init(&msi_dev_attr->attr);
>                 sprintf(name, "%d", entry->irq);
>                 msi_dev_attr->attr.name = name;
>                 msi_dev_attr->attr.mode = S_IRUGO;

> ---
>  drivers/pci/msi.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-2.6/drivers/pci/msi.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/msi.c
> +++ linux-2.6/drivers/pci/msi.c
> @@ -547,6 +547,7 @@ static int populate_msi_sysfs(struct pci
>  		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
>  		if (!msi_dev_attr)
>  			return -ENOMEM;
> +		sysfs_attr_init(&msi_dev_attr->attr);
>  		sprintf(name, "%d", entry->irq);
>  		msi_dev_attr->attr.name = name;
>  		msi_dev_attr->attr.mode = S_IRUGO;

Ah, crap, yes, you are right, my fault, very nice catch.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

greg k-h

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 4:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Sat, Dec 7, 2013 at 12:41 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> The PCI MSI sysfs code is a mess with kobjects for things that don't
>>> really need to be kobjects.  This patch creates attributes dynamically
>>> for the MSI interrupts instead of using kobjects.
>>>
>>> Note, this removes a directory from the current MSI interrupt sysfs
>>> code:
>>>
>>> old MSI kobjects:
>>> pci_device
>>>    └── msi_irqs
>>>        └── 40
>>>            └── mode
>>>
>>> new MSI attributes:
>>> pci_device
>>>    └── msi_irqs
>>>        └── 40
>>>
>>> As there was only one file "mode" with the kobject model, the interrupt
>>> number is now a file that returns the "mode" of the interrupt (msi vs.
>>> msix).
>>>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> I added the acks from Neil and Veaceslav, folded in the
>> Documentation/ABI update, and applied the whole thing to my pci/misc
>> branch for v3.14.  Thanks!
>
> got:
> [   71.429735] BUG: key ffff887fcf082a58 not in .data!
> [   71.429737] ------------[ cut here ]------------
> [   71.429742] WARNING: CPU: 0 PID: 4 at kernel/locking/lockdep.c:2987 lockdep_i
> nit_map+0x127/0x5b0()
> [   71.429743] DEBUG_LOCKS_WARN_ON(1)
> [   71.429744] Modules linked in:
> [   71.429747] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G          I  3.13.0-rc3
> -yh-01187-ge0e4e4e-dirty #45
> [   71.429757] Workqueue: events work_for_cpu_fn
> [   71.429792]  0000000000000009 ffff881fcf95da58 ffffffff82020475 ffff881fcf95d
> aa0
> [   71.429800]  ffff881fcf95da90 ffffffff81097c3d ffff887fcf082a58 ffff88dfcec1a
> d28
> [   71.429809]  0000000000000000 0000000000000000 ffff889fcf2eecd0 ffff881fcf95d
> af0
> [   71.429809] Call Trace:
> [   71.429814]  [<ffffffff82020475>] dump_stack+0x45/0x56
> [   71.429818]  [<ffffffff81097c3d>] warn_slowpath_common+0x7d/0xa0
> [   71.429822]  [<ffffffff81097cac>] warn_slowpath_fmt+0x4c/0x50
> [   71.429826]  [<ffffffff810e3d97>] lockdep_init_map+0x127/0x5b0
> [   71.429835]  [<ffffffff81242fe8>] ? sysfs_new_dirent+0x98/0x140
> [   71.429839]  [<ffffffff81242033>] sysfs_add_file_mode_ns+0x63/0xc0
> [   71.429843]  [<ffffffff8124491d>] internal_create_group+0x18d/0x260
> [   71.429853]  [<ffffffff81556345>] ? populate_msi_sysfs+0x185/0x1d0
> [   71.429857]  [<ffffffff81244a52>] sysfs_create_groups+0x42/0xa0
> [   71.429861]  [<ffffffff81556367>] populate_msi_sysfs+0x1a7/0x1d0
> [   71.429865]  [<ffffffff81556b67>] pci_enable_msi_block+0x1f7/0x2a0
> [   71.429870]  [<ffffffff81549795>] pcie_port_device_register+0x335/0x520
> [   71.429874]  [<ffffffff81549f08>] pcie_portdrv_probe+0x68/0xa0
> [   71.429883]  [<ffffffff8153cf75>] local_pci_probe+0x45/0xa0
> [   71.429887]  [<ffffffff810b0af4>] work_for_cpu_fn+0x14/0x20
> [   71.429891]  [<ffffffff810b431b>] process_one_work+0x28b/0x4a0
> [   71.429895]  [<ffffffff810b4292>] ? process_one_work+0x202/0x4a0
> [   71.429899]  [<ffffffff810b55db>] worker_thread+0x26b/0x3a0
> [   71.429903]  [<ffffffff810e262d>] ? trace_hardirqs_on+0xd/0x10
> [   71.429906]  [<ffffffff810b5370>] ? manage_workers.isra.17+0x340/0x340
> [   71.429912]  [<ffffffff810bc031>] kthread+0x111/0x120
> [   71.429919]  [<ffffffff810cddab>] ? local_clock+0x2b/0x40
> [   71.429923]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> [   71.429927]  [<ffffffff8203842c>] ret_from_fork+0x7c/0xb0
> [   71.429931]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> [   71.429933] ---[ end trace c511e3d74efea94e ]---
>
> looks like Greg forgot adding attr init.
>
> Can you fold attached patch into Greg's patch ?

Sure.  I don't know how to reproduce it, so I couldn't verify the fix,
but I added it to Greg's patch.  Thanks!

> Index: linux-2.6/drivers/pci/msi.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/msi.c
> +++ linux-2.6/drivers/pci/msi.c
> @@ -547,6 +547,7 @@ static int populate_msi_sysfs(struct pci
>                 msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
>                 if (!msi_dev_attr)
>                         return -ENOMEM;
> +               sysfs_attr_init(&msi_dev_attr->attr);
>                 sprintf(name, "%d", entry->irq);
>                 msi_dev_attr->attr.name = name;
>                 msi_dev_attr->attr.mode = S_IRUGO;

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Greg Kroah-Hartman]

On Thu, Dec 12, 2013 at 04:56:20PM -0700, Bjorn Helgaas wrote:
> On Thu, Dec 12, 2013 at 4:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Sat, Dec 7, 2013 at 12:41 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>
> >>> The PCI MSI sysfs code is a mess with kobjects for things that don't
> >>> really need to be kobjects.  This patch creates attributes dynamically
> >>> for the MSI interrupts instead of using kobjects.
> >>>
> >>> Note, this removes a directory from the current MSI interrupt sysfs
> >>> code:
> >>>
> >>> old MSI kobjects:
> >>> pci_device
> >>>    └── msi_irqs
> >>>        └── 40
> >>>            └── mode
> >>>
> >>> new MSI attributes:
> >>> pci_device
> >>>    └── msi_irqs
> >>>        └── 40
> >>>
> >>> As there was only one file "mode" with the kobject model, the interrupt
> >>> number is now a file that returns the "mode" of the interrupt (msi vs.
> >>> msix).
> >>>
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>
> >> I added the acks from Neil and Veaceslav, folded in the
> >> Documentation/ABI update, and applied the whole thing to my pci/misc
> >> branch for v3.14.  Thanks!
> >
> > got:
> > [   71.429735] BUG: key ffff887fcf082a58 not in .data!
> > [   71.429737] ------------[ cut here ]------------
> > [   71.429742] WARNING: CPU: 0 PID: 4 at kernel/locking/lockdep.c:2987 lockdep_i
> > nit_map+0x127/0x5b0()
> > [   71.429743] DEBUG_LOCKS_WARN_ON(1)
> > [   71.429744] Modules linked in:
> > [   71.429747] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G          I  3.13.0-rc3
> > -yh-01187-ge0e4e4e-dirty #45
> > [   71.429757] Workqueue: events work_for_cpu_fn
> > [   71.429792]  0000000000000009 ffff881fcf95da58 ffffffff82020475 ffff881fcf95d
> > aa0
> > [   71.429800]  ffff881fcf95da90 ffffffff81097c3d ffff887fcf082a58 ffff88dfcec1a
> > d28
> > [   71.429809]  0000000000000000 0000000000000000 ffff889fcf2eecd0 ffff881fcf95d
> > af0
> > [   71.429809] Call Trace:
> > [   71.429814]  [<ffffffff82020475>] dump_stack+0x45/0x56
> > [   71.429818]  [<ffffffff81097c3d>] warn_slowpath_common+0x7d/0xa0
> > [   71.429822]  [<ffffffff81097cac>] warn_slowpath_fmt+0x4c/0x50
> > [   71.429826]  [<ffffffff810e3d97>] lockdep_init_map+0x127/0x5b0
> > [   71.429835]  [<ffffffff81242fe8>] ? sysfs_new_dirent+0x98/0x140
> > [   71.429839]  [<ffffffff81242033>] sysfs_add_file_mode_ns+0x63/0xc0
> > [   71.429843]  [<ffffffff8124491d>] internal_create_group+0x18d/0x260
> > [   71.429853]  [<ffffffff81556345>] ? populate_msi_sysfs+0x185/0x1d0
> > [   71.429857]  [<ffffffff81244a52>] sysfs_create_groups+0x42/0xa0
> > [   71.429861]  [<ffffffff81556367>] populate_msi_sysfs+0x1a7/0x1d0
> > [   71.429865]  [<ffffffff81556b67>] pci_enable_msi_block+0x1f7/0x2a0
> > [   71.429870]  [<ffffffff81549795>] pcie_port_device_register+0x335/0x520
> > [   71.429874]  [<ffffffff81549f08>] pcie_portdrv_probe+0x68/0xa0
> > [   71.429883]  [<ffffffff8153cf75>] local_pci_probe+0x45/0xa0
> > [   71.429887]  [<ffffffff810b0af4>] work_for_cpu_fn+0x14/0x20
> > [   71.429891]  [<ffffffff810b431b>] process_one_work+0x28b/0x4a0
> > [   71.429895]  [<ffffffff810b4292>] ? process_one_work+0x202/0x4a0
> > [   71.429899]  [<ffffffff810b55db>] worker_thread+0x26b/0x3a0
> > [   71.429903]  [<ffffffff810e262d>] ? trace_hardirqs_on+0xd/0x10
> > [   71.429906]  [<ffffffff810b5370>] ? manage_workers.isra.17+0x340/0x340
> > [   71.429912]  [<ffffffff810bc031>] kthread+0x111/0x120
> > [   71.429919]  [<ffffffff810cddab>] ? local_clock+0x2b/0x40
> > [   71.429923]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> > [   71.429927]  [<ffffffff8203842c>] ret_from_fork+0x7c/0xb0
> > [   71.429931]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
> > [   71.429933] ---[ end trace c511e3d74efea94e ]---
> >
> > looks like Greg forgot adding attr init.
> >
> > Can you fold attached patch into Greg's patch ?
> 
> Sure.  I don't know how to reproduce it, so I couldn't verify the fix,
> but I added it to Greg's patch.  Thanks!

I think lockdep has to be enabled for this to show up, I wasn't running
with that enabled when I tested the code, my fault, sorry.

greg k-h

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 7:06 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Dec 12, 2013 at 04:56:20PM -0700, Bjorn Helgaas wrote:
>> On Thu, Dec 12, 2013 at 4:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Sat, Dec 7, 2013 at 12:41 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> On Wed, Nov 27, 2013 at 11:46 AM, Greg Kroah-Hartman
>> >> <gregkh@linuxfoundation.org> wrote:
>> >>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >>>
>> >>> The PCI MSI sysfs code is a mess with kobjects for things that don't
>> >>> really need to be kobjects.  This patch creates attributes dynamically
>> >>> for the MSI interrupts instead of using kobjects.
>> >>>
>> >>> Note, this removes a directory from the current MSI interrupt sysfs
>> >>> code:
>> >>>
>> >>> old MSI kobjects:
>> >>> pci_device
>> >>>    └── msi_irqs
>> >>>        └── 40
>> >>>            └── mode
>> >>>
>> >>> new MSI attributes:
>> >>> pci_device
>> >>>    └── msi_irqs
>> >>>        └── 40
>> >>>
>> >>> As there was only one file "mode" with the kobject model, the interrupt
>> >>> number is now a file that returns the "mode" of the interrupt (msi vs.
>> >>> msix).
>> >>>
>> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >>
>> >> I added the acks from Neil and Veaceslav, folded in the
>> >> Documentation/ABI update, and applied the whole thing to my pci/misc
>> >> branch for v3.14.  Thanks!
>> >
>> > got:
>> > [   71.429735] BUG: key ffff887fcf082a58 not in .data!
>> > [   71.429737] ------------[ cut here ]------------
>> > [   71.429742] WARNING: CPU: 0 PID: 4 at kernel/locking/lockdep.c:2987 lockdep_i
>> > nit_map+0x127/0x5b0()
>> > [   71.429743] DEBUG_LOCKS_WARN_ON(1)
>> > [   71.429744] Modules linked in:
>> > [   71.429747] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G          I  3.13.0-rc3
>> > -yh-01187-ge0e4e4e-dirty #45
>> > [   71.429757] Workqueue: events work_for_cpu_fn
>> > [   71.429792]  0000000000000009 ffff881fcf95da58 ffffffff82020475 ffff881fcf95d
>> > aa0
>> > [   71.429800]  ffff881fcf95da90 ffffffff81097c3d ffff887fcf082a58 ffff88dfcec1a
>> > d28
>> > [   71.429809]  0000000000000000 0000000000000000 ffff889fcf2eecd0 ffff881fcf95d
>> > af0
>> > [   71.429809] Call Trace:
>> > [   71.429814]  [<ffffffff82020475>] dump_stack+0x45/0x56
>> > [   71.429818]  [<ffffffff81097c3d>] warn_slowpath_common+0x7d/0xa0
>> > [   71.429822]  [<ffffffff81097cac>] warn_slowpath_fmt+0x4c/0x50
>> > [   71.429826]  [<ffffffff810e3d97>] lockdep_init_map+0x127/0x5b0
>> > [   71.429835]  [<ffffffff81242fe8>] ? sysfs_new_dirent+0x98/0x140
>> > [   71.429839]  [<ffffffff81242033>] sysfs_add_file_mode_ns+0x63/0xc0
>> > [   71.429843]  [<ffffffff8124491d>] internal_create_group+0x18d/0x260
>> > [   71.429853]  [<ffffffff81556345>] ? populate_msi_sysfs+0x185/0x1d0
>> > [   71.429857]  [<ffffffff81244a52>] sysfs_create_groups+0x42/0xa0
>> > [   71.429861]  [<ffffffff81556367>] populate_msi_sysfs+0x1a7/0x1d0
>> > [   71.429865]  [<ffffffff81556b67>] pci_enable_msi_block+0x1f7/0x2a0
>> > [   71.429870]  [<ffffffff81549795>] pcie_port_device_register+0x335/0x520
>> > [   71.429874]  [<ffffffff81549f08>] pcie_portdrv_probe+0x68/0xa0
>> > [   71.429883]  [<ffffffff8153cf75>] local_pci_probe+0x45/0xa0
>> > [   71.429887]  [<ffffffff810b0af4>] work_for_cpu_fn+0x14/0x20
>> > [   71.429891]  [<ffffffff810b431b>] process_one_work+0x28b/0x4a0
>> > [   71.429895]  [<ffffffff810b4292>] ? process_one_work+0x202/0x4a0
>> > [   71.429899]  [<ffffffff810b55db>] worker_thread+0x26b/0x3a0
>> > [   71.429903]  [<ffffffff810e262d>] ? trace_hardirqs_on+0xd/0x10
>> > [   71.429906]  [<ffffffff810b5370>] ? manage_workers.isra.17+0x340/0x340
>> > [   71.429912]  [<ffffffff810bc031>] kthread+0x111/0x120
>> > [   71.429919]  [<ffffffff810cddab>] ? local_clock+0x2b/0x40
>> > [   71.429923]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
>> > [   71.429927]  [<ffffffff8203842c>] ret_from_fork+0x7c/0xb0
>> > [   71.429931]  [<ffffffff810bbf20>] ? kthread_stop+0xf0/0xf0
>> > [   71.429933] ---[ end trace c511e3d74efea94e ]---
>> >
>> > looks like Greg forgot adding attr init.
>> >
>> > Can you fold attached patch into Greg's patch ?
>>
>> Sure.  I don't know how to reproduce it, so I couldn't verify the fix,
>> but I added it to Greg's patch.  Thanks!
>
> I think lockdep has to be enabled for this to show up, I wasn't running
> with that enabled when I tested the code, my fault, sorry.

No problem.  I have

CONFIG_LOCKDEP_SUPPORT=y
CONFIG_LOCKDEP=y
CONFIG_DEBUG_LOCKDEP=y

and I still don't see the problem booting the qemu q35 model.
Whatever, I folded it in anyway.

Bjorn

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Yinghai Lu]

On Thu, Dec 12, 2013 at 8:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> No problem.  I have
>
> CONFIG_LOCKDEP_SUPPORT=y
> CONFIG_LOCKDEP=y
> CONFIG_DEBUG_LOCKDEP=y
>
> and I still don't see the problem booting the qemu q35 model.
> Whatever, I folded it in anyway.

Does qemu q35 use MSI?

Yinghai

Re: [PATCH] PCI: export MSI mode using attributes, not kobjects

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 10:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Dec 12, 2013 at 8:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> No problem.  I have
>>
>> CONFIG_LOCKDEP_SUPPORT=y
>> CONFIG_LOCKDEP=y
>> CONFIG_DEBUG_LOCKDEP=y
>>
>> and I still don't see the problem booting the qemu q35 model.
>> Whatever, I folded it in anyway.
>
> Does qemu q35 use MSI?

Oops, sorry, I *do* see the problem on qemu q35.  I must have messed
something up when I tried it yesterday.  And I verified that your
change fixes it.  Sorry for the noise.

Bjorn