Hi Dan,

On Wed 20 Apr 22, 10:42, Dan Carpenter wrote:
> I ran Smatch on this patch.

Thanks for doing this!

> On Fri, Apr 15, 2022 at 05:37:07PM +0200, Paul Kocialkowski wrote:
> > +void sun6i_isp_capture_configure(struct sun6i_isp_device *isp_dev)
> > +{
> > +	unsigned int width, height;
> > +	unsigned int stride_luma, stride_chroma = 0;
> > +	unsigned int stride_luma_div4, stride_chroma_div4;
> > +	const struct sun6i_isp_capture_format *format;
> > +	const struct v4l2_format_info *info;
> > +	u32 pixelformat;
> > +
> > +	sun6i_isp_capture_dimensions(isp_dev, &width, &height);
> > +	sun6i_isp_capture_format(isp_dev, &pixelformat);
> > +
> > +	format = sun6i_isp_capture_format_find(pixelformat);
> > +	if (WARN_ON(!format))
> > +		return;
> > +
> > +	sun6i_isp_load_write(isp_dev, SUN6I_ISP_MCH_SIZE_CFG_REG,
> > +			     SUN6I_ISP_MCH_SIZE_CFG_WIDTH(width) |
> > +			     SUN6I_ISP_MCH_SIZE_CFG_HEIGHT(height));
> > +
> > +	info = v4l2_format_info(pixelformat);
> > +	if (WARN_ON(!info))
> > +		return;
> > +
> > +	stride_luma = width * info->bpp[0];
> > +	stride_luma_div4 = DIV_ROUND_UP(stride_luma, 4);
> > +
> > +	if (info->comp_planes > 1) {
> > +		stride_chroma = width * info->bpp[1] / info->hdiv;
> > +		stride_chroma_div4 = DIV_ROUND_UP(stride_chroma, 4);
> 
> stride_chroma_div4 is not intialized on the else path.

One could say it's not an issue to put an uninitialized value in this situation
since the hardware won't be taking it into account but I'll initialize the value
early in the next iteration.

> > +	}
> > +
> > +	sun6i_isp_load_write(isp_dev, SUN6I_ISP_MCH_CFG_REG,
> > +			     SUN6I_ISP_MCH_CFG_EN |
> > +			     SUN6I_ISP_MCH_CFG_OUTPUT_FMT(format->output_format) |
> > +			     SUN6I_ISP_MCH_CFG_STRIDE_Y_DIV4(stride_luma_div4) |
> > +			     SUN6I_ISP_MCH_CFG_STRIDE_UV_DIV4(stride_chroma_div4));
> > +}
> 
> [ snip ]
> 
> > +void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_params_state *state = &isp_dev->params.state;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&state->lock, flags);
> > +
> > +	sun6i_isp_params_configure_base(isp_dev);
> > +
> > +	/* Default config is only applied at the very first stream start. */
> > +	if (state->configured)
> > +		goto complete;
> > +
> > +	 sun6i_isp_params_configure_modules(isp_dev,
>         ^
> There is an extra space character here.

Good catch, thanks!

> > +					    &sun6i_isp_params_config_default);
> > +
> > +	state->configured = true;
> > +
> > +complete:
> > +	spin_unlock_irqrestore(&state->lock, flags);
> > +}

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com