All of lore.kernel.org

* Re: [PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"
From: Andrew Jeffery @ 2017-10-05  1:01 UTC (permalink / raw)
  To: Eddie James, openbmc; +Cc: joel, Edward A. James
In-Reply-To: <1506724868-13010-6-git-send-email-eajames@linux.vnet.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@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@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?

> +#define OCC_CMD_IN_PRG_WAIT_MS	50

This is less than the 500ms reschedule period in the SBEFIFO. Does 50ms make
sense here?

> +
>  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) {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply