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

[Cyril Bur <cyrilbur@gmail.com>]

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]