Re: [PATCH v3 3/4] firmware: stratix10-svc: Add initial support for asynchronous communication with Stratix10 service channel

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/05/2025 08:25, Mahesh Rao via B4 Relay wrote:
> From: Mahesh Rao <mahesh.rao@altera.com>
> 
> This commit adds support for asynchronous communication

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


> with the Stratix10 service channel. It introduces
> new definitions to enable asynchronous messaging to
> the Secure Device Manager (SDM). The changes include
> the adding/removing of asynchronous support to existing
> channels, initializing/exit-cleanup of the new asynchronous
> framework and sending/polling of messages to SDM.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597


...

> +	args.a0 = INTEL_SIP_SMC_ASYNC_POLL;
> +	args.a1 =
> +		STRATIX10_SIP_SMC_SET_TRANSACTIONID_X1(handle->transaction_id);
> +
> +	actrl->invoke_fn(actrl, &args, &handle->res);
> +
> +	data->status = 0;
> +	if (handle->res.a0 == INTEL_SIP_SMC_STATUS_OK) {
> +		return 0;
> +	} else if (handle->res.a0 == INTEL_SIP_SMC_STATUS_BUSY) {
> +		dev_dbg(ctrl->dev, "async message is still in progress\n");
> +		return -EAGAIN;
> +	}
> +
> +	dev_err(ctrl->dev,
> +		"Failed to poll async message ,got status as %ld\n",
> +		handle->res.a0);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(stratix10_svc_async_poll);

No, drop entire function. There is no user of it. You cannot add exports
for dead code.

> +
> +/**
> + * stratix10_svc_async_done - Completes an asynchronous transaction.
> + * @chan: Pointer to the service channel structure.
> + * @tx_handle: Handle to the transaction being completed.
> + *
> + * This function completes an asynchronous transaction identified by the given
> + * transaction handle. It ensures that the necessary structures are initialized
> + * and valid before proceeding with the completion operation. The function
> + * deallocates the transaction ID, frees the memory allocated for the handler,
> + * and removes the handler from the transaction list.
> + *
> + * Return: 0 on success, -EINVAL if any input parameter is invalid, or other
> + *         negative error codes on failure.
> + */
> +int stratix10_svc_async_done(struct stratix10_svc_chan *chan, void *tx_handle)
> +{
> +	if (!chan || !tx_handle)
> +		return -EINVAL;
> +
> +	struct stratix10_svc_controller *ctrl = chan->ctrl;
> +	struct stratix10_async_chan *achan = chan->async_chan;
> +
> +	if (!achan) {
> +		dev_err(ctrl->dev, "async channel not allocated\n");
> +		return -EINVAL;
> +	}
> +
> +	struct stratix10_svc_async_handler *handle =
> +		(struct stratix10_svc_async_handler *)tx_handle;
> +	if (!hash_hashed(&handle->next)) {
> +		dev_err(ctrl->dev, "Invalid transaction handle\n");
> +		return -EINVAL;
> +	}
> +
> +	struct stratix10_async_ctrl *actrl = &ctrl->actrl;
> +
> +	spin_lock(&actrl->trx_list_wr_lock);
> +	hash_del_rcu(&handle->next);
> +	spin_unlock(&actrl->trx_list_wr_lock);
> +	synchronize_rcu();
> +	stratix10_deallocate_id(achan->job_id_pool,
> +				STRATIX10_GET_JOBID(handle->transaction_id));
> +	kfree(handle);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(stratix10_svc_async_done);

No, drop entire function. There is no user of it. You cannot add exports
for dead code.

> +
> +static inline void stratix10_smc_1_2(struct stratix10_async_ctrl *actrl,
> +				     const struct arm_smccc_1_2_regs *args,
> +				     struct arm_smccc_1_2_regs *res)
> +{
> +	arm_smccc_1_2_smc(args, res);
> +}
> +
> +/**
> + * stratix10_svc_async_init - Initialize the Stratix10 service controller
> + *                            for asynchronous operations.
> + * @controller: Pointer to the Stratix10 service controller structure.
> + *
> + * This function initializes the asynchronous service controller by setting up
> + * the necessary data structures, initializing the transaction list.
> + *
> + * Return: 0 on success, -EINVAL if the controller is NULL or already initialized,
> + *         -ENOMEM if memory allocation fails, -EADDRINUSE if the client ID is already
> + *         reserved, or other negative error codes on failure.
> + */
> +static int stratix10_svc_async_init(struct stratix10_svc_controller *controller)
> +{
> +	int ret;
> +	struct arm_smccc_res res;
> +
> +	if (!controller)
> +		return -EINVAL;
> +
> +	struct stratix10_async_ctrl *actrl = &controller->actrl;

Do not declare variables in the middle of the code. See coding style.

> +
> +	if (actrl->initialized)
> +		return -EINVAL;
> +
> +	struct device *dev = controller->dev;

Same here.



Best regards,
Krzysztof