[PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Arnd Bergmann]

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-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;
-- 
2.9.0

Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Philipp Zabel]

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

Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62559 bytes --]

Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Arnd Bergmann]

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