[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3a76c7b1-ce02-41eb-a4c0-ae065e9b99f3@kernel.org>
Date: Tue, 27 May 2025 10:44:31 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: mahesh.rao@...era.com, Dinh Nguyen <dinguyen@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: Matthew Gerlach <matthew.gerlach@...era.com>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 3/4] firmware: stratix10-svc: Add initial support for
asynchronous communication with Stratix10 service channel
On 26/05/2025 08:25, Mahesh Rao via B4 Relay wrote:
> From: Mahesh Rao <mahesh.rao@...era.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
Powered by blists - more mailing lists