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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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
