diff mbox series

media: amphion: fix some issues reported by coverity

Message ID 20230717074006.22372-1-ming.qian@nxp.com
State New
Headers
Series media: amphion: fix some issues reported by coverity |

Commit Message

Ming Qian July 17, 2023, 7:40 a.m. UTC 
  CHECKED_RETURN: 4 case
REVERSE_INULL: 2 case
UNINIT: 6 case
UNUSED_VALUE: 1 case

Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")
Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 drivers/media/platform/amphion/vdec.c     |  5 ++++-
 drivers/media/platform/amphion/venc.c     |  6 ++++--
 drivers/media/platform/amphion/vpu_cmds.c |  5 +++--
 drivers/media/platform/amphion/vpu_core.c |  2 ++
 drivers/media/platform/amphion/vpu_dbg.c  | 11 +++++++++--
 drivers/media/platform/amphion/vpu_msgs.c | 12 ++++++------
 6 files changed, 28 insertions(+), 13 deletions(-)

Comments

Nicolas Dufresne July 17, 2023, 2:54 p.m. UTC | #1 
Hi Ming,

Le lundi 17 juillet 2023 à 15:40 +0800, Ming Qian a écrit :
> CHECKED_RETURN: 4 case
> REVERSE_INULL: 2 case
> UNINIT: 6 case
> UNUSED_VALUE: 1 case
> 
> Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  drivers/media/platform/amphion/vdec.c     |  5 ++++-
>  drivers/media/platform/amphion/venc.c     |  6 ++++--
>  drivers/media/platform/amphion/vpu_cmds.c |  5 +++--
>  drivers/media/platform/amphion/vpu_core.c |  2 ++
>  drivers/media/platform/amphion/vpu_dbg.c  | 11 +++++++++--
>  drivers/media/platform/amphion/vpu_msgs.c | 12 ++++++------
>  6 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
> index eeb2ef72df5b..133d77d1ea0c 100644
> --- a/drivers/media/platform/amphion/vdec.c
> +++ b/drivers/media/platform/amphion/vdec.c
> @@ -1019,6 +1019,7 @@ static int vdec_response_frame_abnormal(struct vpu_inst *inst)
>  {
>  	struct vdec_t *vdec = inst->priv;
>  	struct vpu_fs_info info;
> +	int ret;
>  
>  	if (!vdec->req_frame_count)
>  		return 0;
> @@ -1026,7 +1027,9 @@ static int vdec_response_frame_abnormal(struct vpu_inst *inst)
>  	memset(&info, 0, sizeof(info));
>  	info.type = MEM_RES_FRAME;
>  	info.tag = vdec->seq_tag + 0xf0;
> -	vpu_session_alloc_fs(inst, &info);
> +	ret = vpu_session_alloc_fs(inst, &info);
> +	if (ret)
> +		return ret;
>  	vdec->req_frame_count--;
>  
>  	return 0;
> diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
> index 58480e2755ec..4eb57d793a9c 100644
> --- a/drivers/media/platform/amphion/venc.c
> +++ b/drivers/media/platform/amphion/venc.c
> @@ -268,7 +268,7 @@ static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
>  {
>  	struct vpu_inst *inst = to_inst(file);
>  	struct venc_t *venc = inst->priv;
> -	struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
> +	struct v4l2_fract *timeperframe;

Could be just me, but I feel I'm missing some context to understand why this
change. Perhaps the commit message could be improved ?

All other changes looks like improvement to me, so with a good explanation on
this one (and the change seems to be equivalent), you can add:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

>  
>  	if (!parm)
>  		return -EINVAL;
> @@ -279,6 +279,7 @@ static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
>  	if (!vpu_helper_check_type(inst, parm->type))
>  		return -EINVAL;
>  
> +	timeperframe = &parm->parm.capture.timeperframe;
>  	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>  	parm->parm.capture.readbuffers = 0;
>  	timeperframe->numerator = venc->params.frame_rate.numerator;
> @@ -291,7 +292,7 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
>  {
>  	struct vpu_inst *inst = to_inst(file);
>  	struct venc_t *venc = inst->priv;
> -	struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
> +	struct v4l2_fract *timeperframe;
>  	unsigned long n, d;
>  
>  	if (!parm)
> @@ -303,6 +304,7 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
>  	if (!vpu_helper_check_type(inst, parm->type))
>  		return -EINVAL;
>  
> +	timeperframe = &parm->parm.capture.timeperframe;
>  	if (!timeperframe->numerator)
>  		timeperframe->numerator = venc->params.frame_rate.numerator;
>  	if (!timeperframe->denominator)
> diff --git a/drivers/media/platform/amphion/vpu_cmds.c b/drivers/media/platform/amphion/vpu_cmds.c
> index 647d94554fb5..235b71398d40 100644
> --- a/drivers/media/platform/amphion/vpu_cmds.c
> +++ b/drivers/media/platform/amphion/vpu_cmds.c
> @@ -306,7 +306,8 @@ static void vpu_core_keep_active(struct vpu_core *core)
>  
>  	dev_dbg(core->dev, "try to wake up\n");
>  	mutex_lock(&core->cmd_lock);
> -	vpu_cmd_send(core, &pkt);
> +	if (vpu_cmd_send(core, &pkt))
> +		dev_err(core->dev, "fail to keep active\n");
>  	mutex_unlock(&core->cmd_lock);
>  }
>  
> @@ -314,7 +315,7 @@ static int vpu_session_send_cmd(struct vpu_inst *inst, u32 id, void *data)
>  {
>  	unsigned long key;
>  	int sync = false;
> -	int ret = -EINVAL;
> +	int ret;
>  
>  	if (inst->id < 0)
>  		return -EINVAL;
> diff --git a/drivers/media/platform/amphion/vpu_core.c b/drivers/media/platform/amphion/vpu_core.c
> index 43d85a54268b..6f054700d5db 100644
> --- a/drivers/media/platform/amphion/vpu_core.c
> +++ b/drivers/media/platform/amphion/vpu_core.c
> @@ -88,6 +88,8 @@ static int vpu_core_boot_done(struct vpu_core *core)
>  
>  		core->supported_instance_count = min(core->supported_instance_count, count);
>  	}
> +	if (core->supported_instance_count >= BITS_PER_TYPE(core->instance_mask))
> +		core->supported_instance_count = BITS_PER_TYPE(core->instance_mask);
>  	core->fw_version = fw_version;
>  	vpu_core_set_state(core, VPU_CORE_ACTIVE);
>  
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index adc523b95061..982c2c777484 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -50,6 +50,13 @@ static char *vpu_stat_name[] = {
>  	[VPU_BUF_STATE_ERROR] = "error",
>  };
>  
> +static inline const char *to_vpu_stat_name(int state)
> +{
> +	if (state <= VPU_BUF_STATE_ERROR)
> +		return vpu_stat_name[state];
> +	return "unknown";
> +}
> +
>  static int vpu_dbg_instance(struct seq_file *s, void *data)
>  {
>  	struct vpu_inst *inst = s->private;
> @@ -141,7 +148,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>  		num = scnprintf(str, sizeof(str),
>  				"output [%2d] state = %10s, %8s\n",
>  				i, vb2_stat_name[vb->state],
> -				vpu_stat_name[vpu_get_buffer_state(vbuf)]);
> +				to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
>  		if (seq_write(s, str, num))
>  			return 0;
>  	}
> @@ -156,7 +163,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>  		num = scnprintf(str, sizeof(str),
>  				"capture[%2d] state = %10s, %8s\n",
>  				i, vb2_stat_name[vb->state],
> -				vpu_stat_name[vpu_get_buffer_state(vbuf)]);
> +				to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
>  		if (seq_write(s, str, num))
>  			return 0;
>  	}
> diff --git a/drivers/media/platform/amphion/vpu_msgs.c b/drivers/media/platform/amphion/vpu_msgs.c
> index f9eb488d1b5e..d0ead051f7d1 100644
> --- a/drivers/media/platform/amphion/vpu_msgs.c
> +++ b/drivers/media/platform/amphion/vpu_msgs.c
> @@ -32,7 +32,7 @@ static void vpu_session_handle_start_done(struct vpu_inst *inst, struct vpu_rpc_
>  
>  static void vpu_session_handle_mem_request(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
>  {
> -	struct vpu_pkt_mem_req_data req_data;
> +	struct vpu_pkt_mem_req_data req_data = { 0 };
>  
>  	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&req_data);
>  	vpu_trace(inst->dev, "[%d] %d:%d %d:%d %d:%d\n",
> @@ -80,7 +80,7 @@ static void vpu_session_handle_resolution_change(struct vpu_inst *inst, struct v
>  
>  static void vpu_session_handle_enc_frame_done(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
>  {
> -	struct vpu_enc_pic_info info;
> +	struct vpu_enc_pic_info info = { 0 };
>  
>  	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
>  	dev_dbg(inst->dev, "[%d] frame id = %d, wptr = 0x%x, size = %d\n",
> @@ -90,7 +90,7 @@ static void vpu_session_handle_enc_frame_done(struct vpu_inst *inst, struct vpu_
>  
>  static void vpu_session_handle_frame_request(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
>  {
> -	struct vpu_fs_info fs;
> +	struct vpu_fs_info fs = { 0 };
>  
>  	vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
>  	call_void_vop(inst, event_notify, VPU_MSG_ID_FRAME_REQ, &fs);
> @@ -107,7 +107,7 @@ static void vpu_session_handle_frame_release(struct vpu_inst *inst, struct vpu_r
>  		info.type = inst->out_format.type;
>  		call_void_vop(inst, buf_done, &info);
>  	} else if (inst->core->type == VPU_CORE_TYPE_DEC) {
> -		struct vpu_fs_info fs;
> +		struct vpu_fs_info fs = { 0 };
>  
>  		vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
>  		call_void_vop(inst, event_notify, VPU_MSG_ID_FRAME_RELEASE, &fs);
> @@ -122,7 +122,7 @@ static void vpu_session_handle_input_done(struct vpu_inst *inst, struct vpu_rpc_
>  
>  static void vpu_session_handle_pic_decoded(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
>  {
> -	struct vpu_dec_pic_info info;
> +	struct vpu_dec_pic_info info = { 0 };
>  
>  	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
>  	call_void_vop(inst, get_one_frame, &info);
> @@ -130,7 +130,7 @@ static void vpu_session_handle_pic_decoded(struct vpu_inst *inst, struct vpu_rpc
>  
>  static void vpu_session_handle_pic_done(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
>  {
> -	struct vpu_dec_pic_info info;
> +	struct vpu_dec_pic_info info = { 0 };
>  	struct vpu_frame_info frame;
>  
>  	memset(&frame, 0, sizeof(frame));
Ming Qian July 18, 2023, 1:50 a.m. UTC | #2 
>From: Nicolas Dufresne <nicolas@ndufresne.ca>
>Sent: 2023年7月17日 22:55
>To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; hverkuil-
>cisco@xs4all.nl
>Cc: shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
>imx@nxp.com>; X.H. Bao <xiahong.bao@nxp.com>; Eagle Zhou
><eagle.zhou@nxp.com>; Tao Jiang <tao.jiang_2@nxp.com>; linux-
>media@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org
>Subject: [EXT] Re: [PATCH] media: amphion: fix some issues reported by
>coverity
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>Hi Ming,
>
>Le lundi 17 juillet 2023 à 15:40 +0800, Ming Qian a écrit :
>> CHECKED_RETURN: 4 case
>> REVERSE_INULL: 2 case
>> UNINIT: 6 case
>> UNUSED_VALUE: 1 case
>>
>> Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>>  drivers/media/platform/amphion/vdec.c     |  5 ++++-
>>  drivers/media/platform/amphion/venc.c     |  6 ++++--
>>  drivers/media/platform/amphion/vpu_cmds.c |  5 +++--
>> drivers/media/platform/amphion/vpu_core.c |  2 ++
>> drivers/media/platform/amphion/vpu_dbg.c  | 11 +++++++++--
>> drivers/media/platform/amphion/vpu_msgs.c | 12 ++++++------
>>  6 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/amphion/vdec.c
>> b/drivers/media/platform/amphion/vdec.c
>> index eeb2ef72df5b..133d77d1ea0c 100644
>> --- a/drivers/media/platform/amphion/vdec.c
>> +++ b/drivers/media/platform/amphion/vdec.c
>> @@ -1019,6 +1019,7 @@ static int vdec_response_frame_abnormal(struct
>> vpu_inst *inst)  {
>>       struct vdec_t *vdec = inst->priv;
>>       struct vpu_fs_info info;
>> +     int ret;
>>
>>       if (!vdec->req_frame_count)
>>               return 0;
>> @@ -1026,7 +1027,9 @@ static int vdec_response_frame_abnormal(struct
>vpu_inst *inst)
>>       memset(&info, 0, sizeof(info));
>>       info.type = MEM_RES_FRAME;
>>       info.tag = vdec->seq_tag + 0xf0;
>> -     vpu_session_alloc_fs(inst, &info);
>> +     ret = vpu_session_alloc_fs(inst, &info);
>> +     if (ret)
>> +             return ret;
>>       vdec->req_frame_count--;
>>
>>       return 0;
>> diff --git a/drivers/media/platform/amphion/venc.c
>> b/drivers/media/platform/amphion/venc.c
>> index 58480e2755ec..4eb57d793a9c 100644
>> --- a/drivers/media/platform/amphion/venc.c
>> +++ b/drivers/media/platform/amphion/venc.c
>> @@ -268,7 +268,7 @@ static int venc_g_parm(struct file *file, void
>> *fh, struct v4l2_streamparm *parm  {
>>       struct vpu_inst *inst = to_inst(file);
>>       struct venc_t *venc = inst->priv;
>> -     struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
>> +     struct v4l2_fract *timeperframe;
>
>Could be just me, but I feel I'm missing some context to understand why this
>change. Perhaps the commit message could be improved ?
>
>All other changes looks like improvement to me, so with a good explanation
>on this one (and the change seems to be equivalent), you can add:
>
>Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>

Hi Nicolas,
    The Coverity scan report a REVERSE_INULL issue here, that directly dereferencing pointer "param", before Null-checking "parm". 
    I'll split this patch into several patches, one topic one patch.

Ming

>>
>>       if (!parm)
>>               return -EINVAL;
>> @@ -279,6 +279,7 @@ static int venc_g_parm(struct file *file, void *fh,
>struct v4l2_streamparm *parm
>>       if (!vpu_helper_check_type(inst, parm->type))
>>               return -EINVAL;
>>
>> +     timeperframe = &parm->parm.capture.timeperframe;
>>       parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>>       parm->parm.capture.readbuffers = 0;
>>       timeperframe->numerator = venc->params.frame_rate.numerator;
>> @@ -291,7 +292,7 @@ static int venc_s_parm(struct file *file, void
>> *fh, struct v4l2_streamparm *parm  {
>>       struct vpu_inst *inst = to_inst(file);
>>       struct venc_t *venc = inst->priv;
>> -     struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
>> +     struct v4l2_fract *timeperframe;
>>       unsigned long n, d;
>>
>>       if (!parm)
>> @@ -303,6 +304,7 @@ static int venc_s_parm(struct file *file, void *fh,
>struct v4l2_streamparm *parm
>>       if (!vpu_helper_check_type(inst, parm->type))
>>               return -EINVAL;
>>
>> +     timeperframe = &parm->parm.capture.timeperframe;
>>       if (!timeperframe->numerator)
>>               timeperframe->numerator = venc->params.frame_rate.numerator;
>>       if (!timeperframe->denominator)
>> diff --git a/drivers/media/platform/amphion/vpu_cmds.c
>> b/drivers/media/platform/amphion/vpu_cmds.c
>> index 647d94554fb5..235b71398d40 100644
>> --- a/drivers/media/platform/amphion/vpu_cmds.c
>> +++ b/drivers/media/platform/amphion/vpu_cmds.c
>> @@ -306,7 +306,8 @@ static void vpu_core_keep_active(struct vpu_core
>> *core)
>>
>>       dev_dbg(core->dev, "try to wake up\n");
>>       mutex_lock(&core->cmd_lock);
>> -     vpu_cmd_send(core, &pkt);
>> +     if (vpu_cmd_send(core, &pkt))
>> +             dev_err(core->dev, "fail to keep active\n");
>>       mutex_unlock(&core->cmd_lock);
>>  }
>>
>> @@ -314,7 +315,7 @@ static int vpu_session_send_cmd(struct vpu_inst
>> *inst, u32 id, void *data)  {
>>       unsigned long key;
>>       int sync = false;
>> -     int ret = -EINVAL;
>> +     int ret;
>>
>>       if (inst->id < 0)
>>               return -EINVAL;
>> diff --git a/drivers/media/platform/amphion/vpu_core.c
>> b/drivers/media/platform/amphion/vpu_core.c
>> index 43d85a54268b..6f054700d5db 100644
>> --- a/drivers/media/platform/amphion/vpu_core.c
>> +++ b/drivers/media/platform/amphion/vpu_core.c
>> @@ -88,6 +88,8 @@ static int vpu_core_boot_done(struct vpu_core *core)
>>
>>               core->supported_instance_count = min(core-
>>supported_instance_count, count);
>>       }
>> +     if (core->supported_instance_count >= BITS_PER_TYPE(core-
>>instance_mask))
>> +             core->supported_instance_count =
>> + BITS_PER_TYPE(core->instance_mask);
>>       core->fw_version = fw_version;
>>       vpu_core_set_state(core, VPU_CORE_ACTIVE);
>>
>> diff --git a/drivers/media/platform/amphion/vpu_dbg.c
>> b/drivers/media/platform/amphion/vpu_dbg.c
>> index adc523b95061..982c2c777484 100644
>> --- a/drivers/media/platform/amphion/vpu_dbg.c
>> +++ b/drivers/media/platform/amphion/vpu_dbg.c
>> @@ -50,6 +50,13 @@ static char *vpu_stat_name[] = {
>>       [VPU_BUF_STATE_ERROR] = "error",  };
>>
>> +static inline const char *to_vpu_stat_name(int state) {
>> +     if (state <= VPU_BUF_STATE_ERROR)
>> +             return vpu_stat_name[state];
>> +     return "unknown";
>> +}
>> +
>>  static int vpu_dbg_instance(struct seq_file *s, void *data)  {
>>       struct vpu_inst *inst = s->private; @@ -141,7 +148,7 @@ static
>> int vpu_dbg_instance(struct seq_file *s, void *data)
>>               num = scnprintf(str, sizeof(str),
>>                               "output [%2d] state = %10s, %8s\n",
>>                               i, vb2_stat_name[vb->state],
>> -                             vpu_stat_name[vpu_get_buffer_state(vbuf)]);
>> +
>> + to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
>>               if (seq_write(s, str, num))
>>                       return 0;
>>       }
>> @@ -156,7 +163,7 @@ static int vpu_dbg_instance(struct seq_file *s, void
>*data)
>>               num = scnprintf(str, sizeof(str),
>>                               "capture[%2d] state = %10s, %8s\n",
>>                               i, vb2_stat_name[vb->state],
>> -                             vpu_stat_name[vpu_get_buffer_state(vbuf)]);
>> +
>> + to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
>>               if (seq_write(s, str, num))
>>                       return 0;
>>       }
>> diff --git a/drivers/media/platform/amphion/vpu_msgs.c
>> b/drivers/media/platform/amphion/vpu_msgs.c
>> index f9eb488d1b5e..d0ead051f7d1 100644
>> --- a/drivers/media/platform/amphion/vpu_msgs.c
>> +++ b/drivers/media/platform/amphion/vpu_msgs.c
>> @@ -32,7 +32,7 @@ static void vpu_session_handle_start_done(struct
>> vpu_inst *inst, struct vpu_rpc_
>>
>>  static void vpu_session_handle_mem_request(struct vpu_inst *inst,
>> struct vpu_rpc_event *pkt)  {
>> -     struct vpu_pkt_mem_req_data req_data;
>> +     struct vpu_pkt_mem_req_data req_data = { 0 };
>>
>>       vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&req_data);
>>       vpu_trace(inst->dev, "[%d] %d:%d %d:%d %d:%d\n", @@ -80,7 +80,7
>> @@ static void vpu_session_handle_resolution_change(struct vpu_inst
>> *inst, struct v
>>
>>  static void vpu_session_handle_enc_frame_done(struct vpu_inst *inst,
>> struct vpu_rpc_event *pkt)  {
>> -     struct vpu_enc_pic_info info;
>> +     struct vpu_enc_pic_info info = { 0 };
>>
>>       vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
>>       dev_dbg(inst->dev, "[%d] frame id = %d, wptr = 0x%x, size =
>> %d\n", @@ -90,7 +90,7 @@ static void
>> vpu_session_handle_enc_frame_done(struct vpu_inst *inst, struct vpu_
>>
>>  static void vpu_session_handle_frame_request(struct vpu_inst *inst,
>> struct vpu_rpc_event *pkt)  {
>> -     struct vpu_fs_info fs;
>> +     struct vpu_fs_info fs = { 0 };
>>
>>       vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
>>       call_void_vop(inst, event_notify, VPU_MSG_ID_FRAME_REQ, &fs); @@
>> -107,7 +107,7 @@ static void vpu_session_handle_frame_release(struct
>vpu_inst *inst, struct vpu_r
>>               info.type = inst->out_format.type;
>>               call_void_vop(inst, buf_done, &info);
>>       } else if (inst->core->type == VPU_CORE_TYPE_DEC) {
>> -             struct vpu_fs_info fs;
>> +             struct vpu_fs_info fs = { 0 };
>>
>>               vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
>>               call_void_vop(inst, event_notify,
>> VPU_MSG_ID_FRAME_RELEASE, &fs); @@ -122,7 +122,7 @@ static void
>> vpu_session_handle_input_done(struct vpu_inst *inst, struct vpu_rpc_
>>
>>  static void vpu_session_handle_pic_decoded(struct vpu_inst *inst,
>> struct vpu_rpc_event *pkt)  {
>> -     struct vpu_dec_pic_info info;
>> +     struct vpu_dec_pic_info info = { 0 };
>>
>>       vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
>>       call_void_vop(inst, get_one_frame, &info); @@ -130,7 +130,7 @@
>> static void vpu_session_handle_pic_decoded(struct vpu_inst *inst,
>> struct vpu_rpc
>>
>>  static void vpu_session_handle_pic_done(struct vpu_inst *inst, struct
>> vpu_rpc_event *pkt)  {
>> -     struct vpu_dec_pic_info info;
>> +     struct vpu_dec_pic_info info = { 0 };
>>       struct vpu_frame_info frame;
>>
>>       memset(&frame, 0, sizeof(frame));
Ming Qian July 18, 2023, 1:51 a.m. UTC | #3 
>From: Markus Elfring <Markus.Elfring@web.de>
>Sent: 2023年7月18日 1:26
>To: Ming Qian <ming.qian@nxp.com>; linux-media@vger.kernel.org; kernel-
>janitors@vger.kernel.org; linux-arm-kernel@lists.infradead.org; dl-linux-imx
><linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
><festevam@gmail.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Mauro
>Carvalho Chehab <mchehab@kernel.org>; Nicolas Dufresne
><nicolas@ndufresne.ca>; Rob Herring <robh+dt@kernel.org>; Sascha Hauer
><s.hauer@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; Tao Jiang
><tao.jiang_2@nxp.com>; X.H. Bao <xiahong.bao@nxp.com>; Eagle Zhou
><eagle.zhou@nxp.com>
>Cc: LKML <linux-kernel@vger.kernel.org>
>Subject: [EXT] Re: [PATCH] media: amphion: fix some issues reported by
>coverity
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>> CHECKED_RETURN: 4 case
>> REVERSE_INULL: 2 case
>> UNINIT: 6 case
>> UNUSED_VALUE: 1 case
>
>How does such a change combination fit to the development requirement
>“Solve only one problem per patch.”?
>
>See also:
>https://git.ker/
>nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ft
>ree%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%3Fh%3Dv6.5-
>rc2%23n81&data=05%7C01%7Cming.qian%40nxp.com%7C94575f499df74c702
>d8308db86ead678%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
>8252115406304424%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
>LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sd
>ata=nr7bqZMPNvrECSOMJsCzn3aU8UWdinBG4sHRRGRJJ1E%3D&reserved=0
>
>
>Will further imperative change descriptions become more helpful?
>
>Regards,
>Markus

Hi Markus,
    Thanks for your comments, I'll split this patch into 4 patches, one topic one patch.

Ming
Nicolas Dufresne July 18, 2023, 4:17 p.m. UTC | #4 
Le mardi 18 juillet 2023 à 01:50 +0000, Ming Qian a écrit :
> > > diff --git a/drivers/media/platform/amphion/venc.c
> > > b/drivers/media/platform/amphion/venc.c
> > > index 58480e2755ec..4eb57d793a9c 100644
> > > --- a/drivers/media/platform/amphion/venc.c
> > > +++ b/drivers/media/platform/amphion/venc.c
> > > @@ -268,7 +268,7 @@ static int venc_g_parm(struct file *file, void
> > > *fh, struct v4l2_streamparm *parm  {
> > >        struct vpu_inst *inst = to_inst(file);
> > >        struct venc_t *venc = inst->priv;
> > > -     struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
> > > +     struct v4l2_fract *timeperframe;
> > 
> > Could be just me, but I feel I'm missing some context to understand why this
> > change. Perhaps the commit message could be improved ?
> > 
> > All other changes looks like improvement to me, so with a good explanation
> > on this one (and the change seems to be equivalent), you can add:
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> 
> Hi Nicolas,
>     The Coverity scan report a REVERSE_INULL issue here, that directly dereferencing pointer "param", before Null-checking "parm". 
>     I'll split this patch into several patches, one topic one patch.
> 
> Ming

Make sense now, looking forward a split version with more explanation.

regards,
Nicolas
diff mbox series

Patch

diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
index eeb2ef72df5b..133d77d1ea0c 100644
--- a/drivers/media/platform/amphion/vdec.c
+++ b/drivers/media/platform/amphion/vdec.c
@@ -1019,6 +1019,7 @@  static int vdec_response_frame_abnormal(struct vpu_inst *inst)
 {
 	struct vdec_t *vdec = inst->priv;
 	struct vpu_fs_info info;
+	int ret;
 
 	if (!vdec->req_frame_count)
 		return 0;
@@ -1026,7 +1027,9 @@  static int vdec_response_frame_abnormal(struct vpu_inst *inst)
 	memset(&info, 0, sizeof(info));
 	info.type = MEM_RES_FRAME;
 	info.tag = vdec->seq_tag + 0xf0;
-	vpu_session_alloc_fs(inst, &info);
+	ret = vpu_session_alloc_fs(inst, &info);
+	if (ret)
+		return ret;
 	vdec->req_frame_count--;
 
 	return 0;
diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
index 58480e2755ec..4eb57d793a9c 100644
--- a/drivers/media/platform/amphion/venc.c
+++ b/drivers/media/platform/amphion/venc.c
@@ -268,7 +268,7 @@  static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
 {
 	struct vpu_inst *inst = to_inst(file);
 	struct venc_t *venc = inst->priv;
-	struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
+	struct v4l2_fract *timeperframe;
 
 	if (!parm)
 		return -EINVAL;
@@ -279,6 +279,7 @@  static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
 	if (!vpu_helper_check_type(inst, parm->type))
 		return -EINVAL;
 
+	timeperframe = &parm->parm.capture.timeperframe;
 	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
 	parm->parm.capture.readbuffers = 0;
 	timeperframe->numerator = venc->params.frame_rate.numerator;
@@ -291,7 +292,7 @@  static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
 {
 	struct vpu_inst *inst = to_inst(file);
 	struct venc_t *venc = inst->priv;
-	struct v4l2_fract *timeperframe = &parm->parm.capture.timeperframe;
+	struct v4l2_fract *timeperframe;
 	unsigned long n, d;
 
 	if (!parm)
@@ -303,6 +304,7 @@  static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm
 	if (!vpu_helper_check_type(inst, parm->type))
 		return -EINVAL;
 
+	timeperframe = &parm->parm.capture.timeperframe;
 	if (!timeperframe->numerator)
 		timeperframe->numerator = venc->params.frame_rate.numerator;
 	if (!timeperframe->denominator)
diff --git a/drivers/media/platform/amphion/vpu_cmds.c b/drivers/media/platform/amphion/vpu_cmds.c
index 647d94554fb5..235b71398d40 100644
--- a/drivers/media/platform/amphion/vpu_cmds.c
+++ b/drivers/media/platform/amphion/vpu_cmds.c
@@ -306,7 +306,8 @@  static void vpu_core_keep_active(struct vpu_core *core)
 
 	dev_dbg(core->dev, "try to wake up\n");
 	mutex_lock(&core->cmd_lock);
-	vpu_cmd_send(core, &pkt);
+	if (vpu_cmd_send(core, &pkt))
+		dev_err(core->dev, "fail to keep active\n");
 	mutex_unlock(&core->cmd_lock);
 }
 
@@ -314,7 +315,7 @@  static int vpu_session_send_cmd(struct vpu_inst *inst, u32 id, void *data)
 {
 	unsigned long key;
 	int sync = false;
-	int ret = -EINVAL;
+	int ret;
 
 	if (inst->id < 0)
 		return -EINVAL;
diff --git a/drivers/media/platform/amphion/vpu_core.c b/drivers/media/platform/amphion/vpu_core.c
index 43d85a54268b..6f054700d5db 100644
--- a/drivers/media/platform/amphion/vpu_core.c
+++ b/drivers/media/platform/amphion/vpu_core.c
@@ -88,6 +88,8 @@  static int vpu_core_boot_done(struct vpu_core *core)
 
 		core->supported_instance_count = min(core->supported_instance_count, count);
 	}
+	if (core->supported_instance_count >= BITS_PER_TYPE(core->instance_mask))
+		core->supported_instance_count = BITS_PER_TYPE(core->instance_mask);
 	core->fw_version = fw_version;
 	vpu_core_set_state(core, VPU_CORE_ACTIVE);
 
diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
index adc523b95061..982c2c777484 100644
--- a/drivers/media/platform/amphion/vpu_dbg.c
+++ b/drivers/media/platform/amphion/vpu_dbg.c
@@ -50,6 +50,13 @@  static char *vpu_stat_name[] = {
 	[VPU_BUF_STATE_ERROR] = "error",
 };
 
+static inline const char *to_vpu_stat_name(int state)
+{
+	if (state <= VPU_BUF_STATE_ERROR)
+		return vpu_stat_name[state];
+	return "unknown";
+}
+
 static int vpu_dbg_instance(struct seq_file *s, void *data)
 {
 	struct vpu_inst *inst = s->private;
@@ -141,7 +148,7 @@  static int vpu_dbg_instance(struct seq_file *s, void *data)
 		num = scnprintf(str, sizeof(str),
 				"output [%2d] state = %10s, %8s\n",
 				i, vb2_stat_name[vb->state],
-				vpu_stat_name[vpu_get_buffer_state(vbuf)]);
+				to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
 		if (seq_write(s, str, num))
 			return 0;
 	}
@@ -156,7 +163,7 @@  static int vpu_dbg_instance(struct seq_file *s, void *data)
 		num = scnprintf(str, sizeof(str),
 				"capture[%2d] state = %10s, %8s\n",
 				i, vb2_stat_name[vb->state],
-				vpu_stat_name[vpu_get_buffer_state(vbuf)]);
+				to_vpu_stat_name(vpu_get_buffer_state(vbuf)));
 		if (seq_write(s, str, num))
 			return 0;
 	}
diff --git a/drivers/media/platform/amphion/vpu_msgs.c b/drivers/media/platform/amphion/vpu_msgs.c
index f9eb488d1b5e..d0ead051f7d1 100644
--- a/drivers/media/platform/amphion/vpu_msgs.c
+++ b/drivers/media/platform/amphion/vpu_msgs.c
@@ -32,7 +32,7 @@  static void vpu_session_handle_start_done(struct vpu_inst *inst, struct vpu_rpc_
 
 static void vpu_session_handle_mem_request(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
 {
-	struct vpu_pkt_mem_req_data req_data;
+	struct vpu_pkt_mem_req_data req_data = { 0 };
 
 	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&req_data);
 	vpu_trace(inst->dev, "[%d] %d:%d %d:%d %d:%d\n",
@@ -80,7 +80,7 @@  static void vpu_session_handle_resolution_change(struct vpu_inst *inst, struct v
 
 static void vpu_session_handle_enc_frame_done(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
 {
-	struct vpu_enc_pic_info info;
+	struct vpu_enc_pic_info info = { 0 };
 
 	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
 	dev_dbg(inst->dev, "[%d] frame id = %d, wptr = 0x%x, size = %d\n",
@@ -90,7 +90,7 @@  static void vpu_session_handle_enc_frame_done(struct vpu_inst *inst, struct vpu_
 
 static void vpu_session_handle_frame_request(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
 {
-	struct vpu_fs_info fs;
+	struct vpu_fs_info fs = { 0 };
 
 	vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
 	call_void_vop(inst, event_notify, VPU_MSG_ID_FRAME_REQ, &fs);
@@ -107,7 +107,7 @@  static void vpu_session_handle_frame_release(struct vpu_inst *inst, struct vpu_r
 		info.type = inst->out_format.type;
 		call_void_vop(inst, buf_done, &info);
 	} else if (inst->core->type == VPU_CORE_TYPE_DEC) {
-		struct vpu_fs_info fs;
+		struct vpu_fs_info fs = { 0 };
 
 		vpu_iface_unpack_msg_data(inst->core, pkt, &fs);
 		call_void_vop(inst, event_notify, VPU_MSG_ID_FRAME_RELEASE, &fs);
@@ -122,7 +122,7 @@  static void vpu_session_handle_input_done(struct vpu_inst *inst, struct vpu_rpc_
 
 static void vpu_session_handle_pic_decoded(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
 {
-	struct vpu_dec_pic_info info;
+	struct vpu_dec_pic_info info = { 0 };
 
 	vpu_iface_unpack_msg_data(inst->core, pkt, (void *)&info);
 	call_void_vop(inst, get_one_frame, &info);
@@ -130,7 +130,7 @@  static void vpu_session_handle_pic_decoded(struct vpu_inst *inst, struct vpu_rpc
 
 static void vpu_session_handle_pic_done(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
 {
-	struct vpu_dec_pic_info info;
+	struct vpu_dec_pic_info info = { 0 };
 	struct vpu_frame_info frame;
 
 	memset(&frame, 0, sizeof(frame));