Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli <matteomartelli3@gmail.com>]

Matteo Martelli wrote:
> Christophe JAILLET wrote:
> > Le 09/08/2024 à 09:31, Matteo Martelli a écrit :
> > > Christophe JAILLET wrote:
> > >> Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> > >>> This error path was intended to return, and not just print an error.  The
> > >>> current code will lead to an error pointer dereference.
> > >>>
> > >>> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > >>> Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >>> ---
> > >>>    drivers/iio/adc/pac1921.c | 4 ++--
> > >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > >>> index d04c6685d780..8200a47bdf21 100644
> > >>> --- a/drivers/iio/adc/pac1921.c
> > >>> +++ b/drivers/iio/adc/pac1921.c
> > >>> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > >>>    
> > >>>    	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > >>>    	if (IS_ERR(priv->regmap))
> > >>> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > >>> -			      "Cannot initialize register map\n");
> > >>> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > >>
> > >> The (int) is unusual.
> > >>
> > > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > > takes an int as argument.
> > 
> > Ok, but:
> > 
> > 1) With the cast removed, on my x86_64:
> > 	$ make CFLAGS="-Wconversion" drivers/iio/adc/pac1921.o
> > 
> > doesn't generate any error.
> > 
> I can't reproduce the warning in that way either, but maybe CFLAGS gets
> overridden in that case because with the following method I can see the
> warning:
> 
> $ print "CFLAGS_pac1921.o := -Wconversion" >> drivers/iio/adc/Makefile
> $ print "CONFIG_IIO=y\nCONFIG_PAC1921=y" >> arch/x86/configs/x86_64_defconfig
> $ sed -i 's/CONFIG_WERROR=y/CONFIG_WERROR=n/g' arch/x86/configs/x86_64_defconfig
> $ make x86_64_defconfig
> $ make -j7
> 
> drivers/iio/adc/pac1921.c: In function ‘pac1921_probe’:
> drivers/iio/adc/pac1921.c:1171:36: warning: conversion from ‘long int’ to ‘int’ may change value [-Wconversion]
>  1171 |                 dev_err_probe(dev, PTR_ERR(priv->regmap),
>       |                                    ^~~~~~~~~~~~~~~~~~~~~
> 
> Built with gcc version: gcc version 14.1.1 20240522 (GCC)
> 
> Same thing building for aarch64 with gcc version 12.2.0 (Debian 12.2.0-14)
> 
> > 2)
> > 	$ it grep dev_err_probe.*\)PTR_ERR | wc -l
> > 	2
> > 
> > 	$ it grep dev_err_probe.*PTR_ERR | wc -l
> > 	1948
> > So, should the cast be needed, maybe another fix could make sense?
> >
> It could be assigned to the ret value if that would be preferred:
> 	if (IS_ERR(priv->regmap)) {
> 		ret = (int)PTR_ERR(priv->regmap);
> 		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
> 	}
>
> Otherwise a more generic approach could be to let PTR_ERR directly cast to
> (int). I would say that if it is always called after an IS_ERR() it should be
> safe to cast to (int) since the latter should guarantee the pointer value is
> inside int size boundaries. The similar PTR_ERR_OR_ZERO also casts (implicitly)
> to int but it also checks for IS_ERR before the cast.
> Maybe another solution could be introducing a new macro that does the cast but
> before it checks the ptr with IS_ERR(), I came up with the following even
> though it doesn't look very idiomatic:
> 
> #define WITH_PTR_ERR(ret, ptr) if (IS_ERR(ptr) && (ret = (int)PTR_ERR(ptr)))
> ...
> static int pac1921_probe(struct i2c_client *client)
> {
>         ...
> 	WITH_PTR_ERR(ret, priv->regmap) {
> 		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
> 	}
> }
> 
> Maybe there is already some similar use case?
> 
> Anyway, if in general it is preferred to avoid the explicit cast despite the
> Wconversion warning I would be fine with it.
>

Adding another simple alternative I didn't think of before:
        ...
	ret = PTR_ERR_OR_ZERO(priv->regmap);
	if (ret)
		return dev_err_probe(dev, ret, "Cannot initialize register map\n");

The warning would still be produced due to the implicit cast inside
PTR_ERR_OR_ZERO but it could be fixed for all users with an explicit cast if
there will be interest in future to do so.

Also used a bit around:
grep -R -A 3 'ret = PTR_ERR_OR_ZERO' . | grep -e '_err.*(' | wc -l
69

Thanks,
Matteo Martelli