[PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"

[Eddie James]

On 10/04/2017 08:01 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames at us.ibm.com>
>>   
>> This should only be done at this level,
> Bit of a nit with the grammar here: You're using 'this' in multiple contexts
> and I feel that explanation of the contexts is a bit lacking. Can you please
> expand this sentence to cover those points?
>
> The intent of the change seems reasonable though.
>
> Andrew
>
>> instead of clients repeating
>> the entire transfer when they receive "command in progress"
>>   
>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 3313e35..a554550 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/of.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/sched.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/uaccess.h>
>> @@ -33,6 +34,9 @@
>>   #define OCC_CMD_DATA_BYTES	4090
>>   #define OCC_RESP_DATA_BYTES	4089
>>   
>> +#define OCC_TIMEOUT_MS		1000
> The SBEFIFO spec defines different timeout periods for different classes of
> command. One class uses a 1s timeout, whilst another uses 30s. Does the OCC
> only use commands from the former command class?

These are the long timeout, but I don't see that this value corresponds 
to the SBE timeout. If the SBE is timing out, we're done for anyway. 
This is to timeout if we see OCC "command in progress" response for too 
long.

>
>> +#define OCC_CMD_IN_PRG_WAIT_MS	50
> This is less than the 500ms reschedule period in the SBEFIFO. Does 50ms make
> sense here?

Well that reschedule in SBEFIFO is only if you have to wait for more 
data. A transfer doesn't necessarily have to wait that long. In this 
case I'd be hopeful that the op can go through all at once since it's 
just 8 bytes. Haven't done any benchmarking.

Thanks,
Eddie

>
>> +
>>   struct occ {
>>   	struct device *sbefifo;
>>   	char name[32];
>> @@ -565,6 +569,9 @@ static void occ_worker(struct work_struct *work)
>>   {
>>   	int rc = 0, empty, waiting, canceled;
>>   	u16 resp_data_length;
>> +	unsigned long start;
>> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
>> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>>   	struct occ_xfr *xfr;
>>   	struct occ_response *resp;
>>   	struct occ_client *client;
>> @@ -586,6 +593,8 @@ static void occ_worker(struct work_struct *work)
>>   	spin_unlock_irq(&occ->list_lock);
>>   	mutex_lock(&occ->occ_lock);
>>   
>> +	start = jiffies;
>> +
>>   	/* write occ command */
>>   	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>>   			 xfr->cmd_data_length);
>> @@ -597,9 +606,21 @@ static void occ_worker(struct work_struct *work)
>>   		goto done;
>>   
>>   	/* read occ response */
>> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> -	if (rc)
>> -		goto done;
>> +	do {
>> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> +		if (rc)
>> +			goto done;
>> +
>> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
>> +			rc = -EALREADY;
>> +
>> +			if (time_after(jiffies, start + timeout))
>> +				break;
>> +
>> +			set_current_state(TASK_INTERRUPTIBLE);
>> +			schedule_timeout(wait_time);
>> +		}
>> +	} while (rc);
>>   
>>   	resp_data_length = get_unaligned_be16(&resp->data_length);
>>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {