diff mbox series

iio: adc: pac1921: add missing error return in probe()

Message ID 1fa4ab12-0939-477d-bc92-306fd32e4fd9@stanley.mountain (mailing list archive)
State Accepted
Headers show
Series iio: adc: pac1921: add missing error return in probe() | expand

Commit Message

Dan Carpenter Aug. 8, 2024, 7:28 p.m. UTC
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@linaro.org>
---
 drivers/iio/adc/pac1921.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christophe JAILLET Aug. 9, 2024, 6:18 a.m. UTC | #1
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.

CJ

> +				     "Cannot initialize register map\n");
>   
>   	devm_mutex_init(dev, &priv->lock);
>
Matteo Martelli Aug. 9, 2024, 7:19 a.m. UTC | #2
Dan Carpenter wrote:
> 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@linaro.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),
> +				     "Cannot initialize register map\n");
>  
>  	devm_mutex_init(dev, &priv->lock);
>  
> -- 
> 2.43.0
> 

The intent was indeed to return the error. Thanks for catching it.
Acked-by: Matteo Martelli <matteomartelli3@gmail.com>

Thanks,
Matteo Martelli
Matteo Martelli Aug. 9, 2024, 7:31 a.m. UTC | #3
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.
> 
> CJ
> 
> > +				     "Cannot initialize register map\n");
> >   
> >   	devm_mutex_init(dev, &priv->lock);
> >   
> 

Thanks,
Matteo Martelli
Christophe JAILLET Aug. 9, 2024, 8:41 a.m. UTC | #4
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.

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?

CJ

>>
>> CJ
>>
>>> +				     "Cannot initialize register map\n");
>>>    
>>>    	devm_mutex_init(dev, &priv->lock);
>>>    
>>
> 
> Thanks,
> Matteo Martelli
>
Dan Carpenter Aug. 9, 2024, 3:18 p.m. UTC | #5
On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> 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.

I don't want to remove the int because it's unrelated, but Christophe is right
that the int is unusual.  We really would want to discourage that.

regards,
dan carpenter
Matteo Martelli Aug. 9, 2024, 3:51 p.m. UTC | #6
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
Matteo Martelli Aug. 9, 2024, 5:04 p.m. UTC | #7
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
Dan Carpenter Aug. 9, 2024, 5:27 p.m. UTC | #8
Just delete the -Wconversion?  I'm not really familiar that option but it sounds
an even worse version of -Wsign-compare.

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

regards,
dan carpenter
Jonathan Cameron Aug. 10, 2024, 10:35 a.m. UTC | #9
On Fri, 9 Aug 2024 18:18:13 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> > 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.  
> 
> I don't want to remove the int because it's unrelated, but Christophe is right
> that the int is unusual.  We really would want to discourage that.

Applied, but I'd ideally like a follow up patch removing the int and the
couple of similar instances from this driver.  Anyone want to spin one?

Thanks,

Jonathan

> 
> regards,
> dan carpenter
>
Matteo Martelli Sept. 11, 2024, 9:32 a.m. UTC | #10
Quoting Jonathan Cameron (2024-08-10 12:35:18)
> On Fri, 9 Aug 2024 18:18:13 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> > > 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.  
> > 
> > I don't want to remove the int because it's unrelated, but Christophe is right
> > that the int is unusual.  We really would want to discourage that.
> 
> Applied, but I'd ideally like a follow up patch removing the int and the
> couple of similar instances from this driver.  Anyone want to spin one?
> 

I can spin the patch, but at this point I am wondering whether I should remove all
the unnecessary explicit casts that I put to address Wconversion
and Wsign-compare warnings. If that's the general approach to help readability I
am totally fine with it.

> Thanks,
> 
> Jonathan
> 

Thanks,
Matteo Martelli
Jonathan Cameron Sept. 13, 2024, 7:39 p.m. UTC | #11
On Wed, 11 Sep 2024 11:32:04 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Quoting Jonathan Cameron (2024-08-10 12:35:18)
> > On Fri, 9 Aug 2024 18:18:13 +0300
> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >   
> > > On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:  
> > > > 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.    
> > > 
> > > I don't want to remove the int because it's unrelated, but Christophe is right
> > > that the int is unusual.  We really would want to discourage that.  
> > 
> > Applied, but I'd ideally like a follow up patch removing the int and the
> > couple of similar instances from this driver.  Anyone want to spin one?
> >   
> 
> I can spin the patch, but at this point I am wondering whether I should remove all
> the unnecessary explicit casts that I put to address Wconversion
> and Wsign-compare warnings. If that's the general approach to help readability I
> am totally fine with it.

I think it is probably sensible to do so as mostly we know the value ranges
etc so they don't matter.

Jonathan

> 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> Thanks,
> Matteo Martelli
diff mbox series

Patch

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),
+				     "Cannot initialize register map\n");
 
 	devm_mutex_init(dev, &priv->lock);