Hi Greg, Thanks for reviewing this.
On 10/07/14 01:09, Greg Kroah-Hartman wrote:
On Wed, Jun 25, 2014 at 06:30:37PM +0100, Sudeep Holla wrote:+static const struct device_attribute *cache_optional_attrs[] = { + &dev_attr_coherency_line_size, + &dev_attr_ways_of_associativity, + &dev_attr_number_of_sets, + &dev_attr_size, + &dev_attr_attributes, + &dev_attr_physical_line_partition, + NULL +}; + +static int device_add_attrs(struct device *dev, + const struct device_attribute **dev_attrs) +{ + int i, error = 0; + struct device_attribute *dev_attr; + char *buf; + + if (!dev_attrs) + return 0; + + buf = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + for (i = 0; dev_attrs[i]; i++) { + dev_attr = (struct device_attribute *)dev_attrs[i]; + + /* create attributes that provides meaningful value */ + if (dev_attr->show(dev, dev_attr, buf) < 0) + continue; + + error = device_create_file(dev, dev_attrs[i]); + if (error) { + while (--i >= 0) + device_remove_file(dev, dev_attrs[i]); + break; + } + } + + kfree(buf); + return error; +}Ick, why create your own function for this when the driver core has this functionality built into it? Look at the is_visible() callback, and how it is use for an attribute group please.
I agree even I added this function hesitantly as didn't realize that I can use is_visible for this purpose. Thanks for pointing that out I will have a look at it.
+static void device_remove_attrs(struct device *dev, + const struct device_attribute **dev_attrs) +{ + int i; + + if (!dev_attrs) + return; + + for (i = 0; dev_attrs[i]; dev_attrs++, i++) + device_remove_file(dev, dev_attrs[i]); +}You should just remove a whole group at once, not individually.
Right, I must be able to get rid of these 2 functions once I use is_visible callback.
+ +const struct device_attribute ** +__weak cache_get_priv_attr(struct device *cache_idx_dev) +{ + return NULL; +} + +/* Add/Remove cache interface for CPU device */ +static void cpu_cache_sysfs_exit(unsigned int cpu) +{ + int i; + struct device *tmp_dev; + const struct device_attribute **ci_priv_attr; + + if (per_cpu_index_dev(cpu)) { + for (i = 0; i < cache_leaves(cpu); i++) { + tmp_dev = per_cache_index_dev(cpu, i); + if (!tmp_dev) + continue; + ci_priv_attr = cache_get_priv_attr(tmp_dev); + device_remove_attrs(tmp_dev, ci_priv_attr); + device_remove_attrs(tmp_dev, cache_optional_attrs); + device_unregister(tmp_dev); + } + kfree(per_cpu_index_dev(cpu)); + per_cpu_index_dev(cpu) = NULL; + } + device_unregister(per_cpu_cache_dev(cpu)); + per_cpu_cache_dev(cpu) = NULL; +} + +static int cpu_cache_sysfs_init(unsigned int cpu) +{ + struct device *dev = get_cpu_device(cpu); + + if (per_cpu_cacheinfo(cpu) == NULL) + return -ENOENT; + + per_cpu_cache_dev(cpu) = device_create(dev->class, dev, cpu, + NULL, "cache"); + if (IS_ERR_OR_NULL(per_cpu_cache_dev(cpu))) + return PTR_ERR(per_cpu_cache_dev(cpu)); + + /* Allocate all required memory */ + per_cpu_index_dev(cpu) = kzalloc(sizeof(struct device *) * + cache_leaves(cpu), GFP_KERNEL); + if (unlikely(per_cpu_index_dev(cpu) == NULL)) + goto err_out; + + return 0; + +err_out: + cpu_cache_sysfs_exit(cpu); + return -ENOMEM; +} + +static int cache_add_dev(unsigned int cpu) +{ + unsigned short i; + int rc; + struct device *tmp_dev, *parent; + struct cacheinfo *this_leaf; + const struct device_attribute **ci_priv_attr; + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); + + rc = cpu_cache_sysfs_init(cpu); + if (unlikely(rc < 0)) + return rc; + + parent = per_cpu_cache_dev(cpu); + for (i = 0; i < cache_leaves(cpu); i++) { + this_leaf = this_cpu_ci->info_list + i; + if (this_leaf->disable_sysfs) + continue; + tmp_dev = device_create_with_groups(parent->class, parent, i, + this_leaf, + cache_default_groups, + "index%1u", i); + if (IS_ERR_OR_NULL(tmp_dev)) { + rc = PTR_ERR(tmp_dev); + goto err; + } + + rc = device_add_attrs(tmp_dev, cache_optional_attrs); + if (unlikely(rc)) + goto err; + + ci_priv_attr = cache_get_priv_attr(tmp_dev); + rc = device_add_attrs(tmp_dev, ci_priv_attr); + if (unlikely(rc)) + goto err;You just raced with userspace here, creating these files _after_ the device was announced to userspace, causing problems with anyone wanting to read these attributes :( I think if you fix up the is_visible() thing above, these calls will go away, right?
Yes I agree. Regards, Sudeep _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
