Subject: Re: [PATCH V4 2/2] powerpc/powernv : Add support for OPAL-OCC
 command/response interface
To: Cyril Bur <cyrilbur@gmail.com>, linuxppc-dev@lists.ozlabs.org
References: <1498032387-26010-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
 <1498032387-26010-3-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
 <1498093120.1181.5.camel@gmail.com>
Cc: linux-kernel@vger.kernel.org, benh@kernel.crashing.org,
        paulus@samba.org, mpe@ellerman.id.au, svaidy@linux.vnet.ibm.com,
        ego@linux.vnet.ibm.com
From: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Date: Thu, 22 Jun 2017 09:57:53 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.0
MIME-Version: 1.0
In-Reply-To: <1498093120.1181.5.camel@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Message-Id: <6548f1df-9d0e-a5db-677b-48d3d418b1d6@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3501
Lines: 99

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