Re: [PATCH 3/5] firmware: stratix10-svc: add async HWMON read commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of async client on driver removal causes memory leak and breaks subsequent probes.
- [High] Abandoning async transactions via `stratix10_svc_async_done()` without notifying EL3 firmware causes transaction ID desynchronization.
--

commit 6e40e51555b4f357bf75d356699813e67712ecca
Author: Tze Yee Ng <tze.yee.ng@altera.com>

firmware: stratix10-svc: add async HWMON read commands

This commit adds asynchronous Stratix 10 service layer support for hardware
monitor temperature and voltage read commands. It updates the service
layer functions stratix10_svc_async_send() and
stratix10_svc_async_prepare_response() to process the new messages.

> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index 39eb78f5905b3..ca2e354fc20fe 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1317,6 +1317,14 @@ int stratix10_svc_async_send(struct stratix10_svc_chan *chan, void *msg,
>  		args.a0 = INTEL_SIP_SMC_ASYNC_RSU_NOTIFY;
>  		args.a2 = p_msg->arg[0];
>  		break;
> +	case COMMAND_HWMON_READTEMP:
> +		args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READTEMP;
> +		args.a2 = p_msg->arg[0];
> +		break;
> +	case COMMAND_HWMON_READVOLT:
> +		args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READVOLT;
> +		args.a2 = p_msg->arg[0];
> +		break;
>  	default:

[Severity: High]
This is a pre-existing issue, but does abandoning asynchronous transactions
via stratix10_svc_async_done() without notifying the EL3 firmware cause a
transaction ID desynchronization?

When an async transaction times out in stratix10_hwmon_async_read() or
rsu_send_async_msg(), the driver abandons the transaction and frees the ID:

drivers/firmware/stratix10-svc.c:stratix10_svc_async_done() {
    ...
	ida_free(&achan->job_id_pool,
		 STRATIX10_GET_JOBID(handle->transaction_id));
	kfree(handle);
}

Since there is no SMC cancellation command sent to the firmware, the firmware
might still process the timed-out request. If a new command allocates the
same recycled transaction ID, polling the firmware could return the delayed
completion of the original stale transaction.

Will this result in desynchronization between the driver and secure firmware,
potentially returning stale hwmon sensor readings or incorrect statuses?


[Severity: High]
This is a pre-existing issue, but does the missing cleanup of the async
client on driver removal cause a memory leak and break subsequent probes?

Looking at stratix10_rsu_remove() in drivers/firmware/stratix10-rsu.c:

drivers/firmware/stratix10-rsu.c:stratix10_rsu_remove() {
	struct stratix10_rsu_priv *priv = platform_get_drvdata(pdev);

	stratix10_svc_free_channel(priv->chan);
}

It calls stratix10_svc_free_channel() but completely omits calling
stratix10_svc_remove_async_client(). This leaves chan->async_chan
populated on the shared channel array and leaks the asynchronous channel
object and its associated IDA pool.

If the module is unloaded and reloaded, will the next call to
stratix10_svc_add_async_client() inside stratix10_rsu_probe() encounter
a non-NULL async_chan and immediately return -EALREADY?

While the new hwmon driver introduced in this patchset appears to handle
this correctly, it seems the rsu driver remains permanently unable to
probe after a single removal until the system is rebooted.

>  		dev_err(ctrl->dev, "Invalid command ,%d\n", p_msg->command);
>  		ret = -EINVAL;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781861409.git.tze.yee.ng@altera.com?part=3