Re: [PATCH] firmware: stratix10-svc: support up to 4 buffer claims and extend timeout

On 2/10/26 01:16, [email protected] wrote:
> From: Adrian Ng Ho Yin <[email protected]>
> 
> The service layer previously only returned up to three buffer addresses
> per transaction. Extend the logic in svc_thread_cmd_data_claim() to
> collect up to four buffer claims. A new field `kaddr4` is added to
> struct stratix10_svc_cb_data, and the FPGA manager callback unlocks this
> fourth buffer accordingly.
> 
> Timeout values for reconfiguration and buffer transactions are also
> increased (from ~300–720ms to 5000ms), since real-world processing takes
> significantly longer (~600ms or more). The reconfiguration complete
> timeout is also replaced with S10_RECONFIG_TIMEOUT (>1s) to avoid
> premature aborts.
> 
> Additional changes:
> - Callbacks are updated to pass all claimed buffers to clients.
> - Debug logging is added to trace received status values.
> - Simplified buffer wait logic in s10_ops_write() by always using
>    wait_for_completion_timeout().
> 
> These changes improve robustness of FPGA configuration on SoCFPGA
> platforms by handling larger transactions and accommodating realistic
> timing.
> 
> Signed-off-by: Ang Tien Sung <[email protected]>
> Signed-off-by: Adrian Ng Ho Yin <[email protected]>
> ---
>   drivers/firmware/stratix10-svc.c              | 36 +++++++++++++------
>   drivers/fpga/stratix10-soc.c                  | 18 +++++-----
>   .../firmware/intel/stratix10-svc-client.h     |  6 ++--
>   3 files changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index dbed404a71fc..58cf68d9a384 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -342,6 +342,8 @@ static void svc_thread_cmd_data_claim(struct stratix10_svc_controller *ctrl,
>   {
>   	struct arm_smccc_res res;
>   	unsigned long timeout;
> +	void *buf_claim_addr[4] = {NULL};
> +	int buf_claim_count = 0;
>   
>   	reinit_completion(&ctrl->complete_status);
>   	timeout = msecs_to_jiffies(FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS);
> @@ -353,20 +355,32 @@ static void svc_thread_cmd_data_claim(struct stratix10_svc_controller *ctrl,
>   
>   		if (res.a0 == INTEL_SIP_SMC_STATUS_OK) {
>   			if (!res.a1) {
> +				/* Transaction of 4 blocks are now done */

This comment is a bit misleading. How do you know you finish exactly 4 
blocks?

>   				complete(&ctrl->complete_status);
> +				cb_data->status = BIT(SVC_STATUS_BUFFER_DONE);
> +				cb_data->kaddr1 = buf_claim_addr[0];
> +				cb_data->kaddr2 = buf_claim_addr[1];
> +				cb_data->kaddr3 = buf_claim_addr[2];
> +				cb_data->kaddr4 = buf_claim_addr[3];
> +				p_data->chan->scl->receive_cb(p_data->chan->scl,
> +				cb_data);
>   				break;
>   			}
> -			cb_data->status = BIT(SVC_STATUS_BUFFER_DONE);
> -			cb_data->kaddr1 = svc_pa_to_va(res.a1);
> -			cb_data->kaddr2 = (res.a2) ?
> -					  svc_pa_to_va(res.a2) : NULL;
> -			cb_data->kaddr3 = (res.a3) ?
> -					  svc_pa_to_va(res.a3) : NULL;
> -			p_data->chan->scl->receive_cb(p_data->chan->scl,
> -						      cb_data);

The callback used to be only dependent on INTEL_SIP_SMC_STATUS_OK, but 
this change put the dependency on res.a1 == 0 as well. Please add 
comment or add to the commit message the reason for this change.

> -		} else {
> -			pr_debug("%s: secure world busy, polling again\n",
> -				 __func__);
> +
> +			if (buf_claim_count < 4) {
> +				buf_claim_addr[buf_claim_count] = svc_pa_to_va(res.a1);
> +				buf_claim_count++;
> +			}
> +
> +			if (res.a2 && buf_claim_count < 4) {
> +				buf_claim_addr[buf_claim_count] = svc_pa_to_va(res.a2);
> +				buf_claim_count++;
> +			}
> +			if (res.a3 && buf_claim_count < 4) {
> +				buf_claim_addr[buf_claim_count] = svc_pa_to_va(res.a3);
> +				buf_claim_count++;
> +			}

Could there be a case where buf_claim_count >= 4, if so you should add a 
WARN_ON.

> +
>   		}
>   	} while (res.a0 == INTEL_SIP_SMC_STATUS_OK ||
>   		 res.a0 == INTEL_SIP_SMC_STATUS_BUSY ||
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 0a295ccf1644..4fea9458f92b 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -147,6 +147,7 @@ static void s10_receive_callback(struct stratix10_svc_client *client,
>   	u32 status;
>   	int i;
>   
> +	pr_debug("%s data %x\n", __func__, data->status);

Use %#x would be more informative for a status bitmask.

>   	WARN_ONCE(!data, "%s: stratix10_svc_rc_data = NULL", __func__);
>   
>   	status = data->status;
> @@ -163,6 +164,7 @@ static void s10_receive_callback(struct stratix10_svc_client *client,
>   		s10_unlock_bufs(priv, data->kaddr1);
>   		s10_unlock_bufs(priv, data->kaddr2);
>   		s10_unlock_bufs(priv, data->kaddr3);
> +		s10_unlock_bufs(priv, data->kaddr4);
>   	}
>   
>   	complete(&priv->status_return_completion);
> @@ -309,15 +311,8 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
>   				break;
>   		}
>   
> -		/*
> -		 * If callback hasn't already happened, wait for buffers to be
> -		 * returned from service layer
> -		 */
> -		wait_status = 1; /* not timed out */
> -		if (!priv->status)
> -			wait_status = wait_for_completion_timeout(
> -				&priv->status_return_completion,
> -				S10_BUFFER_TIMEOUT);
> +		wait_status = wait_for_completion_timeout(&priv->status_return_completion,
> +							  S10_BUFFER_TIMEOUT);

Can you add a comment why you remove the check for !priv->status to call 
wait_for_completion_timeout() ?

>   
>   		if (test_and_clear_bit(SVC_STATUS_BUFFER_DONE, &priv->status) ||
>   		    test_and_clear_bit(SVC_STATUS_BUFFER_SUBMITTED,
> @@ -353,7 +348,10 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
>   	unsigned long timeout;
>   	int ret;
>   
> -	timeout = usecs_to_jiffies(info->config_complete_timeout_us);
> +	/* The time taken to process this is close to 600ms

nit: comment style is wrong

> +	 * This MUST be increased over 1 second
> +	 */
> +	timeout = S10_RECONFIG_TIMEOUT;
>   
>   	do {
>   		reinit_completion(&priv->status_return_completion);
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index d290060f4c73..abe04f4fad1d 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -68,8 +68,8 @@
>    * timeout value used in Stratix10 FPGA manager driver.
>    * timeout value used in RSU driver
>    */
> -#define SVC_RECONFIG_REQUEST_TIMEOUT_MS         300
> -#define SVC_RECONFIG_BUFFER_TIMEOUT_MS          720
> +#define SVC_RECONFIG_REQUEST_TIMEOUT_MS         5000
> +#define SVC_RECONFIG_BUFFER_TIMEOUT_MS          5000

These are huge jumps in timeout values. You may be masking real firmware 
hangs. Please document why such a large change.

Dinh