Hi Hans, On Sun, Jul 30, 2023 at 11:33 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxx> > Closes: https://lore.kernel.org/linux-media/7c8fc5b4-280e-844e-cdf5-b6ec2a1616aa@xxxxxxxxx/ > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> -- BR, Kate
