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