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

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:

quoted

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.

quoted

Signed-off-by: Shilpasri G Bhat <redacted>
---
- 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]

quoted

+
+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

quoted

+	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]