On Thu, Nov 20, 2008 at 11:24:07AM +1100, Vincent Rizza wrote:
> I've made some more changes based on the feedback. I figured if the
> sysfs entries were removed before the msi_desc structs were deleted
> from msi_list then we wouldn't have to worry about synchronization.
> How does it look? What's the decision on the directory name?
>
>
> Thanks,
>
>
> -- Vinnie
> >From 89928bd9145228ed7691c593b4f0369dd12b3929 Mon Sep 17 00:00:00 2001
> From: Vincent Rizza <vinnie@xxxxxxx>
> Date: Thu, 13 Nov 2008 10:27:33 +1100
> Subject: [PATCH] Add sysfs entry that displays MSI-X IRQs
>
> Creates a sysfs entry for each MSI-X vector. The name of the entry is
> based on the entry_nr attribute in msi_desc.msi_attrib. The file
> contains the MSI-X vector number corresponding to that entry_nr value.
>
> Signed-off-by: Vincent Rizza <vinnie@xxxxxxx>
> Signed-off-by: Brett Grandbois <brettg@xxxxxxx>
> Signed-off-by: Ken Sandars <ksandars@xxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-bus-pci | 10 +++++
> drivers/pci/msi.c | 62 +++++++++++++++++++++++++++++++
> include/linux/msi.h | 4 ++
> include/linux/pci.h | 1 +
> 4 files changed, 77 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ceddcff..9941233 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -9,3 +9,13 @@ Description:
> that some devices may have malformatted data. If the
> underlying VPD has a writable section then the
> corresponding section of this file will be writable.
> +
> +What: /sys/bus/pci/devices/.../msix_irq/<IRQ vector number>
> +Date: November 2008
> +Contact: Vincent Rizza <vinnie@xxxxxxx>
> +Description:
> + If a pci device uses any MSI-X IRQs a new directory
> + is created called "msix_irq". The directory contains a
> + file for each MSI-X IRQ used. The filename is the number
> + stored in msi_desc.msi_attrib.entry_nr. The files contain
> + the MSI-X vector corresponding to their entry_nr number.
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 74801f7..c2c43df 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -25,6 +25,31 @@
>
> static int pci_msi_enable = 1;
>
> +static ssize_t
> +pci_msix_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + /* kobj = msix_dir. The parent kobj is needed to get pci_dev */
> + struct kobject *p_kobj = kobj->parent;
> + struct pci_dev *pdev = to_pci_dev(container_of(p_kobj, struct device,
> + kobj));
> + struct msi_desc *entry;
> + unsigned long msi_index;
> + int ret = 0;
> +
> + ret = strict_strtoul(attr->attr.name, 10, &msi_index);
> + if (!ret) {
> + list_for_each_entry(entry, &pdev->msi_list, list) {
> + if (entry->msi_attrib.entry_nr == msi_index) {
> + ret = snprintf(buf, PAGE_SIZE, "%d\n",
> + entry->irq);
> + break;
> + }
> + }
> + }
> +
> + return ret;
> +}
> +
> /* Arch hooks */
>
> int __attribute__ ((weak))
> @@ -488,10 +513,32 @@ static int msix_capability_init(struct pci_dev *dev,
> return avail;
> }
>
> + /* create directory "msix_irq" */
> + dev->msix_dir = kobject_create_and_add("msix_irq", &dev->dev.kobj);
> + if (!dev->msix_dir) {
> + msi_free_irqs(dev);
> + return -ENOMEM;
> + }
> +
> i = 0;
> list_for_each_entry(entry, &dev->msi_list, list) {
> entries[i].vector = entry->irq;
> set_irq_msi(entry->irq, entry);
> +
> + snprintf(entry->msix_fname, sizeof(entry->msix_fname), "%d",
> + entry->msi_attrib.entry_nr);
> + entry->sysfs_entry.attr.name = (const char *) entry->msix_fname;
> + entry->sysfs_entry.attr.mode = 0444;
> + entry->sysfs_entry.show = pci_msix_show;
> +
> + ret = sysfs_create_file(dev->msix_dir,
> + &entry->sysfs_entry.attr);
> + if (ret) {
> + /* Using name as a flag during clean-up */
> + entry->sysfs_entry.attr.name = NULL;
> + msi_free_irqs(dev);
> + return ret;
> + }
> i++;
> }
> /* Set MSI-X enabled bits */
> @@ -628,6 +675,14 @@ static int msi_free_irqs(struct pci_dev* dev)
> list_for_each_entry(entry, &dev->msi_list, list) {
> if (entry->irq)
> BUG_ON(irq_has_action(entry->irq));
> + /*
> + * Only remove if it's been created. If the attribute's name is
> + * set then this particular "entry" corresponds to a sysfs
> + * file.
> + */
> + if (entry->sysfs_entry.attr.name)
> + sysfs_remove_file(&dev->dev.kobj,
> + &entry->sysfs_entry.attr);
> }
>
> arch_teardown_msi_irqs(dev);
> @@ -645,6 +700,13 @@ static int msi_free_irqs(struct pci_dev* dev)
> kfree(entry);
> }
>
> + /* Also doesn't exist in MSI mode */
> + if (dev->msix_dir) {
> + sysfs_remove_dir(dev->msix_dir);
> + kobject_put(dev->msix_dir);
> + dev->msix_dir = NULL;
> + }
> +
> return 0;
> }
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 8f29392..242e068 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -33,6 +33,10 @@ struct msi_desc {
> void __iomem *mask_base;
> struct pci_dev *dev;
>
> + struct kobj_attribute sysfs_entry;
More thoughts about how this is done internally...
Why do you need an extra kobject at all? Why not just add this to the
attributes of the pci device like all other pci device attributes?
Also see the talk about the virtual pci devices, as to how to add these
attributes so they show up _before_ the uevent is sent to userspace.
That's the proper way these should be hooked up to the device.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
