Date:	Thu, 10 Jul 2014 14:37:23 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Sudeep Holla <sudeep.holla@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Rob Herring <robh@...nel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
	"linux390@...ibm.com" <linux390@...ibm.com>,
	"linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/9] drivers: base: support cpu cache information interface
 to userspace via sysfs

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists