Re: [PATCH v8 2/6] firmware: meson: sm: Add video firmware loading SMC call - sashiko-bot

From: sashiko-bot@kernel.org
To: "Zhentao Guo" <zhentao.guo@amlogic.com>
Cc: conor+dt@kernel.org, robh@kernel.org, neil.armstrong@linaro.org,
	devicetree@vger.kernel.org, media-ci@linuxtv.org,
	linux-amlogic@lists.infradead.org
Subject: Re: [PATCH v8 2/6] firmware: meson: sm: Add video firmware loading SMC call
Date: Mon, 31 Aug 2026 08:45:37 +0000	[thread overview]
Message-ID: <20260831084537.9AF4E1F000E9@smtp.kernel.org> (raw)
In-Reply-To: <20260831-b4-s4-vdec-upstream-v8-2-830195b9bd91@amlogic.com>

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

Pre-existing issues:
- [High] meson_sm_call() unconditionally returns 0 on successful command dispatch, masking SMCCC errors from functions that rely on its return value instead of checking the output parameter.
--

commit 9dde1c44d8065dbf71f452dee2694d932fa3d6ae
Author: Zhentao Guo <zhentao.guo@amlogic.com>

firmware: meson: sm: Add video firmware loading SMC call

This commit adds the SM_LOAD_VIDEO_FW command to the gxbb_chip command
table with SMC ID 0xb200000f for loading video firmware.

> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index ab9751a59b552..887caa5c625db 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -48,6 +48,7 @@ static const struct meson_sm_chip gxbb_chip = {
>  		CMD(SM_THERMAL_CALIB_READ,	0x82000047),
>  		CMD(SM_A1_PWRC_SET,		0x82000093),
>  		CMD(SM_A1_PWRC_GET,		0x82000095),
> +		CMD(SM_LOAD_VIDEO_FW,		0xb200000f),

[Severity: High]
This is a pre-existing issue, but does meson_sm_call() handle SMCCC
errors correctly?

Looking at meson_sm_call(), it unconditionally returns 0 after dispatching
the command:

drivers/firmware/meson/meson_sm.c:meson_sm_call() {
    ...
	lret = __meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4);

	if (ret)
		*ret = lret;

	return 0;
}

This masks SMCCC errors from callers that rely on its return value instead
of checking the output parameter. For example, the power domain driver
(drivers/pmdomain/amlogic/meson-secure-pwrc.c) passes a NULL pointer for
the output parameter and relies on the return value of meson_sm_call()
for error checking. This causes it to assume power operations always
succeed, which could lead to a synchronous external abort (kernel panic)
if hardware within a failed power domain is accessed.

Similarly, meson_sm_get_thermal_calib() passes a pointer to receive
calibration data, but if an error occurs, the SMC error code is directly
written into this data, and the function incorrectly returns 0 (success):

drivers/firmware/meson/meson_sm.c:meson_sm_get_thermal_calib() {
    ...
	return meson_sm_call(fw, SM_THERMAL_CALIB_READ, trim_info, tsensor_id,
			     0, 0, 0, 0);
}

Should meson_sm_call() propagate the error code in lret instead of always
returning 0?

>  		{ /* sentinel */ },
>  	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-b4-s4-vdec-upstream-v8-0-830195b9bd91@amlogic.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic