| Message ID | 20200825112447.126308-10-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | trivial patchs for static code analyzer fixes | expand |
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
> 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
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 --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; }
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(-)