Re: [3/5] pseries: Create device hotplug entry point

On Mon, 2014-09-15 at 15:31 -0500, Nathan Fontenot wrote:

For pseries system the kernel will be notified of hotplug requests in
the form of rtas hotplug events. 

Can you flesh that design out a bit for me, I don't entirely get how it's going
to work.

The kernel gets the rtas hotplug events (in rtasd.c) and spits them out to
userspace, which then writes them back in ?

This patch creates a common routine that can handle these requests in both
the PowerVM anbd PowerKVM environments, handle_dlpar_errorlog(). This also

                ^

creates the initial memory hotplug request handling stub.

For PowerVM this patch also creates a new /proc file that the drmgr
command will use to write rtas hotplug events to.

Why is this different between phyp and KVM?

For future PowerKVM handling the rtas check-exception code can pass
any rtas hotplug events received to handle_dlpar_errorlog().

Internally to the kernel you mean?

That is really confusing, but I think it's just a diff artifact?

+static int handle_dlpar_errorlog(struct rtas_error_log *error_log)
+{
+	struct pseries_errorlog *pseries_log;
+	struct pseries_hp_errorlog *hp_elog;
+	int rc = -EINVAL;
+
+	pseries_log = get_pseries_errorlog(error_log,
+					   PSERIES_ELOG_SECT_ID_HOTPLUG);
+	if (!pseries_log)
+		return rc;
+
+	hp_elog = (struct pseries_hp_errorlog *)pseries_log->data;
+	if (!hp_elog)
+		return rc;

I don't see how that can happen?

struct pseries_errorlog {
	__be16 id;			/* 0x00 2-byte ASCII section ID	*/
	__be16 length;			/* 0x02 Section length in bytes	*/
	uint8_t version;		/* 0x04 Section version		*/
	uint8_t subtype;		/* 0x05 Section subtype		*/
	__be16 creator_component;	/* 0x06 Creator component ID	*/
	uint8_t data[];			/* 0x08 Start of section data	*/
};

Should you be checking for length == 0 instead ?

Also I think the code will probably end up cleaner if you do the endian
conversions immediately when you read the hp_elog, rather than passing it
around in BE and having to remember to convert at all the usages.

+	switch (hp_elog->resource) {
+	case PSERIES_HP_ELOG_RESOURCE_MEM:
+		rc = dlpar_memory(hp_elog);
+		break;

Please add:

  default:
	pr_warn_ratelimited("Unknown resource ..")

Or something.

+	}
+
+	return rc;
+}
+
+static ssize_t dlpar_write(struct file *file, const char __user *buf,
+			   size_t count, loff_t *offset)
+{
+	char *event_buf;
+	int rc;
+
+	event_buf = kmalloc(count + 1, GFP_KERNEL);

Why + 1 ? It's not null-terminated AFAICS.

+	if (!event_buf)
+		return -ENOMEM;
+
+	rc = copy_from_user(event_buf, buf, count);
+	if (rc) {
+		kfree(event_buf);
+		return rc;
+	}
+
+	rc = handle_dlpar_errorlog((struct rtas_error_log *)event_buf);

If you start with a struct rtas_error_log * you shouldn't need any casts.

+	kfree(event_buf);
+	return rc ? rc : count;
+}
+
+static const struct file_operations dlpar_fops = {
+	.write = dlpar_write,
+	.llseek = noop_llseek,
+};
+
 static int __init pseries_dlpar_init(void)
 {
+	struct proc_dir_entry *proc_ent;
+
+	proc_ent = proc_create("powerpc/dlpar", S_IWUSR, NULL, &dlpar_fops);
+	if (proc_ent)
+		proc_set_size(proc_ent, 0);

  else
	error message at least please

Why are we putting it in /proc, can't it go in /sys/kernel like the mobility
stuff?

static ?

EOPNOTSUPP is a bit more standard.

cheers