Re: [PATCH] firmware: stratix10-rsu: Add synchronous fallback for async SVC operations

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 26/5/2026 5:41 pm, NG, TZE YEE wrote:
> From: Mahesh Rao <mahesh.rao@altera.com>
> 
> The RSU driver was migrated to the Stratix10 asynchronous service
> framework, which assumes async client registration always succeeds. On
> platforms where the async controller is unavailable or RSU async support
> is not present, probe fails or sysfs operations cannot reach firmware.
> 
> Detect async capability at probe via stratix10_svc_add_async_client() and
> fall back to the legacy synchronous rsu_send_msg() path when registration
> fails. Track capability in priv->async and branch accordingly for:
> 
>   - Initial RSU status and retry counter during probe (retry is bundled in
>     async status; issue COMMAND_RSU_RETRY separately in sync mode)
>   - notify sysfs store (notify, status, and retry)
>   - SPT table retrieval (COMMAND_RSU_GET_SPT_TABLE async vs mbox
>     COMMAND_MBOX_SEND_CMD synchronous)
> 
> Restore synchronous callbacks (rsu_status_callback, rsu_retry_callback,
> rsu_get_spt_callback) and wire COMMAND_MBOX_SEND_CMD payload handling in
> rsu_send_msg().
> 
> Improve probe and remove cleanup: return on intermediate probe failures,
> remove the async client when registered, and free the SPT response buffer
> on sync-path allocation errors.
> 
> Suggested-by: Anders Hedlund <anders.hedlund@windriver.com>
> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
> Signed-off-by: Asyraaf Azhar <mohamad.asyraaf.azhar@altera.com>
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---
>   drivers/firmware/stratix10-rsu.c | 215 +++++++++++++++++++++++++++++--
>   1 file changed, 201 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-rsu.c b/drivers/firmware/stratix10-rsu.c
> index 6c5f952f48d8..17288e7235e6 100644
> --- a/drivers/firmware/stratix10-rsu.c
> +++ b/drivers/firmware/stratix10-rsu.c
> @@ -18,6 +18,10 @@
>   #include <linux/string.h>
>   #include <linux/sysfs.h>
>   
> +#define RSU_STATE_MASK			GENMASK_ULL(31, 0)
> +#define RSU_VERSION_MASK		GENMASK_ULL(63, 32)
> +#define RSU_ERROR_LOCATION_MASK		GENMASK_ULL(31, 0)
> +#define RSU_ERROR_DETAIL_MASK		GENMASK_ULL(63, 32)
>   /*
>    * INTEL_SIP_SMC_RSU_GET_DEVICE_INFO packs each flash word as:
>    *   [63:32] erase_size, [31:0] size (see stratix10-smc.h).
> @@ -44,6 +48,7 @@
>   
>   #define RSU_RETRY_SLEEP_MS		(1U)
>   #define RSU_ASYNC_MSG_RETRY		(3U)
> +#define RSU_GET_SPT_CMD			0x5A
>   #define RSU_GET_SPT_RESP_LEN		(4 * sizeof(unsigned int))
>   
>   struct flash_device_info {
> @@ -73,6 +78,7 @@ typedef void (*rsu_callback)(struct stratix10_svc_client *client,
>    * @client: active service client
>    * @completion: state for callback completion
>    * @lock: a mutex to protect callback completion state
> + * @async: supports async operations
>    * @status.current_image: address of image currently running in flash
>    * @status.fail_image: address of failed image in flash
>    * @status.version: the interface version number of RSU firmware
> @@ -99,6 +105,7 @@ struct stratix10_rsu_priv {
>   	struct stratix10_svc_client client;
>   	struct completion completion;
>   	struct mutex lock;
> +	bool async;
>   	struct {
>   		unsigned long current_image;
>   		unsigned long fail_image;
> @@ -129,6 +136,8 @@ struct stratix10_rsu_priv {
>   
>   	unsigned long spt0_address;
>   	unsigned long spt1_address;
> +
> +	unsigned int *get_spt_response_buf;
>   };
>   
>   /**
> @@ -148,6 +157,45 @@ static void rsu_device_info_invalidate(struct stratix10_rsu_priv *priv)
>   typedef void (*rsu_async_callback)(struct device *dev,
>   	struct stratix10_rsu_priv *priv, struct stratix10_svc_cb_data *data);
>   
> +/**
> + * rsu_status_callback() - Status callback from Intel Service Layer
> + * @client: pointer to service client
> + * @data: pointer to callback data structure
> + *
> + * Callback from Intel service layer for RSU status request. Status is
> + * only updated after a system reboot, so a get updated status call is
> + * made during driver probe.
> + */
> +static void rsu_status_callback(struct stratix10_svc_client *client,
> +				struct stratix10_svc_cb_data *data)
> +{
> +	struct stratix10_rsu_priv *priv = client->priv;
> +	struct arm_smccc_res *res = (struct arm_smccc_res *)data->kaddr1;
> +
> +	if (data->status == BIT(SVC_STATUS_OK)) {
> +		priv->status.version = FIELD_GET(RSU_VERSION_MASK,
> +						 res->a2);
> +		priv->status.state = FIELD_GET(RSU_STATE_MASK, res->a2);
> +		priv->status.fail_image = res->a1;
> +		priv->status.current_image = res->a0;
> +		priv->status.error_location =
> +			FIELD_GET(RSU_ERROR_LOCATION_MASK, res->a3);
> +		priv->status.error_details =
> +			FIELD_GET(RSU_ERROR_DETAIL_MASK, res->a3);
> +	} else {
> +		dev_err(client->dev, "COMMAND_RSU_STATUS returned 0x%lX\n",
> +			res->a0);
> +		priv->status.version = 0;
> +		priv->status.state = 0;
> +		priv->status.fail_image = 0;
> +		priv->status.current_image = 0;
> +		priv->status.error_location = 0;
> +		priv->status.error_details = 0;
> +	}
> +
> +	complete(&priv->completion);
> +}
> +
>   /**
>    * rsu_async_status_callback() - Status callback from rsu_async_send()
>    * @dev: pointer to device object
> @@ -192,6 +240,32 @@ static void rsu_command_callback(struct stratix10_svc_client *client,
>   	complete(&priv->completion);
>   }
>   
> +/**
> + * rsu_retry_callback() - Callback from Intel service layer for getting
> + * the current image's retry counter from the firmware
> + * @client: pointer to client
> + * @data: pointer to callback data structure
> + *
> + * Callback from Intel service layer for retry counter, which is used by
> + * user to know how many times the images is still allowed to reload
> + * itself before giving up and starting RSU fail-over flow.
> + */
> +static void rsu_retry_callback(struct stratix10_svc_client *client,
> +			       struct stratix10_svc_cb_data *data)
> +{
> +	struct stratix10_rsu_priv *priv = client->priv;
> +	unsigned int *counter = (unsigned int *)data->kaddr1;
> +
> +	if (data->status == BIT(SVC_STATUS_OK))
> +		priv->retry_counter = *counter;
> +	else if (data->status == BIT(SVC_STATUS_NO_SUPPORT))
> +		dev_warn(client->dev, "Secure FW doesn't support retry\n");
> +	else
> +		dev_err(client->dev, "Failed to get retry counter %lu\n",
> +			BIT(data->status));
> +
> +	complete(&priv->completion);
> +}
>   
>   /**
>    * rsu_max_retry_callback() - Callback from Intel service layer for getting
> @@ -337,6 +411,38 @@ static void rsu_async_get_spt_table_callback(struct device *dev,
>   	priv->spt1_address = *((unsigned long *)data->kaddr2);
>   }
>   
> +static void rsu_get_spt_callback(struct stratix10_svc_client *client,
> +				 struct stratix10_svc_cb_data *data)
> +{
> +	struct stratix10_rsu_priv *priv = client->priv;
> +	unsigned long *mbox_err = (unsigned long *)data->kaddr1;
> +	unsigned long *resp_len = (unsigned long *)data->kaddr2;
> +
> +	if (data->status != BIT(SVC_STATUS_OK) || (*mbox_err) ||
> +	    (*resp_len != RSU_GET_SPT_RESP_LEN))
> +		goto error;
> +
> +	priv->spt0_address = priv->get_spt_response_buf[0];
> +	priv->spt0_address <<= 32;
> +	priv->spt0_address |= priv->get_spt_response_buf[1];
> +	priv->spt1_address = priv->get_spt_response_buf[2];
> +	priv->spt1_address <<= 32;
> +	priv->spt1_address |= priv->get_spt_response_buf[3];
> +
> +	goto complete;
> +
> +error:
> +	dev_err(priv->client.dev,
> +		"failed to get SPTs (status=%#x, mbox_err=%lu, resp_len=%lu)\n",
> +		data->status, mbox_err ? *mbox_err : 0,
> +		resp_len ? *resp_len : 0);
> +
> +complete:
> +	stratix10_svc_free_memory(priv->chan, priv->get_spt_response_buf);
> +	priv->get_spt_response_buf = NULL;
> +	complete(&priv->completion);
> +}
> +
>   /**
>    * rsu_send_msg() - send a message to Intel service layer
>    * @priv: pointer to rsu private data
> @@ -355,7 +461,7 @@ static int rsu_send_msg(struct stratix10_rsu_priv *priv,
>   			unsigned long arg,
>   			rsu_callback callback)
>   {
> -	struct stratix10_svc_client_msg msg;
> +	struct stratix10_svc_client_msg msg = {0};
>   	int ret;
>   
>   	mutex_lock(&priv->lock);
> @@ -366,6 +472,14 @@ static int rsu_send_msg(struct stratix10_rsu_priv *priv,
>   	if (arg)
>   		msg.arg[0] = arg;
>   
> +	if (command == COMMAND_MBOX_SEND_CMD) {
> +		msg.arg[1] = 0;
> +		msg.payload = NULL;
> +		msg.payload_length = 0;
> +		msg.payload_output = priv->get_spt_response_buf;
> +		msg.payload_length_output = RSU_GET_SPT_RESP_LEN;
> +	}
> +
>   	ret = stratix10_svc_send(priv->chan, &msg);
>   	if (ret < 0)
>   		goto status_done;
> @@ -437,6 +551,8 @@ static int rsu_send_async_msg(struct device *dev, struct stratix10_rsu_priv *pri
>   
>   	if (status && !handle) {
>   		dev_err(dev, "Failed to send async message\n");
> +		if (msg.payload_output)
> +			stratix10_svc_free_memory(priv->chan, msg.payload_output);
>   		return -ETIMEDOUT;
>   	}
>   
> @@ -476,6 +592,8 @@ static int rsu_send_async_msg(struct device *dev, struct stratix10_rsu_priv *pri
>   	}
>   
>   status_done:
> +	if (msg.payload_output)
> +		stratix10_svc_free_memory(priv->chan, msg.payload_output);
>   	stratix10_svc_async_done(priv->chan, handle);
>   	return ret;
>   }
> @@ -714,15 +832,30 @@ static ssize_t notify_store(struct device *dev,
>   	if (ret)
>   		return ret;
>   
> -	ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_NOTIFY, status, NULL);
> +	if (priv->async)
> +		ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_NOTIFY, status, NULL);
> +	else
> +		ret = rsu_send_msg(priv, COMMAND_RSU_NOTIFY, status, rsu_command_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, RSU notify returned %i\n", ret);
>   		return ret;
>   	}
>   
>   	/* to get the updated state */
> -	ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_STATUS, 0,
> -				 rsu_async_status_callback);
> +	if (priv->async) {
> +		ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_STATUS, 0,
> +					 rsu_async_status_callback);
> +	} else {
> +		ret = rsu_send_msg(priv, COMMAND_RSU_STATUS, 0,
> +				   rsu_status_callback);
> +		/*
> +		 * In async mode COMMAND_RSU_RETRY is part of COMMAND_RSU_STATUS;
> +		 * issue it separately only in synchronous mode.
> +		 */
> +		if (!ret)
> +			ret = rsu_send_msg(priv, COMMAND_RSU_RETRY, 0,
> +					   rsu_retry_callback);
> +	}
>   	if (ret) {
>   		dev_err(dev, "Error, getting RSU status %i\n", ret);
>   		return ret;
> @@ -922,6 +1055,8 @@ static int stratix10_rsu_probe(struct platform_device *pdev)
>   	priv->max_retry = INVALID_RETRY_COUNTER;
>   	priv->spt0_address = INVALID_SPT_ADDRESS;
>   	priv->spt1_address = INVALID_SPT_ADDRESS;
> +	priv->get_spt_response_buf = NULL;
> +	priv->async = false;
>   	rsu_device_info_invalidate(priv);
>   
>   	mutex_init(&priv->lock);
> @@ -934,45 +1069,73 @@ static int stratix10_rsu_probe(struct platform_device *pdev)
>   	}
>   
>   	ret = stratix10_svc_add_async_client(priv->chan, false);
> -	if (ret) {
> -		dev_err(dev, "failed to add async client\n");
> -		stratix10_svc_free_channel(priv->chan);
> -		return ret;
> +	if (ret < 0) {
> +		dev_dbg(dev, "Async operations not supported, fallback to non-async mode\n");
> +		priv->async = false;
> +	} else {
> +		priv->async = true;
>   	}
>   
>   	init_completion(&priv->completion);
>   	platform_set_drvdata(pdev, priv);
>   
>   	/* get the initial state from firmware */
> -	ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_STATUS, 0,
> -				 rsu_async_status_callback);
> +	if (priv->async)
> +		ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_STATUS, 0,
> +					 rsu_async_status_callback);
> +	else
> +		ret = rsu_send_msg(priv, COMMAND_RSU_STATUS, 0,
> +				   rsu_status_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, getting RSU status %i\n", ret);
> -		stratix10_svc_remove_async_client(priv->chan);
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
>   		stratix10_svc_free_channel(priv->chan);
>   		return ret;
>   	}
>   
> +	/*
> +	 * In async mode COMMAND_RSU_RETRY is part of COMMAND_RSU_STATUS;
> +	 * issue it separately only in synchronous mode.
> +	 */
> +	if (!priv->async) {
> +		ret = rsu_send_msg(priv, COMMAND_RSU_RETRY, 0, rsu_retry_callback);
> +		if (ret) {
> +			dev_err(dev, "Error, getting RSU retry %i\n", ret);
> +			stratix10_svc_free_channel(priv->chan);
> +			return ret;
> +		}
> +	}
> +
>   	/* get DCMF version from firmware */
>   	ret = rsu_send_msg(priv, COMMAND_RSU_DCMF_VERSION,
>   			   0, rsu_dcmf_version_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, getting DCMF version %i\n", ret);
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
>   		stratix10_svc_free_channel(priv->chan);
> +		return ret;
>   	}
>   
>   	ret = rsu_send_msg(priv, COMMAND_RSU_DCMF_STATUS,
>   			   0, rsu_dcmf_status_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, getting DCMF status %i\n", ret);
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
>   		stratix10_svc_free_channel(priv->chan);
> +		return ret;
>   	}
>   
>   	ret = rsu_send_msg(priv, COMMAND_RSU_MAX_RETRY, 0,
>   			   rsu_max_retry_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, getting RSU max retry %i\n", ret);
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
>   		stratix10_svc_free_channel(priv->chan);
> +		return ret;
>   	}
>   
>   	/* get QSPI device info from firmware */
> @@ -980,15 +1143,36 @@ static int stratix10_rsu_probe(struct platform_device *pdev)
>   			   rsu_get_device_info_callback);
>   	if (ret) {
>   		dev_err(dev, "Error, getting QSPI Device Info %i\n", ret);
> -		stratix10_svc_remove_async_client(priv->chan);
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
>   		stratix10_svc_free_channel(priv->chan);
>   		return ret;
>   	}
>   
> -	ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_GET_SPT_TABLE, 0,
> -				 rsu_async_get_spt_table_callback);
> +	if (priv->async) {
> +		ret = rsu_send_async_msg(dev, priv, COMMAND_RSU_GET_SPT_TABLE,
> +					 0, rsu_async_get_spt_table_callback);
> +	} else {
> +		priv->get_spt_response_buf =
> +			stratix10_svc_allocate_memory(priv->chan, RSU_GET_SPT_RESP_LEN);
> +		if (IS_ERR(priv->get_spt_response_buf)) {
> +			ret = PTR_ERR(priv->get_spt_response_buf);
> +			priv->get_spt_response_buf = NULL;
> +			dev_err(dev, "failed to allocate get spt buffer\n");
> +		} else {
> +			ret = rsu_send_msg(priv, COMMAND_MBOX_SEND_CMD,
> +					   RSU_GET_SPT_CMD, rsu_get_spt_callback);
> +		}
> +	}
>   	if (ret) {
>   		dev_err(dev, "Error, getting SPT table %i\n", ret);
> +		if (priv->async) {
> +			stratix10_svc_remove_async_client(priv->chan);
> +		} else if (!IS_ERR_OR_NULL(priv->get_spt_response_buf)) {
> +			stratix10_svc_free_memory(priv->chan,
> +						  priv->get_spt_response_buf);
> +			priv->get_spt_response_buf = NULL;
> +		}
>   		stratix10_svc_free_channel(priv->chan);
>   	}
>   
> @@ -999,6 +1183,9 @@ static void stratix10_rsu_remove(struct platform_device *pdev)
>   {
>   	struct stratix10_rsu_priv *priv = platform_get_drvdata(pdev);
>   
> +	if (priv->async)
> +		stratix10_svc_remove_async_client(priv->chan);
> +
>   	stratix10_svc_free_channel(priv->chan);
>   }
>   

Hi,

Gentle ping on this patch — any chance for a review when you have time?
Happy to rebase or revise based on your feedback.

Thanks,
Tze Yee