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

[Andrew Jeffery]

On Thu, 2017-10-05 at 10:38 -0500, Eddie James wrote:
> 
> 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.

It could if the OCC wasn't responding with "command in progress", as
the SBE driver wouldn't be able to tell the difference. Don't know
whether that's possible though.

>  If the SBE is timing out, we're done for anyway. 

Sure.

> This is to timeout if we see OCC "command in progress" response for too 
> long.

Right.

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

Okay, thanks for clearing that up.

> 
> 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) {
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171006/ffe9083c/attachment.sig>