[PATCH V4 0/2] Add support for OCC command/response interface

[PATCH V4 0/2] Add support for OCC command/response interface

[Shilpasri G Bhat]

In P9, OCC(On Chip Controller) can be sent commands inband via shared
memory based command response interface.  This patch adds a platform
driver to support the OCC command-response interface.

The skiboot patch for the interface is posted here:
https://lists.ozlabs.org/pipermail/skiboot/2017-June/007705.html

Shilpasri G Bhat (2):
  powerpc/powernv: Get a unique token for async completions
  powerpc/powernv : Add support for OPAL-OCC command/response interface

 arch/powerpc/include/asm/opal-api.h            |  41 +++-
 arch/powerpc/include/asm/opal.h                |   4 +
 arch/powerpc/platforms/powernv/Makefile        |   2 +-
 arch/powerpc/platforms/powernv/opal-async.c    |  46 ++++
 arch/powerpc/platforms/powernv/opal-occ.c      | 313 +++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
 arch/powerpc/platforms/powernv/opal.c          |   8 +
 7 files changed, 413 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/opal-occ.c

-- 
1.8.3.1

[PATCH V4 1/2] powerpc/powernv: Get a unique token for async completions

[Shilpasri G Bhat]

This patch adds support to get a unique token for async completion
requests. This will be used for creating non-repititive request
handles for consecutive requests in OPAL-OCC command/response
interface.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
- No changes from V3

 arch/powerpc/include/asm/opal.h             |  1 +
 arch/powerpc/platforms/powernv/opal-async.c | 46 +++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 588fb1c..03ed493 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -293,6 +293,7 @@ extern int opal_message_notifier_unregister(enum opal_msg_type msg_type,
 
 extern int __opal_async_get_token(void);
 extern int opal_async_get_token_interruptible(void);
+extern int opal_async_get_unique_token_interruptible(int last_token);
 extern int __opal_async_release_token(int token);
 extern int opal_async_release_token(int token);
 extern int opal_async_wait_response(uint64_t token, struct opal_msg *msg);
diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index 83bebee..8caeea2 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -73,6 +73,52 @@ int opal_async_get_token_interruptible(void)
 }
 EXPORT_SYMBOL_GPL(opal_async_get_token_interruptible);
 
+static int __opal_async_get_new_token(int last_token)
+{
+	unsigned long flags;
+	int token;
+
+	spin_lock_irqsave(&opal_async_comp_lock, flags);
+	token = find_next_bit(opal_async_complete_map, opal_max_async_tokens,
+			      last_token + 1);
+	if (token >= opal_max_async_tokens) {
+		token = find_first_bit(opal_async_complete_map,
+				       opal_max_async_tokens);
+		if (token >= opal_max_async_tokens || token == last_token) {
+			token = -EBUSY;
+			goto out;
+		}
+	}
+
+	pr_debug("%s token = %d\n", __func__, token);
+	if (__test_and_set_bit(token, opal_async_token_map)) {
+		token = -EBUSY;
+		goto out;
+	}
+
+	__clear_bit(token, opal_async_complete_map);
+
+out:
+	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
+	return token;
+}
+
+int opal_async_get_unique_token_interruptible(int last_token)
+{
+	int token;
+
+	/* Wait until a token is available */
+	if (down_interruptible(&opal_async_sem))
+		return -ERESTARTSYS;
+
+	token = __opal_async_get_new_token(last_token);
+	if (token < 0)
+		up(&opal_async_sem);
+
+	return token;
+}
+EXPORT_SYMBOL_GPL(opal_async_get_unique_token_interruptible);
+
 int __opal_async_release_token(int token)
 {
 	unsigned long flags;
-- 
1.8.3.1

[PATCH V4 2/2] powerpc/powernv : Add support for OPAL-OCC command/response interface

[Shilpasri G Bhat]

In P9, OCC (On-Chip-Controller) supports shared memory based
commad-response interface. Within the shared memory there is an OPAL
command buffer and OCC response buffer that can be used to send
inband commands to OCC. This patch adds a platform driver to support
the command/response interface between OCC and the host.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
- Hold occ->cmd_in_progress in read()
- Reset occ->rsp_consumed if copy_to_user() fails

 arch/powerpc/include/asm/opal-api.h            |  41 +++-
 arch/powerpc/include/asm/opal.h                |   3 +
 arch/powerpc/platforms/powernv/Makefile        |   2 +-
 arch/powerpc/platforms/powernv/opal-occ.c      | 313 +++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
 arch/powerpc/platforms/powernv/opal.c          |   8 +
 6 files changed, 366 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/opal-occ.c

diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index cb3e624..011d86c 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -42,6 +42,10 @@
 #define OPAL_I2C_STOP_ERR	-24
 #define OPAL_XIVE_PROVISIONING	-31
 #define OPAL_XIVE_FREE_ACTIVE	-32
+#define OPAL_OCC_INVALID_STATE	-33
+#define OPAL_OCC_BUSY		-34
+#define OPAL_OCC_CMD_TIMEOUT	-35
+#define OPAL_OCC_RSP_MISMATCH	-36
 
 /* API Tokens (in r0) */
 #define OPAL_INVALID_CALL		       -1
@@ -190,7 +194,8 @@
 #define OPAL_NPU_INIT_CONTEXT			146
 #define OPAL_NPU_DESTROY_CONTEXT		147
 #define OPAL_NPU_MAP_LPAR			148
-#define OPAL_LAST				148
+#define OPAL_OCC_COMMAND			149
+#define OPAL_LAST				149
 
 /* Device tree flags */
 
@@ -829,6 +834,40 @@ struct opal_prd_msg_header {
 
 struct opal_prd_msg;
 
+enum occ_cmd {
+	OCC_CMD_AMESTER_PASS_THRU = 0,
+	OCC_CMD_CLEAR_SENSOR_DATA,
+	OCC_CMD_SET_POWER_CAP,
+	OCC_CMD_SET_POWER_SHIFTING_RATIO,
+	OCC_CMD_SELECT_SENSOR_GROUPS,
+	OCC_CMD_LAST
+};
+
+struct opal_occ_cmd_rsp_msg {
+	__be64 cdata;
+	__be64 rdata;
+	__be16 cdata_size;
+	__be16 rdata_size;
+	u8 cmd;
+	u8 request_id;
+	u8 status;
+};
+
+struct opal_occ_cmd_data {
+	__be16 size;
+	u8 cmd;
+	u8 data[];
+};
+
+struct opal_occ_rsp_data {
+	__be16 size;
+	u8 status;
+	u8 data[];
+};
+
+#define MAX_OPAL_CMD_DATA_LENGTH        4090
+#define MAX_OCC_RSP_DATA_LENGTH         8698
+
 #define OCC_RESET                       0
 #define OCC_LOAD                        1
 #define OCC_THROTTLE                    2
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 03ed493..e55ed79 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -346,6 +346,9 @@ static inline int opal_get_async_rc(struct opal_msg msg)
 
 void opal_wake_poller(void);
 
+int64_t opal_occ_command(int chip_id, struct opal_occ_cmd_rsp_msg *msg,
+			 bool retry);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_POWERPC_OPAL_H */
diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index b5d98cb..f5f0902 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -2,7 +2,7 @@ obj-y			+= setup.o opal-wrappers.o opal.o opal-async.o idle.o
 obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
 obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
 obj-y			+= opal-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
-obj-y			+= opal-kmsg.o
+obj-y			+= opal-kmsg.o opal-occ.o
 
 obj-$(CONFIG_SMP)	+= smp.o subcore.o subcore-asm.o
 obj-$(CONFIG_PCI)	+= pci.o pci-ioda.o npu-dma.o
diff --git a/arch/powerpc/platforms/powernv/opal-occ.c b/arch/powerpc/platforms/powernv/opal-occ.c
new file mode 100644
index 0000000..b346724
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/opal-occ.c
@@ -0,0 +1,313 @@
+/*
+ * Copyright IBM Corporation 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "opal-occ: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <asm/opal.h>
+
+struct occ {
+	struct miscdevice dev;
+	struct opal_occ_rsp_data *rsp;
+	atomic_t session;
+	atomic_t cmd_in_progress;
+	atomic_t rsp_consumed;
+	int id;
+	u8 last_token;
+} *occs;
+static int nr_occs;
+
+static int __send_occ_command(struct opal_occ_cmd_rsp_msg *msg,
+			      int chip_id, bool retry)
+{
+	struct opal_msg async_msg;
+	int rc;
+
+	rc = opal_occ_command(chip_id, msg, retry);
+	if (rc == OPAL_ASYNC_COMPLETION) {
+		rc = opal_async_wait_response(msg->request_id, &async_msg);
+		if (rc) {
+			pr_devel("Failed to wait for async response %d\n", rc);
+			return rc;
+		}
+	}
+
+	return rc ? rc : opal_get_async_rc(async_msg);
+}
+
+static int send_occ_command(struct opal_occ_cmd_rsp_msg *msg, struct occ *occ)
+{
+	int token, rc;
+
+	token = opal_async_get_unique_token_interruptible(occ->last_token);
+	if (token < 0) {
+		pr_devel("Failed to get the token for OCC command %d (%d)\n",
+			 msg->cmd, token);
+		return token;
+	}
+
+	msg->request_id = token;
+	rc = __send_occ_command(msg, occ->id, false);
+
+	switch (rc) {
+	case OPAL_OCC_CMD_TIMEOUT:
+	case OPAL_OCC_RSP_MISMATCH:
+		pr_devel("Failed OCC command with %d. Retrying it again\n", rc);
+		occ->last_token = token;
+		opal_async_release_token(token);
+		token = opal_async_get_unique_token_interruptible(token);
+		if (token < 0) {
+			pr_devel("Failed to get the token for retry OCC command %d (%d)\n",
+				 msg->cmd, token);
+			return opal_error_code(rc);
+		}
+
+		msg->request_id = token;
+		rc = __send_occ_command(msg, occ->id, true);
+		break;
+	default:
+		break;
+	}
+
+	occ->last_token = token;
+	opal_async_release_token(token);
+	return opal_error_code(rc);
+}
+
+static int opal_occ_cmd_prepare(struct opal_occ_cmd_data *cmd, struct occ *occ)
+{
+	struct opal_occ_cmd_rsp_msg msg;
+	int rc;
+
+	msg.cmd = cmd->cmd;
+	msg.cdata = cpu_to_be64(__pa(cmd->data));
+	msg.cdata_size = cpu_to_be16(cmd->size);
+	msg.rdata = cpu_to_be64(__pa(occ->rsp->data));
+
+	rc = send_occ_command(&msg, occ);
+	if (rc) {
+		pr_info("Failed OCC command %d with %d\n", cmd->cmd, rc);
+		return rc;
+	}
+
+	occ->rsp->size = be16_to_cpu(msg.rdata_size);
+	occ->rsp->status = msg.status;
+	if (occ->rsp->size > MAX_OCC_RSP_DATA_LENGTH) {
+		pr_devel("Bigger OCC response size, clipping to %d\n",
+			 MAX_OCC_RSP_DATA_LENGTH);
+		occ->rsp->size = MAX_OCC_RSP_DATA_LENGTH;
+	}
+
+	atomic_set(&occ->rsp_consumed, 1);
+	return rc;
+}
+
+static ssize_t opal_occ_write(struct file *file, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	struct miscdevice *dev = file->private_data;
+	struct occ *occ = container_of(dev, struct occ, dev);
+	struct opal_occ_cmd_data *cmd;
+	int rc;
+
+	if (count < sizeof(*cmd))
+		return -EINVAL;
+
+	if (atomic_cmpxchg(&occ->cmd_in_progress, 0, 1))
+		return -EBUSY;
+
+	cmd = kmalloc(count, GFP_KERNEL);
+	if (!cmd) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = copy_from_user(cmd, buf, count);
+	if (rc) {
+		pr_err("Failed to copy OCC command request message\n");
+		rc = -EFAULT;
+		goto free_cmd;
+	}
+
+	if (cmd->size > MAX_OPAL_CMD_DATA_LENGTH) {
+		rc = -EINVAL;
+		goto free_cmd;
+	}
+
+	rc = opal_occ_cmd_prepare(cmd, occ);
+	if (!rc)
+		rc = count;
+
+free_cmd:
+	kfree(cmd);
+out:
+	atomic_set(&occ->cmd_in_progress, 0);
+	return rc;
+}
+
+static ssize_t opal_occ_read(struct file *file, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	struct miscdevice *dev = file->private_data;
+	struct occ *occ = container_of(dev, struct occ, dev);
+	int rc;
+
+	if (count < sizeof(*occ->rsp) + occ->rsp->size)
+		return -EINVAL;
+
+	if (!atomic_cmpxchg(&occ->rsp_consumed, 1, 0))
+		return -EBUSY;
+
+	if (atomic_cmpxchg(&occ->cmd_in_progress, 0, 1))
+		return -EBUSY;
+
+	rc = copy_to_user((void __user *)buf, occ->rsp,
+			  sizeof(occ->rsp) + occ->rsp->size);
+	if (rc) {
+		atomic_set(&occ->rsp_consumed, 1);
+		atomic_set(&occ->cmd_in_progress, 0);
+		pr_err("Failed to copy OCC response data to user\n");
+		return rc;
+	}
+
+	atomic_set(&occ->cmd_in_progress, 0);
+	return sizeof(*occ->rsp) + occ->rsp->size;
+}
+
+static int opal_occ_open(struct inode *inode, struct file *file)
+{
+	struct miscdevice *dev = file->private_data;
+	struct occ *occ = container_of(dev, struct occ, dev);
+
+	return atomic_cmpxchg(&occ->session, 0, 1) ? -EBUSY : 0;
+}
+
+static int opal_occ_release(struct inode *inode, struct file *file)
+{
+	struct miscdevice *dev = file->private_data;
+	struct occ *occ = container_of(dev, struct occ, dev);
+
+	atomic_set(&occ->session, 0);
+
+	return 0;
+}
+
+static const struct file_operations opal_occ_fops = {
+	.open		= opal_occ_open,
+	.read		= opal_occ_read,
+	.write		= opal_occ_write,
+	.release	= opal_occ_release,
+	.owner		= THIS_MODULE,
+};
+
+#define MAX_POSSIBLE_CHIPS	256
+
+static int opal_occ_probe(struct platform_device *pdev)
+{
+	unsigned int chip[MAX_POSSIBLE_CHIPS];
+	unsigned int cpu;
+	unsigned int prev_chip_id = UINT_MAX;
+	int i, rc;
+
+	for_each_possible_cpu(cpu) {
+		unsigned int id = cpu_to_chip_id(cpu);
+
+		if (prev_chip_id != id) {
+			int j = nr_occs;
+
+			while (--j >= 0)
+				if (chip[j] == id)
+					continue;
+
+			prev_chip_id = id;
+			chip[nr_occs++] = id;
+			WARN_ON_ONCE(nr_occs >= MAX_POSSIBLE_CHIPS - 1);
+		}
+	}
+
+	occs = kcalloc(nr_occs, sizeof(*occs), GFP_KERNEL);
+	if (!occs)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_occs; i++) {
+		char name[10];
+
+		occs[i].id = chip[i];
+		occs[i].dev.minor = MISC_DYNAMIC_MINOR;
+		snprintf(name, 10, "occ%d", chip[i]);
+		occs[i].dev.name = name;
+		occs[i].dev.fops = &opal_occ_fops;
+		occs[i].last_token = -1;
+		occs[i].rsp = kmalloc(sizeof(occs[i].rsp) +
+				      MAX_OCC_RSP_DATA_LENGTH,
+				      GFP_KERNEL);
+		if (!occs[i].rsp) {
+			rc = -ENOMEM;
+			goto free_occs;
+		}
+
+		rc = misc_register(&occs[i].dev);
+		if (rc)
+			goto free_occ_rsp_data;
+	}
+
+	return 0;
+
+free_occ_rsp_data:
+	kfree(occs[i].rsp);
+free_occs:
+	while (--i >= 0) {
+		kfree(occs[i].rsp);
+		misc_deregister(&occs[i].dev);
+	}
+	kfree(occs);
+
+	return rc;
+}
+
+static int opal_occ_remove(struct platform_device *pdev)
+{
+	int i;
+
+	for (i = 0; i < nr_occs; i++) {
+		kfree(occs[i].rsp);
+		misc_deregister(&occs[i].dev);
+	}
+
+	kfree(occs);
+	return 0;
+}
+
+static const struct of_device_id opal_occ_match[] = {
+	{ .compatible = "ibm,opal-occ-cmd-rsp-interface" },
+	{ },
+};
+
+static struct platform_driver opal_occ_driver = {
+	.driver = {
+		.name           = "opal-occ",
+		.of_match_table = opal_occ_match,
+	},
+	.probe	= opal_occ_probe,
+	.remove	= opal_occ_remove,
+};
+
+module_platform_driver(opal_occ_driver);
+
+MODULE_DESCRIPTION("PowerNV OPAL-OCC driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index f620572..e6bf18d 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -310,3 +310,4 @@ OPAL_CALL(opal_xive_dump,			OPAL_XIVE_DUMP);
 OPAL_CALL(opal_npu_init_context,		OPAL_NPU_INIT_CONTEXT);
 OPAL_CALL(opal_npu_destroy_context,		OPAL_NPU_DESTROY_CONTEXT);
 OPAL_CALL(opal_npu_map_lpar,			OPAL_NPU_MAP_LPAR);
+OPAL_CALL(opal_occ_command,			OPAL_OCC_COMMAND);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 59684b4..d87c61b 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -815,6 +815,9 @@ static int __init opal_init(void)
 	opal_pdev_init("ibm,opal-flash");
 	opal_pdev_init("ibm,opal-prd");
 
+	/* Initialize platform device: OCC_OPAL command-response interface */
+	opal_pdev_init("ibm,opal-occ-cmd-rsp-interface");
+
 	/* Initialise platform device: oppanel interface */
 	opal_pdev_init("ibm,opal-oppanel");
 
@@ -859,6 +862,7 @@ void opal_shutdown(void)
 EXPORT_SYMBOL_GPL(opal_flash_write);
 EXPORT_SYMBOL_GPL(opal_flash_erase);
 EXPORT_SYMBOL_GPL(opal_prd_msg);
+EXPORT_SYMBOL_GPL(opal_occ_command);
 
 /* Convert a region of vmalloc memory to an opal sg list */
 struct opal_sg_list *opal_vmalloc_to_sg_list(void *vmalloc_addr,
@@ -937,6 +941,10 @@ int opal_error_code(int rc)
 	case OPAL_UNSUPPORTED:		return -EIO;
 	case OPAL_HARDWARE:		return -EIO;
 	case OPAL_INTERNAL_ERROR:	return -EIO;
+	case OPAL_OCC_BUSY:		return -EBUSY;
+	case OPAL_OCC_INVALID_STATE:
+	case OPAL_OCC_CMD_TIMEOUT:
+	case OPAL_OCC_RSP_MISMATCH:	return -EIO;
 	default:
 		pr_err("%s: unexpected OPAL error %d\n", __func__, rc);
 		return -EIO;
-- 
1.8.3.1

Re: [PATCH V4 2/2] powerpc/powernv : Add support for OPAL-OCC command/response interface

[Cyril Bur]

On Wed, 2017-06-21 at 13:36 +0530, Shilpasri G Bhat wrote:
> In P9, OCC (On-Chip-Controller) supports shared memory based
> commad-response interface. Within the shared memory there is an OPAL
> command buffer and OCC response buffer that can be used to send
> inband commands to OCC. This patch adds a platform driver to support
> the command/response interface between OCC and the host.
> 

Sorry I probably should have pointed out earlier that I don't really
understand the first patch or exactly what problem you're trying to
solve. I've left it ignored, feel free to explain what the idea is
there or hopefully someone who can see what you're trying to do can
step in.

As for this patch, just one thing.


> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> - Hold occ->cmd_in_progress in read()
> - Reset occ->rsp_consumed if copy_to_user() fails
> 
>  arch/powerpc/include/asm/opal-api.h            |  41 +++-
>  arch/powerpc/include/asm/opal.h                |   3 +
>  arch/powerpc/platforms/powernv/Makefile        |   2 +-
>  arch/powerpc/platforms/powernv/opal-occ.c      | 313 +++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
>  arch/powerpc/platforms/powernv/opal.c          |   8 +
>  6 files changed, 366 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/platforms/powernv/opal-occ.c
> 

[snip]

> +
> +static ssize_t opal_occ_read(struct file *file, char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct miscdevice *dev = file->private_data;
> +	struct occ *occ = container_of(dev, struct occ, dev);
> +	int rc;
> +
> +	if (count < sizeof(*occ->rsp) + occ->rsp->size)
> +		return -EINVAL;
> +
> +	if (!atomic_cmpxchg(&occ->rsp_consumed, 1, 0))
> +		return -EBUSY;
> +
> +	if (atomic_cmpxchg(&occ->cmd_in_progress, 0, 1))
> +		return -EBUSY;
> +

Personally I would have done these two checks the other way around, it
doesn't really matter which one you do first but what does matter is
that you undo the change you did in the first cmpxchg if the second
cmpxchg causes you do return.

In this case if cmd_in_progress then you'll have marked the response as
consumed...

> +	rc = copy_to_user((void __user *)buf, occ->rsp,
> +			  sizeof(occ->rsp) + occ->rsp->size);
> +	if (rc) {
> +		atomic_set(&occ->rsp_consumed, 1);
> +		atomic_set(&occ->cmd_in_progress, 0);
> +		pr_err("Failed to copy OCC response data to user\n");
> +		return rc;
> +	}
> +
> +	atomic_set(&occ->cmd_in_progress, 0);
> +	return sizeof(*occ->rsp) + occ->rsp->size;
> +}
> +

[snip]

Re: [PATCH V4 2/2] powerpc/powernv : Add support for OPAL-OCC command/response interface

[Shilpasri G Bhat]

Hi Cyril,

On 06/22/2017 06:28 AM, Cyril Bur wrote:
> On Wed, 2017-06-21 at 13:36 +0530, Shilpasri G Bhat wrote:
>> In P9, OCC (On-Chip-Controller) supports shared memory based
>> commad-response interface. Within the shared memory there is an OPAL
>> command buffer and OCC response buffer that can be used to send
>> inband commands to OCC. This patch adds a platform driver to support
>> the command/response interface between OCC and the host.
>>
> 
> Sorry I probably should have pointed out earlier that I don't really
> understand the first patch or exactly what problem you're trying to
> solve. I've left it ignored, feel free to explain what the idea is
> there or hopefully someone who can see what you're trying to do can
> step in.

Thanks for reviewing this patch.

For the first patch however, OCC expects a different request_id in the command
interface every time OPAL is requesting a new command .
'opal_async_get_token_interruptible()' returns a free token from the
'opal_async_complete_map' which does not work for the above OCC requirement as
we may end up getting the same token. Thus the first patch tries to get a new
token excluding a token that was used for the last command.


> 
> As for this patch, just one thing.
> 
> 
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> - Hold occ->cmd_in_progress in read()
>> - Reset occ->rsp_consumed if copy_to_user() fails
>>
>>  arch/powerpc/include/asm/opal-api.h            |  41 +++-
>>  arch/powerpc/include/asm/opal.h                |   3 +
>>  arch/powerpc/platforms/powernv/Makefile        |   2 +-
>>  arch/powerpc/platforms/powernv/opal-occ.c      | 313 +++++++++++++++++++++++++
>>  arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
>>  arch/powerpc/platforms/powernv/opal.c          |   8 +
>>  6 files changed, 366 insertions(+), 2 deletions(-)
>>  create mode 100644 arch/powerpc/platforms/powernv/opal-occ.c
>>
> 
> [snip]
> 
>> +
>> +static ssize_t opal_occ_read(struct file *file, char __user *buf,
>> +			     size_t count, loff_t *ppos)
>> +{
>> +	struct miscdevice *dev = file->private_data;
>> +	struct occ *occ = container_of(dev, struct occ, dev);
>> +	int rc;
>> +
>> +	if (count < sizeof(*occ->rsp) + occ->rsp->size)
>> +		return -EINVAL;
>> +
>> +	if (!atomic_cmpxchg(&occ->rsp_consumed, 1, 0))
>> +		return -EBUSY;
>> +
>> +	if (atomic_cmpxchg(&occ->cmd_in_progress, 0, 1))
>> +		return -EBUSY;
>> +
> 
> Personally I would have done these two checks the other way around, it
> doesn't really matter which one you do first but what does matter is
> that you undo the change you did in the first cmpxchg if the second
> cmpxchg causes you do return.
> 
> In this case if cmd_in_progress then you'll have marked the response as
> consumed...

Here, if cmd_in_progress is set by some other thread doing a write() then it
will set the 'rsp_consumed' to valid on successful command completion. If
write() fails then we are doing a good thing here by not setting 'rsp_consumed'
so the user will not be able to read previous command's response.

Thanks and Regards,
Shilpa

> 
>> +	rc = copy_to_user((void __user *)buf, occ->rsp,
>> +			  sizeof(occ->rsp) + occ->rsp->size);
>> +	if (rc) {
>> +		atomic_set(&occ->rsp_consumed, 1);
>> +		atomic_set(&occ->cmd_in_progress, 0);
>> +		pr_err("Failed to copy OCC response data to user\n");
>> +		return rc;
>> +	}
>> +
>> +	atomic_set(&occ->cmd_in_progress, 0);
>> +	return sizeof(*occ->rsp) + occ->rsp->size;
>> +}
>> +
> 
> [snip]
> 

Re: [PATCH V4 2/2] powerpc/powernv : Add support for OPAL-OCC command/response interface

[Cyril Bur]

On Thu, 2017-06-22 at 09:57 +0530, Shilpasri G Bhat wrote:
> Hi Cyril,
> 
> On 06/22/2017 06:28 AM, Cyril Bur wrote:
> > On Wed, 2017-06-21 at 13:36 +0530, Shilpasri G Bhat wrote:
> > > In P9, OCC (On-Chip-Controller) supports shared memory based
> > > commad-response interface. Within the shared memory there is an OPAL
> > > command buffer and OCC response buffer that can be used to send
> > > inband commands to OCC. This patch adds a platform driver to support
> > > the command/response interface between OCC and the host.
> > > 
> > 
> > Sorry I probably should have pointed out earlier that I don't really
> > understand the first patch or exactly what problem you're trying to
> > solve. I've left it ignored, feel free to explain what the idea is
> > there or hopefully someone who can see what you're trying to do can
> > step in.
> 
> Thanks for reviewing this patch.
> 
> For the first patch however, OCC expects a different request_id in the command
> interface every time OPAL is requesting a new command .
> 'opal_async_get_token_interruptible()' returns a free token from the
> 'opal_async_complete_map' which does not work for the above OCC requirement as
> we may end up getting the same token. Thus the first patch tries to get a new
> token excluding a token that was used for the last command.
> 

Oh yes I see because you're using the OPAL token to the opal call as
the OCC msg request_id. Is there any requirement that these two numbers
match?

Is the 'different' a per OCC or a global 'all OCC' requirement?

Couldn't you just have a counter in your occ struct in this patch that
you increment on each call?

I've looked through your skiboot patch and it looks like you use the
msg->request_id so that you know who to respond to - wouldn't be easier
to just pass the opal token through as an extra parameter to
OPAL_OCC_COMMAND and store it alongside the msg structure?

Apologies I shouldn't have overlooked this for so long.

> 
> > 
> > As for this patch, just one thing.
> > 
> > 
> > > Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> > > ---
> > > - Hold occ->cmd_in_progress in read()
> > > - Reset occ->rsp_consumed if copy_to_user() fails
> > > 
> > >  arch/powerpc/include/asm/opal-api.h            |  41 +++-
> > >  arch/powerpc/include/asm/opal.h                |   3 +
> > >  arch/powerpc/platforms/powernv/Makefile        |   2 +-
> > >  arch/powerpc/platforms/powernv/opal-occ.c      | 313 +++++++++++++++++++++++++
> > >  arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
> > >  arch/powerpc/platforms/powernv/opal.c          |   8 +
> > >  6 files changed, 366 insertions(+), 2 deletions(-)
> > >  create mode 100644 arch/powerpc/platforms/powernv/opal-occ.c
> > > 
> > 
> > [snip]
> > 
> > > +
> > > +static ssize_t opal_occ_read(struct file *file, char __user *buf,
> > > +			     size_t count, loff_t *ppos)
> > > +{
> > > +	struct miscdevice *dev = file->private_data;
> > > +	struct occ *occ = container_of(dev, struct occ, dev);
> > > +	int rc;
> > > +
> > > +	if (count < sizeof(*occ->rsp) + occ->rsp->size)
> > > +		return -EINVAL;
> > > +
> > > +	if (!atomic_cmpxchg(&occ->rsp_consumed, 1, 0))
> > > +		return -EBUSY;
> > > +
> > > +	if (atomic_cmpxchg(&occ->cmd_in_progress, 0, 1))
> > > +		return -EBUSY;
> > > +
> > 
> > Personally I would have done these two checks the other way around, it
> > doesn't really matter which one you do first but what does matter is
> > that you undo the change you did in the first cmpxchg if the second
> > cmpxchg causes you do return.
> > 
> > In this case if cmd_in_progress then you'll have marked the response as
> > consumed...
> 
> Here, if cmd_in_progress is set by some other thread doing a write() then it
> will set the 'rsp_consumed' to valid on successful command completion. If
> write() fails then we are doing a good thing here by not setting 'rsp_consumed'
> so the user will not be able to read previous command's response.
> 

Yep, if you're happy I'm happy, I suppose there is a scenario where you
might lose a response.

read() and sees that there is a response, and marks it as consumed,
meanwhile a write grabs cmd_in_progress which causes the read()er to
return EBUSY but something fails before the write()er can actually send
an occ command so there is a lost command there... I mean, either way
we lose that command since if the write() had successfully sent the
command we would have overwritten the previous response... if you're
happy with this I'm happy with this. It is incredibly unlikey to happen
and this should be the same program doing this, I'm happy to have them
be able to shoot themselves in the foot like that.

> Thanks and Regards,
> Shilpa
> 
> > 
> > > +	rc = copy_to_user((void __user *)buf, occ->rsp,
> > > +			  sizeof(occ->rsp) + occ->rsp->size);
> > > +	if (rc) {
> > > +		atomic_set(&occ->rsp_consumed, 1);
> > > +		atomic_set(&occ->cmd_in_progress, 0);
> > > +		pr_err("Failed to copy OCC response data to user\n");
> > > +		return rc;
> > > +	}
> > > +
> > > +	atomic_set(&occ->cmd_in_progress, 0);
> > > +	return sizeof(*occ->rsp) + occ->rsp->size;
> > > +}
> > > +
> > 
> > [snip]
> > 
> 
>