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:

quoted

+static const struct device_attribute *cache_optional_attrs[] =3D {
+=09&dev_attr_coherency_line_size,
+=09&dev_attr_ways_of_associativity,
+=09&dev_attr_number_of_sets,
+=09&dev_attr_size,
+=09&dev_attr_attributes,
+=09&dev_attr_physical_line_partition,
+=09NULL
+};
+
+static int device_add_attrs(struct device *dev,
+=09=09=09    const struct device_attribute **dev_attrs)
+{
+=09int i, error =3D 0;
+=09struct device_attribute *dev_attr;
+=09char *buf;
+
+=09if (!dev_attrs)
+=09=09return 0;
+
+=09buf =3D kmalloc(PAGE_SIZE, GFP_KERNEL);
+=09if (!buf)
+=09=09return -ENOMEM;
+
+=09for (i =3D 0; dev_attrs[i]; i++) {
+=09=09dev_attr =3D (struct device_attribute *)dev_attrs[i];
+
+=09=09/* create attributes that provides meaningful value */
+=09=09if (dev_attr->show(dev, dev_attr, buf) < 0)
+=09=09=09continue;
+
+=09=09error =3D device_create_file(dev, dev_attrs[i]);
+=09=09if (error) {
+=09=09=09while (--i >=3D 0)
+=09=09=09=09device_remove_file(dev, dev_attrs[i]);
+=09=09=09break;
+=09=09}
+=09}
+
+=09kfree(buf);
+=09return 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 loo=
k
at it.

quoted

+static void device_remove_attrs(struct device *dev,
+=09=09=09=09const struct device_attribute **dev_attrs)
+{
+=09int i;
+
+=09if (!dev_attrs)
+=09=09return;
+
+=09for (i =3D 0; dev_attrs[i]; dev_attrs++, i++)
+=09=09device_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.

quoted

+
+const struct device_attribute **
+__weak cache_get_priv_attr(struct device *cache_idx_dev)
+{
+=09return NULL;
+}
+
+/* Add/Remove cache interface for CPU device */
+static void cpu_cache_sysfs_exit(unsigned int cpu)
+{
+=09int i;
+=09struct device *tmp_dev;
+=09const struct device_attribute **ci_priv_attr;
+
+=09if (per_cpu_index_dev(cpu)) {
+=09=09for (i =3D 0; i < cache_leaves(cpu); i++) {
+=09=09=09tmp_dev =3D per_cache_index_dev(cpu, i);
+=09=09=09if (!tmp_dev)
+=09=09=09=09continue;
+=09=09=09ci_priv_attr =3D cache_get_priv_attr(tmp_dev);
+=09=09=09device_remove_attrs(tmp_dev, ci_priv_attr);
+=09=09=09device_remove_attrs(tmp_dev, cache_optional_attrs);
+=09=09=09device_unregister(tmp_dev);
+=09=09}
+=09=09kfree(per_cpu_index_dev(cpu));
+=09=09per_cpu_index_dev(cpu) =3D NULL;
+=09}
+=09device_unregister(per_cpu_cache_dev(cpu));
+=09per_cpu_cache_dev(cpu) =3D NULL;
+}
+
+static int cpu_cache_sysfs_init(unsigned int cpu)
+{
+=09struct device *dev =3D get_cpu_device(cpu);
+
+=09if (per_cpu_cacheinfo(cpu) =3D=3D NULL)
+=09=09return -ENOENT;
+
+=09per_cpu_cache_dev(cpu) =3D device_create(dev->class, dev, cpu,
+=09=09=09=09=09       NULL, "cache");
+=09if (IS_ERR_OR_NULL(per_cpu_cache_dev(cpu)))
+=09=09return PTR_ERR(per_cpu_cache_dev(cpu));
+
+=09/* Allocate all required memory */
+=09per_cpu_index_dev(cpu) =3D kzalloc(sizeof(struct device *) *
+=09=09=09=09=09 cache_leaves(cpu), GFP_KERNEL);
+=09if (unlikely(per_cpu_index_dev(cpu) =3D=3D NULL))
+=09=09goto err_out;
+
+=09return 0;
+
+err_out:
+=09cpu_cache_sysfs_exit(cpu);
+=09return -ENOMEM;
+}
+
+static int cache_add_dev(unsigned int cpu)
+{
+=09unsigned short i;
+=09int rc;
+=09struct device *tmp_dev, *parent;
+=09struct cacheinfo *this_leaf;
+=09const struct device_attribute **ci_priv_attr;
+=09struct cpu_cacheinfo *this_cpu_ci =3D get_cpu_cacheinfo(cpu);
+
+=09rc =3D cpu_cache_sysfs_init(cpu);
+=09if (unlikely(rc < 0))
+=09=09return rc;
+
+=09parent =3D per_cpu_cache_dev(cpu);
+=09for (i =3D 0; i < cache_leaves(cpu); i++) {
+=09=09this_leaf =3D this_cpu_ci->info_list + i;
+=09=09if (this_leaf->disable_sysfs)
+=09=09=09continue;
+=09=09tmp_dev =3D device_create_with_groups(parent->class, parent, i,
+=09=09=09=09=09=09    this_leaf,
+=09=09=09=09=09=09    cache_default_groups,
+=09=09=09=09=09=09    "index%1u", i);
+=09=09if (IS_ERR_OR_NULL(tmp_dev)) {
+=09=09=09rc =3D PTR_ERR(tmp_dev);
+=09=09=09goto err;
+=09=09}
+
+=09=09rc =3D device_add_attrs(tmp_dev, cache_optional_attrs);
+=09=09if (unlikely(rc))
+=09=09=09goto err;
+
+=09=09ci_priv_attr =3D cache_get_priv_attr(tmp_dev);
+=09=09rc =3D device_add_attrs(tmp_dev, ci_priv_attr);
+=09=09if (unlikely(rc))
+=09=09=09goto 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