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

[Sudeep Holla]

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

--
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/