* Re: [PATCH] media: s5p-mfc: Always pass NULL to s5p_mfc_cmd_host2risc_v6()
[not found] ` <20250715-media-s5p-mfc-fix-uninit-const-pointer-v1-1-4d52b58cafe9@kernel.org>
@ 2025-07-29 14:24 ` Nicolas Dufresne
2025-07-30 0:52 ` Nathan Chancellor
2025-07-30 7:04 ` Marek Szyprowski
1 sibling, 1 reply; 5+ messages in thread
From: Nicolas Dufresne @ 2025-07-29 14:24 UTC (permalink / raw)
To: Nathan Chancellor, Marek Szyprowski, Andrzej Hajda,
Mauro Carvalho Chehab
Cc: linux-arm-kernel, linux-media, llvm, patches, stable
[-- Attachment #1: Type: text/plain, Size: 5903 bytes --]
Hi Nathan,
Le mardi 15 juillet 2025 à 15:13 -0700, Nathan Chancellor a écrit :
> A new warning in clang [1] points out a few places in s5p_mfc_cmd_v6.c
> where an uninitialized variable is passed as a const pointer:
>
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:45:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 45 | &h2r_args);
> | ^~~~~~~~
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:133:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 133 | &h2r_args);
> | ^~~~~~~~
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:148:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 148 | &h2r_args);
> | ^~~~~~~~
>
> The args parameter in s5p_mfc_cmd_host2risc_v6() is never actually used,
> so just pass NULL to it in the places where h2r_args is currently
> passed, clearing up the warning and not changing the functionality of
> the code.
>
> Cc: stable@vger.kernel.org
> Fixes: f96f3cfa0bb8 ("[media] s5p-mfc: Update MFC v4l2 driver to support MFC6.x")
> Link: https://github.com/llvm/llvm-project/commit/00dacf8c22f065cb52efb14cd091d441f19b319e [1]
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2103
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> From what I can tell, it seems like ->cmd_host2risc() is only ever
> called from v6 code, which always passes NULL? It seems like it should
> be possible to just drop .cmd_host2risc on the v5 side, then update
> .cmd_host2risc to only take two parameters? If so, I can send a follow
> up as a clean up, so that this can go back relatively conflict free.
It seems so yes. For this specific patch, I would probably rename "args" to
"__unused" to make the reading faster. But does not matter so much if you later
remove it.
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> .../platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c | 22 +++++-----------------
> 1 file changed, 5 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> index 47bc3014b5d8..735471c50dbb 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> @@ -31,7 +31,6 @@ static int s5p_mfc_cmd_host2risc_v6(struct s5p_mfc_dev *dev, int cmd,
>
> static int s5p_mfc_sys_init_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> const struct s5p_mfc_buf_size_v6 *buf_size = dev->variant->buf_size->priv;
> int ret;
>
> @@ -41,33 +40,23 @@ static int s5p_mfc_sys_init_cmd_v6(struct s5p_mfc_dev *dev)
>
> mfc_write(dev, dev->ctx_buf.dma, S5P_FIMV_CONTEXT_MEM_ADDR_V6);
> mfc_write(dev, buf_size->dev_ctx, S5P_FIMV_CONTEXT_MEM_SIZE_V6);
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SYS_INIT_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SYS_INIT_V6, NULL);
> }
>
> static int s5p_mfc_sleep_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> -
> - memset(&h2r_args, 0, sizeof(struct s5p_mfc_cmd_args));
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SLEEP_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SLEEP_V6, NULL);
> }
>
> static int s5p_mfc_wakeup_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> -
> - memset(&h2r_args, 0, sizeof(struct s5p_mfc_cmd_args));
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_WAKEUP_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_WAKEUP_V6, NULL);
> }
>
> /* Open a new instance and get its number */
> static int s5p_mfc_open_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> {
> struct s5p_mfc_dev *dev = ctx->dev;
> - struct s5p_mfc_cmd_args h2r_args;
> int codec_type;
>
> mfc_debug(2, "Requested codec mode: %d\n", ctx->codec_mode);
> @@ -130,14 +119,13 @@ static int s5p_mfc_open_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> mfc_write(dev, 0, S5P_FIMV_D_CRC_CTRL_V6); /* no crc */
>
> return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_OPEN_INSTANCE_V6,
> - &h2r_args);
> + NULL);
> }
>
> /* Close instance */
> static int s5p_mfc_close_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> {
> struct s5p_mfc_dev *dev = ctx->dev;
> - struct s5p_mfc_cmd_args h2r_args;
> int ret = 0;
>
> dev->curr_ctx = ctx->num;
> @@ -145,7 +133,7 @@ static int s5p_mfc_close_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> mfc_write(dev, ctx->inst_no, S5P_FIMV_INSTANCE_ID_V6);
> ret = s5p_mfc_cmd_host2risc_v6(dev,
> S5P_FIMV_H2R_CMD_CLOSE_INSTANCE_V6,
> - &h2r_args);
> + NULL);
> } else {
> ret = -EINVAL;
> }
>
> ---
> base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
> change-id: 20250715-media-s5p-mfc-fix-uninit-const-pointer-cbf944ae4b4b
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] media: s5p-mfc: Always pass NULL to s5p_mfc_cmd_host2risc_v6()
[not found] ` <20250715-media-s5p-mfc-fix-uninit-const-pointer-v1-1-4d52b58cafe9@kernel.org>
2025-07-29 14:24 ` [PATCH] media: s5p-mfc: Always pass NULL to s5p_mfc_cmd_host2risc_v6() Nicolas Dufresne
@ 2025-07-30 7:04 ` Marek Szyprowski
1 sibling, 0 replies; 5+ messages in thread
From: Marek Szyprowski @ 2025-07-30 7:04 UTC (permalink / raw)
To: Nathan Chancellor, Andrzej Hajda, Mauro Carvalho Chehab
Cc: linux-arm-kernel, linux-media, llvm, patches, stable
On 16.07.2025 00:13, Nathan Chancellor wrote:
> A new warning in clang [1] points out a few places in s5p_mfc_cmd_v6.c
> where an uninitialized variable is passed as a const pointer:
>
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:45:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 45 | &h2r_args);
> | ^~~~~~~~
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:133:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 133 | &h2r_args);
> | ^~~~~~~~
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c:148:7: error: variable 'h2r_args' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
> 148 | &h2r_args);
> | ^~~~~~~~
>
> The args parameter in s5p_mfc_cmd_host2risc_v6() is never actually used,
> so just pass NULL to it in the places where h2r_args is currently
> passed, clearing up the warning and not changing the functionality of
> the code.
>
> Cc: stable@vger.kernel.org
> Fixes: f96f3cfa0bb8 ("[media] s5p-mfc: Update MFC v4l2 driver to support MFC6.x")
> Link: https://github.com/llvm/llvm-project/commit/00dacf8c22f065cb52efb14cd091d441f19b319e [1]
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2103
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> From what I can tell, it seems like ->cmd_host2risc() is only ever
> called from v6 code, which always passes NULL? It seems like it should
> be possible to just drop .cmd_host2risc on the v5 side, then update
> .cmd_host2risc to only take two parameters? If so, I can send a follow
> up as a clean up, so that this can go back relatively conflict free.
> ---
> .../platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c | 22 +++++-----------------
> 1 file changed, 5 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> index 47bc3014b5d8..735471c50dbb 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_cmd_v6.c
> @@ -31,7 +31,6 @@ static int s5p_mfc_cmd_host2risc_v6(struct s5p_mfc_dev *dev, int cmd,
>
> static int s5p_mfc_sys_init_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> const struct s5p_mfc_buf_size_v6 *buf_size = dev->variant->buf_size->priv;
> int ret;
>
> @@ -41,33 +40,23 @@ static int s5p_mfc_sys_init_cmd_v6(struct s5p_mfc_dev *dev)
>
> mfc_write(dev, dev->ctx_buf.dma, S5P_FIMV_CONTEXT_MEM_ADDR_V6);
> mfc_write(dev, buf_size->dev_ctx, S5P_FIMV_CONTEXT_MEM_SIZE_V6);
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SYS_INIT_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SYS_INIT_V6, NULL);
> }
>
> static int s5p_mfc_sleep_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> -
> - memset(&h2r_args, 0, sizeof(struct s5p_mfc_cmd_args));
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SLEEP_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_SLEEP_V6, NULL);
> }
>
> static int s5p_mfc_wakeup_cmd_v6(struct s5p_mfc_dev *dev)
> {
> - struct s5p_mfc_cmd_args h2r_args;
> -
> - memset(&h2r_args, 0, sizeof(struct s5p_mfc_cmd_args));
> - return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_WAKEUP_V6,
> - &h2r_args);
> + return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_WAKEUP_V6, NULL);
> }
>
> /* Open a new instance and get its number */
> static int s5p_mfc_open_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> {
> struct s5p_mfc_dev *dev = ctx->dev;
> - struct s5p_mfc_cmd_args h2r_args;
> int codec_type;
>
> mfc_debug(2, "Requested codec mode: %d\n", ctx->codec_mode);
> @@ -130,14 +119,13 @@ static int s5p_mfc_open_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> mfc_write(dev, 0, S5P_FIMV_D_CRC_CTRL_V6); /* no crc */
>
> return s5p_mfc_cmd_host2risc_v6(dev, S5P_FIMV_H2R_CMD_OPEN_INSTANCE_V6,
> - &h2r_args);
> + NULL);
> }
>
> /* Close instance */
> static int s5p_mfc_close_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> {
> struct s5p_mfc_dev *dev = ctx->dev;
> - struct s5p_mfc_cmd_args h2r_args;
> int ret = 0;
>
> dev->curr_ctx = ctx->num;
> @@ -145,7 +133,7 @@ static int s5p_mfc_close_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
> mfc_write(dev, ctx->inst_no, S5P_FIMV_INSTANCE_ID_V6);
> ret = s5p_mfc_cmd_host2risc_v6(dev,
> S5P_FIMV_H2R_CMD_CLOSE_INSTANCE_V6,
> - &h2r_args);
> + NULL);
> } else {
> ret = -EINVAL;
> }
>
> ---
> base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
> change-id: 20250715-media-s5p-mfc-fix-uninit-const-pointer-cbf944ae4b4b
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 5+ messages in thread