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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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.

> +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.

> +
> +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?