[PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage
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
---
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)
Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage
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
> ---
> 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
Tested-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage
On Thu, Jun 29, 2017 at 11:13 AM, Philipp Zabel 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
> Tested-by: Philipp Zabel
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
