Re: [PATCH 3/3] Expose MSI-X interrupts through a dynamically generated sysfs directory

[AmÃrico Wang]

On Tue, Oct 20, 2009 at 1:50 PM, Matthew Wilcox <matthew@xxxxxx> wrote:
>
> From: Matthew Wilcox <willy@xxxxxxxxxxxxxxx>
> Date: Mon, 19 Oct 2009 01:35:41 -0400
> Subject: [PATCH 3/3] Expose MSI-X interrupts through a dynamically generated sysfs directory
>
> Introduce the ability to dynamically generate the attributes (which are
> then added to sysfs). ÂAdd a user in the form of the PCI MSI code, which
> was why I started on this in the first place.
> ---
> Âdrivers/pci/msi.c   |  77 ++++++++++++++++++++++++++++++++++++++++++++++--
> Âfs/sysfs/group.c   Â|  Â7 ++++-
> Âinclude/linux/pci.h  |  Â2 +
> Âinclude/linux/sysfs.h | Â Â9 ++++++
> Â4 files changed, 90 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f9cf317..e0971e6 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -275,6 +275,9 @@ static void free_msi_irqs(struct pci_dev *dev)
> Â{
> Â Â Â Âstruct msi_desc *entry, *tmp;
>
> + Â Â Â if (dev->msix_dir.name)
> + Â Â Â Â Â Â Â sysfs_remove_group(&dev->dev.kobj, &dev->msix_dir);
> +
> Â Â Â Âlist_for_each_entry(entry, &dev->msi_list, list) {
> Â Â Â Â Â Â Â Âint i, nvec;
> Â Â Â Â Â Â Â Âif (!entry->irq)
> @@ -447,13 +450,12 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned pos,
> Â}
>
> Âstatic int msix_setup_entries(struct pci_dev *dev, unsigned pos,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void __iomem *base, struct msix_entry *entries,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int nvec)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void __iomem *base, struct msix_entry *entries)
> Â{
> Â Â Â Âstruct msi_desc *entry;
> Â Â Â Âint i;
>
> - Â Â Â for (i = 0; i < nvec; i++) {
> + Â Â Â for (i = 0; i < dev->nr_irqs; i++) {
> Â Â Â Â Â Â Â Âentry = alloc_msi_entry(dev);
> Â Â Â Â Â Â Â Âif (!entry) {
> Â Â Â Â Â Â Â Â Â Â Â Âif (!i)
> @@ -495,6 +497,64 @@ static void msix_program_entries(struct pci_dev *dev,
> Â Â Â Â}
> Â}
>
> +struct pci_msix_attribute {
> + Â Â Â struct device_attribute da;
> + Â Â Â unsigned irq;
> + Â Â Â char name[8]; Â /* current max is 5: "2047\0" */
> +};
> +
> +static ssize_t
> +pci_msix_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + Â Â Â struct pci_msix_attribute *msix_attr;
> + Â Â Â msix_attr = container_of(attr, struct pci_msix_attribute, da);
> + Â Â Â return snprintf(buf, PAGE_SIZE, "%u\n", msix_attr->irq);
> +}
> +
> +#define kobj_to_pci_dev(obj) to_pci_dev(container_of(obj, struct device, kobj))


You define this, but no one uses it?

> +
> +static int msix_populate(struct dentry *dentry, struct attribute_group *grp)
> +{
> + Â Â Â struct pci_dev *pdev = container_of(grp, struct pci_dev, msix_dir);
> + Â Â Â unsigned i, nr_irqs = pdev->nr_irqs;
> + Â Â Â struct pci_msix_attribute *attr;
> + Â Â Â struct attribute **array;
> + Â Â Â struct msi_desc *desc;
> +
> + Â Â Â array = kmalloc((nr_irqs + 1) * sizeof(void *), GFP_KERNEL);
> + Â Â Â if (!array)
> + Â Â Â Â Â Â Â return -ENOMEM;
> + Â Â Â attr = kmalloc(nr_irqs * sizeof(*attr), GFP_KERNEL);
> + Â Â Â if (!attr)
> + Â Â Â Â Â Â Â return -ENOMEM;

Here leaks memory allocated above.

> +
> + Â Â Â for (i = 0; i < nr_irqs; i++)
> + Â Â Â Â Â Â Â array[i] = &attr[i].da.attr;
> + Â Â Â array[i] = NULL;
> + Â Â Â grp->attrs = array;
> +
> + Â Â Â list_for_each_entry(desc, &pdev->msi_list, list) {
> + Â Â Â Â Â Â Â attr->irq = desc->irq;
> + Â Â Â Â Â Â Â snprintf(attr->name, sizeof(attr->name), "%u",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â desc->msi_attrib.entry_nr);
> + Â Â Â Â Â Â Â attr->da.attr.name = (const char *)&attr->name;
> + Â Â Â Â Â Â Â attr->da.attr.mode = 0444;
> + Â Â Â Â Â Â Â attr->da.show = pci_msix_show;
> + Â Â Â Â Â Â Â attr++;
> + Â Â Â }
> +
> + Â Â Â return sysfs_populate_group(dentry, grp);
> +}
> +
> +static void msix_depopulate(struct dentry *dentry, struct attribute_group *grp)
> +{
> + Â Â Â sysfs_depopulate_group(dentry, grp);
> +
> + Â Â Â kfree(grp->attrs[0]);
> + Â Â Â kfree(grp->attrs);
> + Â Â Â grp->attrs = NULL;
> +}
> +
> Â/**
> Â* msix_capability_init - configure device's MSI-X capability
> Â* @dev: pointer to the pci_dev data structure of MSI-X device function
> @@ -524,10 +584,19 @@ static int msix_capability_init(struct pci_dev *dev,
> Â Â Â Âif (!base)
> Â Â Â Â Â Â Â Âreturn -ENOMEM;
>
> - Â Â Â ret = msix_setup_entries(dev, pos, base, entries, nvec);
> + Â Â Â dev->nr_irqs = nvec;
> +
> + Â Â Â dev->msix_dir.name = "irqs";
> + Â Â Â dev->msix_dir.populate = msix_populate;
> + Â Â Â dev->msix_dir.depopulate = msix_depopulate;
> + Â Â Â ret = sysfs_create_group(&dev->dev.kobj, &dev->msix_dir);
> Â Â Â Âif (ret)
> Â Â Â Â Â Â Â Âreturn ret;
>
> + Â Â Â ret = msix_setup_entries(dev, pos, base, entries);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â goto error;
> +
> Â Â Â Âret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
> Â Â Â Âif (ret)
> Â Â Â Â Â Â Â Âgoto error;
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 37ac584..c97139f 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -102,6 +102,8 @@ static
> Âint group_populate(struct dentry *dentry, struct sysfs_dirent *sd)
> Â{
> Â Â Â Âstruct attribute_group *grp = sd->s_dir.data;
> + Â Â Â if (grp->populate)
> + Â Â Â Â Â Â Â return grp->populate(dentry, grp);
> Â Â Â Âreturn sysfs_populate_group(dentry, grp);
> Â}
>
> @@ -109,7 +111,10 @@ static
> Âvoid group_depopulate(struct dentry *dentry, struct sysfs_dirent *sd)
> Â{
> Â Â Â Âstruct attribute_group *grp = sd->s_dir.data;
> - Â Â Â sysfs_depopulate_group(dentry, grp);
> + Â Â Â if (grp->depopulate)
> + Â Â Â Â Â Â Â grp->depopulate(dentry, grp);
> + Â Â Â else
> + Â Â Â Â Â Â Â sysfs_depopulate_group(dentry, grp);
> Â}
>
> Âstatic void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f5c7cd3..47c7fc6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -291,6 +291,8 @@ 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 attribute_group msix_dir;
> + Â Â Â unsigned short nr_irqs;
> Â#endif
> Â Â Â Âstruct pci_vpd *vpd;
> Â#ifdef CONFIG_PCI_IOV
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9d68fed..cf9d200 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -19,6 +19,7 @@
>
> Âstruct kobject;
> Âstruct module;
> +struct dentry;
>
> Â/* FIXME
> Â* The *owner field is no longer used.
> @@ -36,6 +37,10 @@ struct attribute_group {
>    Âmode_t         Â(*is_visible)(struct kobject *,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct attribute *, int);
>    Âstruct attribute    Â**attrs;
> +    int           (*populate)(struct dentry *,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct attribute_group *);
> +    void          Â(*depopulate)(struct dentry *,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct attribute_group *);
> Â};
>
>
> @@ -115,6 +120,10 @@ int sysfs_update_group(struct kobject *kobj,
> Â Â Â Â Â Â Â Â Â Â Â const struct attribute_group *grp);
> Âvoid sysfs_remove_group(struct kobject *kobj,
> Â Â Â Â Â Â Â Â Â Â Â Âconst struct attribute_group *grp);
> +int sysfs_populate_group(struct dentry *dentry,
> + Â Â Â Â Â Â Â Â Â Â Â const struct attribute_group *grp);
> +void sysfs_depopulate_group(struct dentry *dentry,
> + Â Â Â Â Â Â Â Â Â Â Â const struct attribute_group *grp);
> Âint sysfs_add_file_to_group(struct kobject *kobj,
> Â Â Â Â Â Â Â Â Â Â Â Âconst struct attribute *attr, const char *group);
> Âvoid sysfs_remove_file_from_group(struct kobject *kobj,
> --
> 1.6.3.3
>
> --
> Matthew Wilcox             ÂIntel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. ÂWe can't possibly take such
> a retrograde step."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> Please read the FAQ at Âhttp://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/