| Message ID | 20200529200031.4117841-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] staging: media: atomisp: fix incorrect NULL pointer check | expand |
See also Nathan's 7 patch series. https://lore.kernel.org/lkml/20200527071150.3381228-1-natechancellor@gmail.com/ Might be some overlap between series? On Fri, May 29, 2020 at 1:00 PM Arnd Bergmann <arnd@arndb.de> wrote: > > Checking the pointer to a member of a struct against NULL > is pointless as clang points out: > > drivers/staging/media/atomisp/pci/atomisp_cmd.c:4278:17: error: address of 'config->info' will always evaluate to 'true' > > Check the original pointer instead, which may also be > unnecessary here, but makes a little more sense. > > Fixes: 9d4fa1a16b28 ("media: atomisp: cleanup directory hierarchy") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/staging/media/atomisp/pci/atomisp_cmd.c | 2 +- > drivers/staging/media/atomisp/pci/sh_css.c | 4 ++-- > drivers/staging/media/atomisp/pci/sh_css_sp.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c > index 5be690f876c1..342fc3b34fe0 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c > @@ -4275,7 +4275,7 @@ int atomisp_param(struct atomisp_sub_device *asd, int flag, > atomisp_css_get_dvs_grid_info( > &asd->params.curr_grid_info); > > - if (!&config->info) { > + if (!config) { > dev_err(isp->dev, "ERROR: NULL pointer in grid_info\n"); > return -EINVAL; > } > diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c > index d77432254a2c..e91c6029c651 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css.c > +++ b/drivers/staging/media/atomisp/pci/sh_css.c > @@ -8534,7 +8534,7 @@ ia_css_pipe_load_extension(struct ia_css_pipe *pipe, > > if (firmware->info.isp.type == IA_CSS_ACC_OUTPUT) > { > - if (&pipe->output_stage) > + if (pipe) > append_firmware(&pipe->output_stage, firmware); > else { > IA_CSS_LEAVE_ERR_PRIVATE(IA_CSS_ERR_INTERNAL_ERROR); > @@ -8542,7 +8542,7 @@ ia_css_pipe_load_extension(struct ia_css_pipe *pipe, > } > } else if (firmware->info.isp.type == IA_CSS_ACC_VIEWFINDER) > { > - if (&pipe->vf_stage) > + if (pipe) > append_firmware(&pipe->vf_stage, firmware); > else { > IA_CSS_LEAVE_ERR_PRIVATE(IA_CSS_ERR_INTERNAL_ERROR); > diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c > index e574396ad0f4..c0e579c1705f 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c > @@ -1022,7 +1022,7 @@ sh_css_sp_init_stage(struct ia_css_binary *binary, > if (!pipe) > return IA_CSS_ERR_INTERNAL_ERROR; > ia_css_get_crop_offsets(pipe, &args->in_frame->info); > - } else if (&binary->in_frame_info) > + } else if (binary) > { > pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num); > if (!pipe) > @@ -1036,7 +1036,7 @@ sh_css_sp_init_stage(struct ia_css_binary *binary, > if (!pipe) > return IA_CSS_ERR_INTERNAL_ERROR; > ia_css_get_crop_offsets(pipe, &args->in_frame->info); > - } else if (&binary->in_frame_info) { > + } else if (binary) { > pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num); > if (!pipe) > return IA_CSS_ERR_INTERNAL_ERROR; > -- > 2.26.2 > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200529200031.4117841-1-arnd%40arndb.de.
On Fri, May 29, 2020 at 10:04 PM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > See also Nathan's 7 patch series. > https://lore.kernel.org/lkml/20200527071150.3381228-1-natechancellor@gmail.com/ > > Might be some overlap between series? > Probably. I really should have checked when I saw the number of warnings. At least this gives Mauro a chance to double-check the changes and see if Nathan and I came to different conclusions on any of them. Arnd
On Fri, May 29, 2020 at 10:23 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, May 29, 2020 at 10:04 PM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > > > See also Nathan's 7 patch series. > > https://lore.kernel.org/lkml/20200527071150.3381228-1-natechancellor@gmail.com/ > > > > Might be some overlap between series? > > > > Probably. I really should have checked when I saw the number of warnings. > > At least this gives Mauro a chance to double-check the changes and see if > Nathan and I came to different conclusions on any of them. I checked now and found that the overlap is smaller than I expected. In each case, Nathans' solution seems more complete than mine, so this patch ("staging: media: atomisp: fix incorrect NULL pointer check") and also "staging: media: atomisp: fix a type conversion warning" can be dropped, but I think the others are still needed. Arnd
On Fri, May 29, 2020 at 10:31:44PM +0200, Arnd Bergmann wrote: > On Fri, May 29, 2020 at 10:23 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Fri, May 29, 2020 at 10:04 PM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > See also Nathan's 7 patch series. > > > https://lore.kernel.org/lkml/20200527071150.3381228-1-natechancellor@gmail.com/ > > > > > > Might be some overlap between series? > > > > > > > Probably. I really should have checked when I saw the number of warnings. > > > > At least this gives Mauro a chance to double-check the changes and see if > > Nathan and I came to different conclusions on any of them. > > I checked now and found that the overlap is smaller than I expected. > In each case, Nathans' solution seems more complete than mine, > so this patch ("staging: media: atomisp: fix incorrect NULL pointer check") > and also "staging: media: atomisp: fix a type conversion warning" can be > dropped, but I think the others are still needed. > > Arnd Thanks for double checking! I will read through the rest of the series and review as I can. Cheers, Nathan
Em Fri, 29 May 2020 22:31:44 +0200 Arnd Bergmann <arnd@arndb.de> escreveu: > On Fri, May 29, 2020 at 10:23 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Fri, May 29, 2020 at 10:04 PM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > See also Nathan's 7 patch series. > > > https://lore.kernel.org/lkml/20200527071150.3381228-1-natechancellor@gmail.com/ > > > > > > Might be some overlap between series? > > > > > > > Probably. I really should have checked when I saw the number of warnings. > > > > At least this gives Mauro a chance to double-check the changes and see if > > Nathan and I came to different conclusions on any of them. > > I checked now and found that the overlap is smaller than I expected. > In each case, Nathans' solution seems more complete than mine, > so this patch ("staging: media: atomisp: fix incorrect NULL pointer check") > and also "staging: media: atomisp: fix a type conversion warning" can be > dropped, but I think the others are still needed. Hi Arnd, I applied most of the patches from you. I had to add two extra patches before this one: [PATCH 5/9] staging: media: atomisp: fix stack overflow in init_pipe_defaults() And rebase it, because otherwise gcc would fail to compile here. I'm placing the patches I have so far ready for atomisp on this tree: https://git.linuxtv.org/mchehab/media-next.git/log/ Thanks, Mauro
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c index 5be690f876c1..342fc3b34fe0 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c @@ -4275,7 +4275,7 @@ int atomisp_param(struct atomisp_sub_device *asd, int flag, atomisp_css_get_dvs_grid_info( &asd->params.curr_grid_info); - if (!&config->info) { + if (!config) { dev_err(isp->dev, "ERROR: NULL pointer in grid_info\n"); return -EINVAL; } diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c index d77432254a2c..e91c6029c651 100644 --- a/drivers/staging/media/atomisp/pci/sh_css.c +++ b/drivers/staging/media/atomisp/pci/sh_css.c @@ -8534,7 +8534,7 @@ ia_css_pipe_load_extension(struct ia_css_pipe *pipe, if (firmware->info.isp.type == IA_CSS_ACC_OUTPUT) { - if (&pipe->output_stage) + if (pipe) append_firmware(&pipe->output_stage, firmware); else { IA_CSS_LEAVE_ERR_PRIVATE(IA_CSS_ERR_INTERNAL_ERROR); @@ -8542,7 +8542,7 @@ ia_css_pipe_load_extension(struct ia_css_pipe *pipe, } } else if (firmware->info.isp.type == IA_CSS_ACC_VIEWFINDER) { - if (&pipe->vf_stage) + if (pipe) append_firmware(&pipe->vf_stage, firmware); else { IA_CSS_LEAVE_ERR_PRIVATE(IA_CSS_ERR_INTERNAL_ERROR); diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c index e574396ad0f4..c0e579c1705f 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c @@ -1022,7 +1022,7 @@ sh_css_sp_init_stage(struct ia_css_binary *binary, if (!pipe) return IA_CSS_ERR_INTERNAL_ERROR; ia_css_get_crop_offsets(pipe, &args->in_frame->info); - } else if (&binary->in_frame_info) + } else if (binary) { pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num); if (!pipe) @@ -1036,7 +1036,7 @@ sh_css_sp_init_stage(struct ia_css_binary *binary, if (!pipe) return IA_CSS_ERR_INTERNAL_ERROR; ia_css_get_crop_offsets(pipe, &args->in_frame->info); - } else if (&binary->in_frame_info) { + } else if (binary) { pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num); if (!pipe) return IA_CSS_ERR_INTERNAL_ERROR;
Checking the pointer to a member of a struct against NULL is pointless as clang points out: drivers/staging/media/atomisp/pci/atomisp_cmd.c:4278:17: error: address of 'config->info' will always evaluate to 'true' Check the original pointer instead, which may also be unnecessary here, but makes a little more sense. Fixes: 9d4fa1a16b28 ("media: atomisp: cleanup directory hierarchy") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/media/atomisp/pci/atomisp_cmd.c | 2 +- drivers/staging/media/atomisp/pci/sh_css.c | 4 ++-- drivers/staging/media/atomisp/pci/sh_css_sp.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)