Re: [PATCH v2 12/13] media: stm32: dcmipp: rename bytecap into capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The driver calculates pixel alignment requirements using an unvalidated user format, resulting in missed hardware alignment constraints and unaligned memory access.
- [Low] Stale comment references the old `dcmipp_bytecap_device` structure name.

Pre-existing issues:
- [High] The `dcmipp_capture_start_streaming` function leaks queued buffers if the source subdev cannot be found.
--

commit c041aaac664f7bb802b626e9d71a0c93fba4ced4
Author: Alain Volmat <alain.volmat@foss.st.com>
Subject: media: stm32: dcmipp: rename bytecap into capture

Since the bytecap video driver is now handling all capture (byte & pixel),
rename the file and structs into capture.

> diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-capture.c
> --- a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-capture.c
[ ... ]
> @@ -371,7 +371,7 @@ static int dcmipp_bytecap_try_fmt_vid_cap(struct file *file, void *priv,
>  	u32 in_w, in_h;
>  
>  	/* Don't accept a pixelformat that is not on the table */
> -	if (!dcmipp_bytecap_is_format_valid(vcap, format->pixelformat))
> +	if (!dcmipp_capture_is_format_valid(vcap, format->pixelformat))
>  		format->pixelformat = fmt_default.pixelformat;

[Severity: High]
Does this code fail to enforce hardware alignment if the user requests an
invalid format?

When the walign variable is computed at the start of this function, the
user's format->pixelformat hasn't been validated yet. If the format is
invalid, hdw_pixel_alignment() returns 0, leaving walign as 0. 

When the invalid format is then overridden to fmt_default.pixelformat here,
walign is never recalculated. This allows the subsequent
v4l_bound_align_image() call to use walign = 0, which could result in an
unaligned bytesperline and stride.

Could this cause a WARN_ON() in dcmipp_capture_buf_prepare() and unaligned
memory accesses in the DMA engine?

[ ... ]
> @@ -580,7 +580,7 @@ static int dcmipp_bytecap_start_streaming(struct vb2_queue *vq,
>  
>  	/*
>  	 * Get source subdev - since link is IMMUTABLE, pointer is cached
> -	 * within the dcmipp_bytecap_device structure
> +	 * within the dcmipp_capture_device structure
>  	 */
>  	if (!vcap->s_subdev) {
>  		pad = media_pad_remote_pad_first(&vcap->vdev.entity.pads[0]);
>  		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this early return leak queued buffers?

According to the videobuf2 API contract, if start_streaming fails, the driver
must return all queued buffers to the QUEUED state so userspace regains
ownership.

If media_pad_remote_pad_first() fails, this code returns -EINVAL directly
instead of going to the err_buffer_done label. This bypasses the cleanup path
and leaves the buffers stranded in vcap->buffers. Should this return be
changed to jump to a cleanup label?

[ ... ]
> @@ -1130,10 +1130,10 @@ static int dcmipp_name_to_pipe_id(const char *name)
>  		return -EINVAL;
>  }
>  
> -struct dcmipp_ent_device *dcmipp_bytecap_ent_init(const char *entity_name,
> +struct dcmipp_ent_device *dcmipp_capture_ent_init(const char *entity_name,
>  						  struct dcmipp_device *dcmipp)
>  {
> -	struct dcmipp_bytecap_device *vcap;
> +	struct dcmipp_capture_device *vcap;
>  	struct device *dev = dcmipp->dev;
>  	struct video_device *vdev;
>  	struct vb2_queue *q;
>  	const unsigned long pad_flag = MEDIA_PAD_FL_SINK;
>  	int ret = 0;
> 
>  	/* Allocate the dcmipp_bytecap_device struct */

[Severity: Low]
This isn't a bug, but should this comment also be updated to reflect the
new dcmipp_capture_device structure name?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-stm32-dcmipp-pixel-pipes-support-v2-0-3c76b5f93157@foss.st.com?part=12