diff mbox series

[v2,09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()

Message ID 20200825112447.126308-10-kuhn.chenqun@huawei.com
State New
Headers show
Series trivial patchs for static code analyzer fixes | expand

Commit Message

Chenqun (kuhn) Aug. 25, 2020, 11:24 a.m. UTC
Clang static code analyzer show warning:
hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never read
        val = s->reg_set[offset >> 2];

The default value of 'val' is '0', so we can break the 'default' branch and return 'val'.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/exynos4210_combiner.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Peter Maydell Aug. 25, 2020, 12:41 p.m. UTC | #1
On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never read
>         val = s->reg_set[offset >> 2];
>
> The default value of 'val' is '0', so we can break the 'default' branch and return 'val'.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/exynos4210_combiner.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
> index b8561e4180..e2e745bbaa 100644
> --- a/hw/intc/exynos4210_combiner.c
> +++ b/hw/intc/exynos4210_combiner.c
> @@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
>              hw_error("exynos4210.combiner: overflow of reg_set by 0x"
>                      TARGET_FMT_plx "offset\n", offset);
>          }
> -        val = s->reg_set[offset >> 2];
> -        return 0;
> +        break;
>      }
>      return val;
>  }

The code as it stands is definitely wrong, but I'm not sure
this is the correct fix. Surely the intention must have been
to return the actual register value from the reg_set[] array,
not to return 0 ?

I suspect the correct fix here is simply to delete the
"return 0" line and leave the assignment to val as it is.
Ideally you should check the h/w datasheet to confirm.

thanks
-- PMM
Chenqun (kuhn) Aug. 27, 2020, 7 a.m. UTC | #2
> Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in
> exynos4210_combiner_read()
> 
> On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >
> > Clang static code analyzer show warning:
> > hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never
> read
> >         val = s->reg_set[offset >> 2];
> >
> > The default value of 'val' is '0', so we can break the 'default' branch and return
> 'val'.
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> > Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/intc/exynos4210_combiner.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/exynos4210_combiner.c
> > b/hw/intc/exynos4210_combiner.c index b8561e4180..e2e745bbaa 100644
> > --- a/hw/intc/exynos4210_combiner.c
> > +++ b/hw/intc/exynos4210_combiner.c
> > @@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr
> offset, unsigned size)
> >              hw_error("exynos4210.combiner: overflow of reg_set by 0x"
> >                      TARGET_FMT_plx "offset\n", offset);
> >          }
> > -        val = s->reg_set[offset >> 2];
> > -        return 0;
> > +        break;
> >      }
> >      return val;
> >  }
> 
> The code as it stands is definitely wrong, but I'm not sure this is the correct fix.
> Surely the intention must have been to return the actual register value from
> the reg_set[] array, not to return 0 ?
> 
> I suspect the correct fix here is simply to delete the "return 0" line and leave
> the assignment to val as it is.

Hi Peter,
  I think you're right.

> Ideally you should check the h/w datasheet to confirm.
 I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner Operation' had only five types of registers.

Register             Description                                              Type
----------------------------------------------------------------------------------------------------------------------------------------
IESR(0/1/2/3)   Interrupt Enable Set Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)         RW
IECR(0/1/2/3)   Interrupt Enable Clear Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)       RW
ISTR(0/1/2/3)   Interrupt Status Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)             R
IMSR(0/1/2/3)  Interrupt Masked Status Register for Group(0~3/ 4 ~7/ 8~ 11/ 12~ 15)       R
CIPSR0        Combined Interrupt Pending Status0                                  R

The exynos4210_combiner_write() function has write operations for IESR and IECR.
The CIPSR, ISTR, and IMSR read operations are specified in the exynos4210_combiner_read() function.

So, This 'default' branch in exynos4210_combiner_read() function read operation is only for IESR and IECR.

And, in the exynos4210_combiner_write function, the default assignment statement for IESR and IECR is " s->reg_set[offset >> 2] = val;".
Therefore, if we read the two register(IESR and IECR) values, we can directly use this assignment statement "val = s->reg_set[offset >> 2];".

Please confirm that if there is no problem, I will modify it in later version.

Thanks,
Chen Qun
Peter Maydell Aug. 27, 2020, 9:28 a.m. UTC | #3
On Thu, 27 Aug 2020 at 08:01, Chenqun (kuhn) <kuhn.chenqun@huawei.com> wrote:
>
> > Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in
> > exynos4210_combiner_read()

> > The code as it stands is definitely wrong, but I'm not sure this is the correct fix.
> > Surely the intention must have been to return the actual register value from
> > the reg_set[] array, not to return 0 ?
> >
> > I suspect the correct fix here is simply to delete the "return 0" line and leave
> > the assignment to val as it is.
>
> Hi Peter,
>   I think you're right.
>
> > Ideally you should check the h/w datasheet to confirm.
>  I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner Operation' had only five types of registers.

> Please confirm that if there is no problem, I will modify it in later version.

Thanks for checking the datasheet -- I agree and we should keep
the "val = s->reg_set[offset >> 2];" and only delete the "return 0".

-- PMM
diff mbox series

Patch

diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
index b8561e4180..e2e745bbaa 100644
--- a/hw/intc/exynos4210_combiner.c
+++ b/hw/intc/exynos4210_combiner.c
@@ -228,8 +228,7 @@  exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
             hw_error("exynos4210.combiner: overflow of reg_set by 0x"
                     TARGET_FMT_plx "offset\n", offset);
         }
-        val = s->reg_set[offset >> 2];
-        return 0;
+        break;
     }
     return val;
 }