diff mbox series

iio: bmi323: Fix array reference in bmi323_core_runtime_suspend()

Message ID 20240909-iio-bmi323-fix-array-ref-v1-1-51c220f22229@kernel.org (mailing list archive)
State Rejected
Headers show
Series iio: bmi323: Fix array reference in bmi323_core_runtime_suspend() | expand

Commit Message

Nathan Chancellor Sept. 9, 2024, 4:38 p.m. UTC 
Clang warns (or errors with CONFIG_WERROR):

  drivers/iio/imu/bmi323/bmi323_core.c:133:27: error: variable 'bmi323_ext_reg_savestate' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
    133 | static const unsigned int bmi323_ext_reg_savestate[] = {
        |                           ^~~~~~~~~~~~~~~~~~~~~~~~

bmi323_ext_reg_savestate is only used within sizeof() through
ARRAY_SIZE(), so it is not unused, but it will not be emitted in the
final binary because sizeof() is evaluated only at compile time.
bmi323_ext_reg_savestate should have been used in the second parameter
in the call to bmi323_read_ext_reg() in the second for loop in
bmi323_core_runtime_suspend().

Fixes: 16531118ba63 ("iio: bmi323: peripheral in lowest power state on suspend")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/iio/imu/bmi323/bmi323_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 5ba0cb92584ba5e107c97001e09013c1da0772a8
change-id: 20240909-iio-bmi323-fix-array-ref-a0672a8213f0

Best regards,

Comments

Denis Benato Sept. 9, 2024, 7:10 p.m. UTC | #1 
On 09/09/24 18:38, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR):
> 
>   drivers/iio/imu/bmi323/bmi323_core.c:133:27: error: variable 'bmi323_ext_reg_savestate' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
>     133 | static const unsigned int bmi323_ext_reg_savestate[] = {
>         |                           ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> bmi323_ext_reg_savestate is only used within sizeof() through
> ARRAY_SIZE(), so it is not unused, but it will not be emitted in the
> final binary because sizeof() is evaluated only at compile time.
> bmi323_ext_reg_savestate should have been used in the second parameter
> in the call to bmi323_read_ext_reg() in the second for loop in
> bmi323_core_runtime_suspend().
> 
> Fixes: 16531118ba63 ("iio: bmi323: peripheral in lowest power state on suspend")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/iio/imu/bmi323/bmi323_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> index 671401ce80dcf947b7b64ea3af112d2a42ca5501..64dbce23ce17bcdd11c0d4c454dbeb9de17ef56c 100644
> --- a/drivers/iio/imu/bmi323/bmi323_core.c
> +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> @@ -2199,7 +2199,7 @@ static int bmi323_core_runtime_suspend(struct device *dev)
>  	}
>  
>  	for (unsigned int i = 0; i < ARRAY_SIZE(bmi323_ext_reg_savestate); i++) {
> -		ret = bmi323_read_ext_reg(data, bmi323_reg_savestate[i],
> +		ret = bmi323_read_ext_reg(data, bmi323_ext_reg_savestate[i],
>  					  &savestate->reg_settings[i]);
>  		if (ret) {
>  			dev_err(data->dev,
> 
> ---
> base-commit: 5ba0cb92584ba5e107c97001e09013c1da0772a8
> change-id: 20240909-iio-bmi323-fix-array-ref-a0672a8213f0
> 
> Best regards,
Hello Nathan,

Thank you kindly for spotting and fixing it.

Regrettably while integrating suggestions I received I also changed the patch semantic and due to my hardware not having the irq pin connected to the cpu this went unnoticed.

Best regards,
Denis Benato
Jonathan Cameron Sept. 14, 2024, 2:10 p.m. UTC | #2 
On Mon, 9 Sep 2024 21:10:35 +0200
Denis Benato <benato.denis96@gmail.com> wrote:

> On 09/09/24 18:38, Nathan Chancellor wrote:
> > Clang warns (or errors with CONFIG_WERROR):
> > 
> >   drivers/iio/imu/bmi323/bmi323_core.c:133:27: error: variable 'bmi323_ext_reg_savestate' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> >     133 | static const unsigned int bmi323_ext_reg_savestate[] = {
> >         |                           ^~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > bmi323_ext_reg_savestate is only used within sizeof() through
> > ARRAY_SIZE(), so it is not unused, but it will not be emitted in the
> > final binary because sizeof() is evaluated only at compile time.
> > bmi323_ext_reg_savestate should have been used in the second parameter
> > in the call to bmi323_read_ext_reg() in the second for loop in
> > bmi323_core_runtime_suspend().
> > 
> > Fixes: 16531118ba63 ("iio: bmi323: peripheral in lowest power state on suspend")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >  drivers/iio/imu/bmi323/bmi323_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> > index 671401ce80dcf947b7b64ea3af112d2a42ca5501..64dbce23ce17bcdd11c0d4c454dbeb9de17ef56c 100644
> > --- a/drivers/iio/imu/bmi323/bmi323_core.c
> > +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> > @@ -2199,7 +2199,7 @@ static int bmi323_core_runtime_suspend(struct device *dev)
> >  	}
> >  
> >  	for (unsigned int i = 0; i < ARRAY_SIZE(bmi323_ext_reg_savestate); i++) {
> > -		ret = bmi323_read_ext_reg(data, bmi323_reg_savestate[i],
> > +		ret = bmi323_read_ext_reg(data, bmi323_ext_reg_savestate[i],
> >  					  &savestate->reg_settings[i]);
> >  		if (ret) {
> >  			dev_err(data->dev,
> > 
> > ---
> > base-commit: 5ba0cb92584ba5e107c97001e09013c1da0772a8
> > change-id: 20240909-iio-bmi323-fix-array-ref-a0672a8213f0
> > 
> > Best regards,  
> Hello Nathan,
> 
> Thank you kindly for spotting and fixing it.
> 
> Regrettably while integrating suggestions I received I also changed the patch semantic and due to my hardware not having the irq pin connected to the cpu this went unnoticed.
> 
Slight delay on these due to the likely opening of the merge window
meaning I probably won't get another pull request in.  That's fine
as these are fixes and can go in during rc1, but until rc1 is released
I won't have the driver in my fixes tree upstream.

Anyhow upshot is that I'll sit on this and the CONFIG_PM one until
after RC1.

Thanks,

Jonathan

> Best regards,
> Denis Benato
Jonathan Cameron Sept. 14, 2024, 2:20 p.m. UTC | #3 
On Mon, 9 Sep 2024 21:10:35 +0200
Denis Benato <benato.denis96@gmail.com> wrote:

> On 09/09/24 18:38, Nathan Chancellor wrote:
> > Clang warns (or errors with CONFIG_WERROR):
> > 
> >   drivers/iio/imu/bmi323/bmi323_core.c:133:27: error: variable 'bmi323_ext_reg_savestate' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> >     133 | static const unsigned int bmi323_ext_reg_savestate[] = {
> >         |                           ^~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > bmi323_ext_reg_savestate is only used within sizeof() through
> > ARRAY_SIZE(), so it is not unused, but it will not be emitted in the
> > final binary because sizeof() is evaluated only at compile time.
> > bmi323_ext_reg_savestate should have been used in the second parameter
> > in the call to bmi323_read_ext_reg() in the second for loop in
> > bmi323_core_runtime_suspend().
> > 
> > Fixes: 16531118ba63 ("iio: bmi323: peripheral in lowest power state on suspend")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >  drivers/iio/imu/bmi323/bmi323_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> > index 671401ce80dcf947b7b64ea3af112d2a42ca5501..64dbce23ce17bcdd11c0d4c454dbeb9de17ef56c 100644
> > --- a/drivers/iio/imu/bmi323/bmi323_core.c
> > +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> > @@ -2199,7 +2199,7 @@ static int bmi323_core_runtime_suspend(struct device *dev)
> >  	}
> >  
> >  	for (unsigned int i = 0; i < ARRAY_SIZE(bmi323_ext_reg_savestate); i++) {
> > -		ret = bmi323_read_ext_reg(data, bmi323_reg_savestate[i],
> > +		ret = bmi323_read_ext_reg(data, bmi323_ext_reg_savestate[i],
> >  					  &savestate->reg_settings[i]);
This is saving it in the wrong place.
Noticed as Dan sent out another patch for this that was different.

So I'll pick up his instead.  Note that similar bug exists in resume.

Jonathan

> >  		if (ret) {
> >  			dev_err(data->dev,
> > 
> > ---
> > base-commit: 5ba0cb92584ba5e107c97001e09013c1da0772a8
> > change-id: 20240909-iio-bmi323-fix-array-ref-a0672a8213f0
> > 
> > Best regards,  
> Hello Nathan,
> 
> Thank you kindly for spotting and fixing it.
> 
> Regrettably while integrating suggestions I received I also changed the patch semantic and due to my hardware not having the irq pin connected to the cpu this went unnoticed.
> 
> Best regards,
> Denis Benato
diff mbox series

Patch

diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
index 671401ce80dcf947b7b64ea3af112d2a42ca5501..64dbce23ce17bcdd11c0d4c454dbeb9de17ef56c 100644
--- a/drivers/iio/imu/bmi323/bmi323_core.c
+++ b/drivers/iio/imu/bmi323/bmi323_core.c
@@ -2199,7 +2199,7 @@  static int bmi323_core_runtime_suspend(struct device *dev)
 	}
 
 	for (unsigned int i = 0; i < ARRAY_SIZE(bmi323_ext_reg_savestate); i++) {
-		ret = bmi323_read_ext_reg(data, bmi323_reg_savestate[i],
+		ret = bmi323_read_ext_reg(data, bmi323_ext_reg_savestate[i],
 					  &savestate->reg_settings[i]);
 		if (ret) {
 			dev_err(data->dev,