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

[Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>]

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