| Message ID | 20230730153343.22033-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] media: atomisp: Fix smatch warnings caused by atomisp custom assert() usage | expand |
Hi Hans, On Sun, Jul 30, 2023 at 11:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The current error-checking of me->stages in sh_css_sp_init_pipeline() > has some issues / weirdness: > > 1. It is checked at the top of the function, but only using the atomisp > custom assert() macro which e.g. smatch does not recognize > > 2. It is first dereferenced in "first_binary = me->stages->binary", but > outside of the assert it is checked much later, triggering the following > smatch warning: > > drivers/staging/media/atomisp/pci/sh_css_sp.c:1255 sh_css_sp_init_pipeline() > warn: variable dereferenced before check 'me->stages' (see line 1224) > > Drop the custom assert() calls (note 'me' is never NULL) and instead add > a regular check for me->stages not being set. > > Reported-by: Hans Verkuil <hverkuil@xs4all.nl> > Closes: https://lore.kernel.org/linux-media/7c8fc5b4-280e-844e-cdf5-b6ec2a1616aa@xs4all.nl/ > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/staging/media/atomisp/pci/sh_css_sp.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c > index 4ef1c9e61ea8..eba70d4800a1 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c > @@ -51,6 +51,7 @@ > #include "ia_css_event.h" > #include "mmu_device.h" > #include "ia_css_spctrl.h" > +#include "atomisp_internal.h" > > #ifndef offsetof > #define offsetof(T, x) ((unsigned int)&(((T *)0)->x)) > @@ -1210,14 +1211,15 @@ sh_css_sp_init_pipeline(struct ia_css_pipeline *me, > struct ia_css_binary *first_binary = NULL; > struct ia_css_pipe *pipe = NULL; > unsigned int num; > - > enum ia_css_pipe_id pipe_id = id; > unsigned int thread_id; > u8 if_config_index, tmp_if_config_index; > > - assert(me); > - > - assert(me->stages); > + if (!me->stages) { > + dev_err(atomisp_dev, "%s called on a pipeline without stages\n", > + __func__); > + return; /* FIXME should be able to return an error */ Agree, an error handling is needed for the caller. > + } > > first_binary = me->stages->binary; > > @@ -1250,8 +1252,8 @@ sh_css_sp_init_pipeline(struct ia_css_pipeline *me, > } /* if (first_binary != NULL) */ > > /* Signal the host immediately after start for SP_ISYS_COPY only */ > - if ((me->num_stages == 1) && me->stages && > - (me->stages->sp_func == IA_CSS_PIPELINE_ISYS_COPY)) > + if (me->num_stages == 1 && > + me->stages->sp_func == IA_CSS_PIPELINE_ISYS_COPY) > sh_css_sp_group.config.no_isp_sync = true; > > /* Init stage data */ > -- > 2.41.0 > It is good for me. Reviewed-by: Kate Hsuan <hpa@redhat.com> -- BR, Kate
diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c index 4ef1c9e61ea8..eba70d4800a1 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c @@ -51,6 +51,7 @@ #include "ia_css_event.h" #include "mmu_device.h" #include "ia_css_spctrl.h" +#include "atomisp_internal.h" #ifndef offsetof #define offsetof(T, x) ((unsigned int)&(((T *)0)->x)) @@ -1210,14 +1211,15 @@ sh_css_sp_init_pipeline(struct ia_css_pipeline *me, struct ia_css_binary *first_binary = NULL; struct ia_css_pipe *pipe = NULL; unsigned int num; - enum ia_css_pipe_id pipe_id = id; unsigned int thread_id; u8 if_config_index, tmp_if_config_index; - assert(me); - - assert(me->stages); + if (!me->stages) { + dev_err(atomisp_dev, "%s called on a pipeline without stages\n", + __func__); + return; /* FIXME should be able to return an error */ + } first_binary = me->stages->binary; @@ -1250,8 +1252,8 @@ sh_css_sp_init_pipeline(struct ia_css_pipeline *me, } /* if (first_binary != NULL) */ /* Signal the host immediately after start for SP_ISYS_COPY only */ - if ((me->num_stages == 1) && me->stages && - (me->stages->sp_func == IA_CSS_PIPELINE_ISYS_COPY)) + if (me->num_stages == 1 && + me->stages->sp_func == IA_CSS_PIPELINE_ISYS_COPY) sh_css_sp_group.config.no_isp_sync = true; /* Init stage data */
The current error-checking of me->stages in sh_css_sp_init_pipeline() has some issues / weirdness: 1. It is checked at the top of the function, but only using the atomisp custom assert() macro which e.g. smatch does not recognize 2. It is first dereferenced in "first_binary = me->stages->binary", but outside of the assert it is checked much later, triggering the following smatch warning: drivers/staging/media/atomisp/pci/sh_css_sp.c:1255 sh_css_sp_init_pipeline() warn: variable dereferenced before check 'me->stages' (see line 1224) Drop the custom assert() calls (note 'me' is never NULL) and instead add a regular check for me->stages not being set. Reported-by: Hans Verkuil <hverkuil@xs4all.nl> Closes: https://lore.kernel.org/linux-media/7c8fc5b4-280e-844e-cdf5-b6ec2a1616aa@xs4all.nl/ Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/media/atomisp/pci/sh_css_sp.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)