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

[Matteo Martelli <matteomartelli3@gmail.com>]

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.

> CJ
> 

Thanks,
Matteo Martelli