Message ID | 20170628201435.3237712-1-arnd@arndb.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Arnd, thank you for the cleanup. I see two issues below: On Wed, 2017-06-28 at 22:13 +0200, Arnd Bergmann wrote: > While looking at a compiler warning, I noticed the use of > IS_ERR_OR_NULL, which is generally a sign of a bad API design > and should be avoided. > > In this driver, this is fairly easy, we can simply stop storing > error pointers in persistent structures, and change the one > function that might return either a NULL pointer or an error > code to consistently return error pointers when failing. > > Fixes: e130291212df ("[media] media: Add i.MX media core driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > I can't reproduce the original warning any more, but this > patch still makes sense by itself. > --- > drivers/staging/media/imx/imx-ic-prpencvf.c | 41 ++++++++++++++++------------- > drivers/staging/media/imx/imx-media-csi.c | 29 +++++++++++--------- > drivers/staging/media/imx/imx-media-dev.c | 2 +- > drivers/staging/media/imx/imx-media-of.c | 2 +- > drivers/staging/media/imx/imx-media-vdic.c | 37 ++++++++++++++------------ > 5 files changed, 61 insertions(+), 50 deletions(-) > [...] > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index a2d26693912e..a4b3c305dcc8 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -122,11 +122,11 @@ static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev) > > static void csi_idmac_put_ipu_resources(struct csi_priv *priv) > { > - if (!IS_ERR_OR_NULL(priv->idmac_ch)) > + if (priv->idmac_ch) > ipu_idmac_put(priv->idmac_ch); > priv->idmac_ch = NULL; > > - if (!IS_ERR_OR_NULL(priv->smfc)) > + if (priv->smfc) > ipu_smfc_put(priv->smfc); > priv->smfc = NULL; > } > @@ -134,23 +134,26 @@ static void csi_idmac_put_ipu_resources(struct csi_priv *priv) > static int csi_idmac_get_ipu_resources(struct csi_priv *priv) > { > int ch_num, ret; > + struct ipu_smfc *smfc, *idmac_ch; This should be + struct ipuv3_channel *idmac_ch; + struct ipu_smfc *smfc; instead. [...] > diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c > index 48cbc7716758..c58ff0831890 100644 > --- a/drivers/staging/media/imx/imx-media-dev.c > +++ b/drivers/staging/media/imx/imx-media-dev.c > @@ -91,7 +91,7 @@ imx_media_add_async_subdev(struct imx_media_dev *imxmd, > if (imx_media_find_async_subdev(imxmd, np, devname)) { > dev_dbg(imxmd->md.dev, "%s: already added %s\n", > __func__, np ? np->name : devname); > - imxsd = NULL; > + imxsd = ERR_PTR(-EEXIST); And since this returns -EEXIST now, ... > goto out; > } > > diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c > index b026fe66467c..4aac42cb79a4 100644 > --- a/drivers/staging/media/imx/imx-media-of.c > +++ b/drivers/staging/media/imx/imx-media-of.c > @@ -115,7 +115,7 @@ of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np, > > /* register this subdev with async notifier */ > imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL); > - if (IS_ERR_OR_NULL(imxsd)) > + if (IS_ERR(imxsd)) > return imxsd; ... this changes behaviour: imx-media: imx_media_of_parse failed with -17 imx-media: probe of capture-subsystem failed with error -17 We must continue to return NULL here if imxsd == -EEXIST: - return imxsd; + return PTR_ERR(imxsd) == -EEXIST ? NULL : imxsd; or change the code where of_parse_subdev is called (from imx_media_of_parse, and recursively from of_parse_subdev) to not handle the -EEXIST return value as an error. With those fixed, Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
Hi Arnd, [auto build test ERROR on linuxtv-media/master] [also build test ERROR on next-20170630] [cannot apply to v4.12-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/staging-imx-remove-confusing-IS_ERR_OR_NULL-usage/20170701-095942 base: git://linuxtv.org/media_tree.git master config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): drivers/staging/media/imx/imx-media-csi.c: In function 'csi_idmac_get_ipu_resources': >> drivers/staging/media/imx/imx-media-csi.c:149:11: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types] idmac_ch = ipu_idmac_get(priv->ipu, ch_num); ^ drivers/staging/media/imx/imx-media-csi.c:156:17: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types] priv->idmac_ch = idmac_ch; ^ cc1: some warnings being treated as errors vim +149 drivers/staging/media/imx/imx-media-csi.c 143 v4l2_err(&priv->sd, "failed to get SMFC\n"); 144 ret = PTR_ERR(smfc); 145 goto out; 146 } 147 priv->smfc = smfc; 148 > 149 idmac_ch = ipu_idmac_get(priv->ipu, ch_num); 150 if (IS_ERR(idmac_ch)) { 151 v4l2_err(&priv->sd, "could not get IDMAC channel %u\n", 152 ch_num); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Jun 29, 2017 at 11:13 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> @@ -134,23 +134,26 @@ static void csi_idmac_put_ipu_resources(struct csi_priv *priv) >> static int csi_idmac_get_ipu_resources(struct csi_priv *priv) >> { >> int ch_num, ret; >> + struct ipu_smfc *smfc, *idmac_ch; > > This should be > > + struct ipuv3_channel *idmac_ch; > + struct ipu_smfc *smfc; > > instead. Fixed in v2 now. > > ... this changes behaviour: > > imx-media: imx_media_of_parse failed with -17 > imx-media: probe of capture-subsystem failed with error -17 > > We must continue to return NULL here if imxsd == -EEXIST: > > - return imxsd; > + return PTR_ERR(imxsd) == -EEXIST ? NULL : imxsd; > > or change the code where of_parse_subdev is called (from > imx_media_of_parse, and recursively from of_parse_subdev) to not handle > the -EEXIST return value as an error. > > With those fixed, > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> I thought about it some more and tried to find a better solution for this function, which is now a bit different, so I did not add your tags. Can you have another look at v2? This time, of_parse_subdev separates the return code from the pointer, which seems less confusing in a function like that. There are in fact two cases where we return NULL and it's not clear if the caller should treat that as success or failure. I've left the current behavior the same but added comments there. Arnd
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index ed363fe3b3d0..7a9d9f32f989 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -134,19 +134,19 @@ static inline struct prp_priv *sd_to_priv(struct v4l2_subdev *sd) static void prp_put_ipu_resources(struct prp_priv *priv) { - if (!IS_ERR_OR_NULL(priv->ic)) + if (priv->ic) ipu_ic_put(priv->ic); priv->ic = NULL; - if (!IS_ERR_OR_NULL(priv->out_ch)) + if (priv->out_ch) ipu_idmac_put(priv->out_ch); priv->out_ch = NULL; - if (!IS_ERR_OR_NULL(priv->rot_in_ch)) + if (priv->rot_in_ch) ipu_idmac_put(priv->rot_in_ch); priv->rot_in_ch = NULL; - if (!IS_ERR_OR_NULL(priv->rot_out_ch)) + if (priv->rot_out_ch) ipu_idmac_put(priv->rot_out_ch); priv->rot_out_ch = NULL; } @@ -154,43 +154,46 @@ static void prp_put_ipu_resources(struct prp_priv *priv) static int prp_get_ipu_resources(struct prp_priv *priv) { struct imx_ic_priv *ic_priv = priv->ic_priv; + struct ipu_ic *ic; + struct ipuv3_channel *out_ch, *rot_in_ch, *rot_out_ch; int ret, task = ic_priv->task_id; priv->ipu = priv->md->ipu[ic_priv->ipu_id]; - priv->ic = ipu_ic_get(priv->ipu, task); - if (IS_ERR(priv->ic)) { + ic = ipu_ic_get(priv->ipu, task); + if (IS_ERR(ic)) { v4l2_err(&ic_priv->sd, "failed to get IC\n"); - ret = PTR_ERR(priv->ic); + ret = PTR_ERR(ic); goto out; } + priv->ic = ic; - priv->out_ch = ipu_idmac_get(priv->ipu, - prp_channel[task].out_ch); - if (IS_ERR(priv->out_ch)) { + out_ch = ipu_idmac_get(priv->ipu, prp_channel[task].out_ch); + if (IS_ERR(out_ch)) { v4l2_err(&ic_priv->sd, "could not get IDMAC channel %u\n", prp_channel[task].out_ch); - ret = PTR_ERR(priv->out_ch); + ret = PTR_ERR(out_ch); goto out; } + priv->out_ch = out_ch; - priv->rot_in_ch = ipu_idmac_get(priv->ipu, - prp_channel[task].rot_in_ch); - if (IS_ERR(priv->rot_in_ch)) { + rot_in_ch = ipu_idmac_get(priv->ipu, prp_channel[task].rot_in_ch); + if (IS_ERR(rot_in_ch)) { v4l2_err(&ic_priv->sd, "could not get IDMAC channel %u\n", prp_channel[task].rot_in_ch); - ret = PTR_ERR(priv->rot_in_ch); + ret = PTR_ERR(rot_in_ch); goto out; } + priv->rot_in_ch = rot_in_ch; - priv->rot_out_ch = ipu_idmac_get(priv->ipu, - prp_channel[task].rot_out_ch); - if (IS_ERR(priv->rot_out_ch)) { + rot_out_ch = ipu_idmac_get(priv->ipu, prp_channel[task].rot_out_ch); + if (IS_ERR(rot_out_ch)) { v4l2_err(&ic_priv->sd, "could not get IDMAC channel %u\n", prp_channel[task].rot_out_ch); - ret = PTR_ERR(priv->rot_out_ch); + ret = PTR_ERR(rot_out_ch); goto out; } + priv->rot_out_ch = rot_out_ch; return 0; out: diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index a2d26693912e..a4b3c305dcc8 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -122,11 +122,11 @@ static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev) static void csi_idmac_put_ipu_resources(struct csi_priv *priv) { - if (!IS_ERR_OR_NULL(priv->idmac_ch)) + if (priv->idmac_ch) ipu_idmac_put(priv->idmac_ch); priv->idmac_ch = NULL; - if (!IS_ERR_OR_NULL(priv->smfc)) + if (priv->smfc) ipu_smfc_put(priv->smfc); priv->smfc = NULL; } @@ -134,23 +134,26 @@ static void csi_idmac_put_ipu_resources(struct csi_priv *priv) static int csi_idmac_get_ipu_resources(struct csi_priv *priv) { int ch_num, ret; + struct ipu_smfc *smfc, *idmac_ch; ch_num = IPUV3_CHANNEL_CSI0 + priv->smfc_id; - priv->smfc = ipu_smfc_get(priv->ipu, ch_num); - if (IS_ERR(priv->smfc)) { + smfc = ipu_smfc_get(priv->ipu, ch_num); + if (IS_ERR(smfc)) { v4l2_err(&priv->sd, "failed to get SMFC\n"); - ret = PTR_ERR(priv->smfc); + ret = PTR_ERR(smfc); goto out; } + priv->smfc = smfc; - priv->idmac_ch = ipu_idmac_get(priv->ipu, ch_num); - if (IS_ERR(priv->idmac_ch)) { + idmac_ch = ipu_idmac_get(priv->ipu, ch_num); + if (IS_ERR(idmac_ch)) { v4l2_err(&priv->sd, "could not get IDMAC channel %u\n", ch_num); - ret = PTR_ERR(priv->idmac_ch); + ret = PTR_ERR(idmac_ch); goto out; } + priv->idmac_ch = idmac_ch; return 0; out: @@ -1583,6 +1586,7 @@ static int csi_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh, static int csi_registered(struct v4l2_subdev *sd) { struct csi_priv *priv = v4l2_get_subdevdata(sd); + struct ipu_csi *csi; int i, ret; u32 code; @@ -1590,11 +1594,12 @@ static int csi_registered(struct v4l2_subdev *sd) priv->md = dev_get_drvdata(sd->v4l2_dev->dev); /* get handle to IPU CSI */ - priv->csi = ipu_csi_get(priv->ipu, priv->csi_id); - if (IS_ERR(priv->csi)) { + csi = ipu_csi_get(priv->ipu, priv->csi_id); + if (IS_ERR(csi)) { v4l2_err(&priv->sd, "failed to get CSI%d\n", priv->csi_id); - return PTR_ERR(priv->csi); + return PTR_ERR(csi); } + priv->csi = csi; for (i = 0; i < CSI_NUM_PADS; i++) { priv->pad[i].flags = (i == CSI_SINK_PAD) ? @@ -1663,7 +1668,7 @@ static void csi_unregistered(struct v4l2_subdev *sd) if (priv->fim) imx_media_fim_free(priv->fim); - if (!IS_ERR_OR_NULL(priv->csi)) + if (priv->csi) ipu_csi_put(priv->csi); } diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c index 48cbc7716758..c58ff0831890 100644 --- a/drivers/staging/media/imx/imx-media-dev.c +++ b/drivers/staging/media/imx/imx-media-dev.c @@ -91,7 +91,7 @@ imx_media_add_async_subdev(struct imx_media_dev *imxmd, if (imx_media_find_async_subdev(imxmd, np, devname)) { dev_dbg(imxmd->md.dev, "%s: already added %s\n", __func__, np ? np->name : devname); - imxsd = NULL; + imxsd = ERR_PTR(-EEXIST); goto out; } diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c index b026fe66467c..4aac42cb79a4 100644 --- a/drivers/staging/media/imx/imx-media-of.c +++ b/drivers/staging/media/imx/imx-media-of.c @@ -115,7 +115,7 @@ of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np, /* register this subdev with async notifier */ imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL); - if (IS_ERR_OR_NULL(imxsd)) + if (IS_ERR(imxsd)) return imxsd; if (is_csi_port) { diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c index 7eabdc4aa79f..433474d58e3e 100644 --- a/drivers/staging/media/imx/imx-media-vdic.c +++ b/drivers/staging/media/imx/imx-media-vdic.c @@ -126,15 +126,15 @@ struct vdic_priv { static void vdic_put_ipu_resources(struct vdic_priv *priv) { - if (!IS_ERR_OR_NULL(priv->vdi_in_ch_p)) + if (priv->vdi_in_ch_p) ipu_idmac_put(priv->vdi_in_ch_p); priv->vdi_in_ch_p = NULL; - if (!IS_ERR_OR_NULL(priv->vdi_in_ch)) + if (priv->vdi_in_ch) ipu_idmac_put(priv->vdi_in_ch); priv->vdi_in_ch = NULL; - if (!IS_ERR_OR_NULL(priv->vdi_in_ch_n)) + if (priv->vdi_in_ch_n) ipu_idmac_put(priv->vdi_in_ch_n); priv->vdi_in_ch_n = NULL; @@ -146,40 +146,43 @@ static void vdic_put_ipu_resources(struct vdic_priv *priv) static int vdic_get_ipu_resources(struct vdic_priv *priv) { int ret, err_chan; + struct ipuv3_channel *ch; + struct ipu_vdi *vdi; priv->ipu = priv->md->ipu[priv->ipu_id]; - priv->vdi = ipu_vdi_get(priv->ipu); - if (IS_ERR(priv->vdi)) { + vdi = ipu_vdi_get(priv->ipu); + if (IS_ERR(vdi)) { v4l2_err(&priv->sd, "failed to get VDIC\n"); - ret = PTR_ERR(priv->vdi); + ret = PTR_ERR(vdi); goto out; } + priv->vdi = vdi; if (!priv->csi_direct) { - priv->vdi_in_ch_p = ipu_idmac_get(priv->ipu, - IPUV3_CHANNEL_MEM_VDI_PREV); - if (IS_ERR(priv->vdi_in_ch_p)) { + ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_PREV); + if (IS_ERR(ch)) { err_chan = IPUV3_CHANNEL_MEM_VDI_PREV; - ret = PTR_ERR(priv->vdi_in_ch_p); + ret = PTR_ERR(ch); goto out_err_chan; } + priv->vdi_in_ch_p = ch; - priv->vdi_in_ch = ipu_idmac_get(priv->ipu, - IPUV3_CHANNEL_MEM_VDI_CUR); - if (IS_ERR(priv->vdi_in_ch)) { + ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_CUR); + if (IS_ERR(ch)) { err_chan = IPUV3_CHANNEL_MEM_VDI_CUR; - ret = PTR_ERR(priv->vdi_in_ch); + ret = PTR_ERR(ch); goto out_err_chan; } + priv->vdi_in_ch = ch; - priv->vdi_in_ch_n = ipu_idmac_get(priv->ipu, - IPUV3_CHANNEL_MEM_VDI_NEXT); + ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT); if (IS_ERR(priv->vdi_in_ch_n)) { err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT; - ret = PTR_ERR(priv->vdi_in_ch_n); + ret = PTR_ERR(ch); goto out_err_chan; } + priv->vdi_in_ch_n = ch; } return 0;
While looking at a compiler warning, I noticed the use of IS_ERR_OR_NULL, which is generally a sign of a bad API design and should be avoided. In this driver, this is fairly easy, we can simply stop storing error pointers in persistent structures, and change the one function that might return either a NULL pointer or an error code to consistently return error pointers when failing. Fixes: e130291212df ("[media] media: Add i.MX media core driver") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- I can't reproduce the original warning any more, but this patch still makes sense by itself. --- drivers/staging/media/imx/imx-ic-prpencvf.c | 41 ++++++++++++++++------------- drivers/staging/media/imx/imx-media-csi.c | 29 +++++++++++--------- drivers/staging/media/imx/imx-media-dev.c | 2 +- drivers/staging/media/imx/imx-media-of.c | 2 +- drivers/staging/media/imx/imx-media-vdic.c | 37 ++++++++++++++------------ 5 files changed, 61 insertions(+), 50 deletions(-)