Re: [PATCH 2/9] drivers: base: support cpu cache information interface to userspace via sysfs

Sudeep Holla
2014-07-10T13:37:23Z

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