| Message ID | 20230717074006.22372-1-ming.qian@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: amphion: fix some issues reported by coverity | expand |
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));
> 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.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc2#n81 Will further imperative change descriptions become more helpful? Regards, Markus
>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));
>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
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 --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));
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(-)