Re: [PATCH] media: s5p-mfc: Always pass NULL to s5p_mfc_cmd_host2risc_v6()

[Marek Szyprowski <m.szyprowski@samsung.com>]

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