Message-ID: <0f74ed36-13bd-4b6c-9d5e-f52cc25235f8@altera.com>
Date: Wed, 28 May 2025 15:59:05 +0530
From: Mahesh Rao <mahesh.rao@...era.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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

Hi Krzysztof,
	Thanks for reviewing the code.

On 27-05-2025 02:14 pm, Krzysztof Kozlowski wrote:
> 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

Will incorporate the change.

> 
> 
>> 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

Will incorporate the change.

> 
> 
> ...
> 
>> +	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.

These functions have been newly introduced for the Stratix10-SVC 
platform driver. The client drivers that will utilize these APIs are 
currently under development and are planned for inclusion in a 
subsequent patch set. Would you prefer that I include a sample client 
driver using these APIs in this patch set instead?

> 
>> +
>> +/**
>> + * 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.

Same as above.

> 
>> +
>> +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.

Will incorporate the change.

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

Will incorporate the change.

> 
> 
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists