Re: [PATCH] powerpc/pseries: Export raw per-CPU VPA data via debugfs

Hi Aravinda,

Aravinda Prasad [off-list ref] writes:

This patch exports the raw per-CPU VPA data via debugfs.
A per-CPU file is created which exports the VPA data of
that CPU to help debug some of the VPA related issues or
to analyze the per-CPU VPA related statistics.

Do we really need this in debugfs? I'm not saying we don't, but I'm also
not really clear why we need it.

If there is a good reason for exporting it, do we really want to export
it in distro kernels, or should it be behind a CONFIG ?

You never use this other than temporarily when creating the file.

+	int		cpu;
+};
+static DEFINE_PER_CPU(struct vpa, cpu_vpa);

And you never really use the per_cpu() either.

So you don't need the vpa struct at all AFAICS.

+
+static struct dentry *vpa_dir;

That can just be a local in vpa_debugfs_init().

Like this please:

static ssize_t vpa_file_read(struct file *filp, char __user *buf, size_t len,
			     loff_t *pos)

+{
+	long int rc;
+	struct vpa *vpa = filp->private_data;
+	struct lppaca *lppaca = &lppaca_of(vpa->cpu);

Once you've stashed the CPU number in private_data (see below) you can
get it back with:

	int cpu = (long)filp->private_data;
	struct lppaca *lppaca = &lppaca_of(cpu);

+
+	if (len < sizeof(struct lppaca))
+		return -EINVAL;
+
+	rc = copy_to_user(buf, lppaca, sizeof(struct lppaca));
+	if (rc)
+		return -EFAULT;

You should use simple_read_from_buffer().

+
+	return 0;
+}
+
+static int vpa_file_open(struct inode *inode, struct file *filp)
+{
+	struct vpa *vpa = inode->i_private;
+
+	filp->private_data = vpa;
+	return 0;
+}

You can just use simple_open().

+static int vpa_file_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}

I don't think you need release if it's empty.

+static const struct file_operations vpa_fops = {
+	.open		= vpa_file_open,
+	.release	= vpa_file_release,
+	.read		= vpa_file_read,
+	.llseek		= no_llseek,
+};
+
+static int __init vpa_debugfs_init(void)
+{
+	char name[10];

That's not big enough if you end up with 4 billion CPUs. Make it 16.

+	int i;
+
+	if (!firmware_has_feature(FW_FEATURE_SPLPAR))
+		return 0;
+
+	vpa_dir = debugfs_create_dir("vpa", powerpc_debugfs_root);
+	if (!vpa_dir) {
+		pr_warn("%s: can't create vpa root dir\n", __func__);
+		return -ENOMEM;
+	}
+
+	/* set up the per-cpu vpa file*/
+	for_each_possible_cpu(i) {

What happens when you read the file for a possible but not present CPU?

+		struct vpa *vpa = &per_cpu(cpu_vpa, i);
+
+		vpa->cpu = i;
+		sprintf(name, "cpu-%d", i);
+
+		vpa->file = debugfs_create_file(name, 0400, vpa_dir, vpa,
+					&vpa_fops);

Can be:
		struct dentry *d;
		d = debugfs_create_file(name, 0400, dir, (void *)cpu, &vpa_fops);

Where you're stashing the cpu number in the private_data as a void *.

+		if (!vpa->file) {
+			pr_warn("%s: can't create per-cpu vpa file\n",
+					__func__);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+machine_arch_initcall(pseries, vpa_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */

cheers