On Tue, 2019-02-26 at 17:43 -0700, Shaobo He wrote: > The fixes included in this commit essentially removes NULL pointer > checks on the return values of function `get_queue_ctx` as well as > `v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c. > > Function `get_queue_ctx` is very unlikely to return a NULL pointer > because its return value is an address composed of the base address > pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`. > Since the offset of either field is not 0, for the return value to be > NULL, pointer `m2m_ctx` must be a very large unsigned value such that > its addition to the offset overflows to NULL which may be undefined > according to this post: > https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if > `m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I > think it is reasonable to conclude that the return value of function > `get_queue_ctx` cannot be NULL. > > Given the return values of `get_queue_ctx` not being NULL, we can follow > a similar reasoning to conclude that the return value of > `v4l2_mem_get_vq` cannot be NULL since its return value is the same > address as the return value of `get_queue_ctx`. Therefore, this patch > also removes NULL pointer checks on the return values of > `v4l2_mem_get_vq`. > > Signed-off-by: Shaobo He <shaobo@xxxxxxxxxxx> Hi Shaobo, It seems this is v2 of 1551128631-19713-1-git-send-email-shaobo@xxxxxxxxxxx, and it should be marked as such. Do you think you can read Documentation/process/submitting-patches.rst, for your future patches? Also, two comments... > drivers/media/platform/coda/coda-common.c | 4 ---- > drivers/media/platform/imx-pxp.c | 7 ------- > drivers/media/platform/m2m-deinterlace.c | 7 ------- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 7 ------- > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 ------- > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 ------------- > drivers/media/platform/mx2_emmaprp.c | 7 ------- > drivers/media/platform/rcar_fdp1.c | 3 --- > drivers/media/platform/rcar_jpu.c | 8 -------- > drivers/media/platform/rockchip/rga/rga.c | 4 ---- > drivers/media/platform/s5p-g2d/g2d.c | 4 ---- > drivers/media/platform/s5p-jpeg/jpeg-core.c | 7 ------- > drivers/media/platform/sh_veu.c | 2 -- > drivers/media/platform/ti-vpe/vpe.c | 7 ------- > drivers/media/platform/vicodec/vicodec-core.c | 5 ----- > drivers/media/platform/vim2m.c | 7 ------- > drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ---- > 17 files changed, 103 deletions(-) > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > index 7518f01..ee1e05b 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct v4l2_format *f, > struct vb2_queue *vq; > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > - if (!vq) > - return -EINVAL; > > q_data = get_q_data(ctx, f->type); > if (!q_data) > @@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv, > ctx->quantization = f->fmt.pix.quantization; > > dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > - if (!dst_vq) > - return -EINVAL; > > /* > * Setting the capture queue format is not possible while the capture > diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c > index c1c2554..d079b3c 100644 > --- a/drivers/media/platform/imx-pxp.c > +++ b/drivers/media/platform/imx-pxp.c > @@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void *priv, > > static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f) > { > - struct vb2_queue *vq; > struct pxp_q_data *q_data; > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > - if (!vq) > - return -EINVAL; > - It seems your patch also removes unused code, but this is not really explained in the commit log. Perhaps it is better to split all these changes on their own patch: one patch to remove dead code, and then another patch to remove unneeded null checks. And also, I think you should add some comments, either in v4l2_m2m_get_vq's declaration or definition, explaining that the return value cannot be NULL. I have to say: I'm not a fan of "improvement" patches in code paths that are anything but hot... but knock yourself out! Thanks, Eze
