[PATCH v11 18/19] media: platform: Add mali-c55 parameters video node

[Dan Scally]

Hi Sakari, thanks for the comments

On 30/07/2025 15:36, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the update.
>
> On Mon, Jul 14, 2025 at 04:06:44PM +0100, Daniel Scally wrote:
>> +static int
>> +mali_c55_params_validate_buffer(struct device *dev,
>> +				const struct v4l2_params_buffer *buffer)
>> +{
>> +	/* Only v1 is supported at the moment. */
>> +	if (buffer->version != MALI_C55_PARAM_BUFFER_V1) {
>> +		dev_dbg(dev, "Unsupported extensible format version: %u\n",
>> +			buffer->version);
>> +		return -EINVAL;
>> +	}
> Is there anything else to validate here?


Not at this stage; the rest of the validation is done by Jacopo's new framework

>
> I guess nothing is done with the information other than the value is being
> checked above, but if it had an effect on something, one would need to copy
> the information to memory not accessible to the user.


Thats done later in .buf_prepare()

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
>> +{
>> +	struct mali_c55_params *params = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +	struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
>> +	struct mali_c55 *mali_c55 = params->mali_c55;
>> +	struct v4l2_params_buffer *config;
>> +	int ret;
>> +
>> +	ret = v4l2_params_buffer_validate(
>> +		mali_c55->dev, vb,
>> +		v4l2_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
>> +		mali_c55_params_validate_buffer);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Copy the parameters buffer provided by userspace to the internal
>> +	 * scratch buffer. This protects against the chance of userspace making
>> +	 * changed to the buffer content whilst the driver processes it.
>> +	 */
>> +	config = vb2_plane_vaddr(vb, 0);
>> +	memcpy(buf->config, config, v4l2_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>> +
>> +	return v4l2_params_blocks_validate(mali_c55->dev, buf->config,
>> +					   mali_c55_block_handlers,
>> +					   ARRAY_SIZE(mali_c55_block_handlers),
>> +					   NULL);
>> +}
>> +
>> +static void mali_c55_params_buf_queue(struct vb2_buffer *vb)
>> +{
>> +	struct mali_c55_params *params = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +	struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
>> +
>> +	spin_lock(&params->buffers.lock);
>> +	list_add_tail(&buf->queue, &params->buffers.queue);
>> +	spin_unlock(&params->buffers.lock);
>> +}
>> +
>> +static void mali_c55_params_return_buffers(struct mali_c55_params *params,
>> +					   enum vb2_buffer_state state)
>> +{
>> +	struct mali_c55_params_buf *buf, *tmp;
>> +
>> +	guard(spinlock)(&params->buffers.lock);
>> +
>> +	list_for_each_entry_safe(buf, tmp, &params->buffers.queue, queue) {
>> +		list_del(&buf->queue);
>> +		vb2_buffer_done(&buf->vb.vb2_buf, state);
>> +	}
>> +}
>> +
>> +static int mali_c55_params_start_streaming(struct vb2_queue *q,
>> +					   unsigned int count)
>> +{
>> +	struct mali_c55_params *params = vb2_get_drv_priv(q);
>> +	struct mali_c55 *mali_c55 = params->mali_c55;
>> +	int ret;
>> +
>> +	ret = pm_runtime_resume_and_get(mali_c55->dev);
>> +	if (ret)
>> +		goto err_return_buffers;
>> +
>> +	ret = video_device_pipeline_alloc_start(&params->vdev);
>> +	if (ret)
>> +		goto err_pm_put;
>> +
>> +	ret = video_device_pipeline_started(&params->vdev);
>> +	if (ret < 0)
>> +		goto err_stop_pipeline;
>> +
>> +	return 0;
>> +
>> +err_stop_pipeline:
>> +	video_device_pipeline_stop(&params->vdev);
>> +err_pm_put:
>> +	pm_runtime_put(mali_c55->dev);
>> +err_return_buffers:
>> +	mali_c55_params_return_buffers(params, VB2_BUF_STATE_QUEUED);
>> +
>> +	return ret;
>> +}
>> +
>> +static void mali_c55_params_stop_streaming(struct vb2_queue *q)
>> +{
>> +	struct mali_c55_params *params = vb2_get_drv_priv(q);
>> +	struct media_pipeline *pipe;
>> +
>> +	pipe = video_device_pipeline(&params->vdev);
>> +	if (mali_c55_pipeline_ready(pipe))
>> +		media_pipeline_stopped(pipe);
>> +
>> +	video_device_pipeline_stop(&params->vdev);
>> +	mali_c55_params_return_buffers(params, VB2_BUF_STATE_ERROR);
>> +}
>> +
>> +static const struct vb2_ops mali_c55_params_vb2_ops = {
>> +	.queue_setup = mali_c55_params_queue_setup,
>> +	.buf_init = mali_c55_params_buf_init,
>> +	.buf_cleanup = mali_c55_params_buf_cleanup,
>> +	.buf_queue = mali_c55_params_buf_queue,
>> +	.buf_prepare = mali_c55_params_buf_prepare,
>> +	.wait_prepare = vb2_ops_wait_prepare,
>> +	.wait_finish = vb2_ops_wait_finish,
>> +	.start_streaming = mali_c55_params_start_streaming,
>> +	.stop_streaming = mali_c55_params_stop_streaming,
>> +};
>> +
>> +void mali_c55_params_write_config(struct mali_c55 *mali_c55)
>> +{
>> +	struct mali_c55_params *params = &mali_c55->params;
>> +	struct v4l2_params_buffer *config;
>> +	struct mali_c55_params_buf *buf;
>> +	size_t block_offset = 0;
>> +	size_t max_offset;
>> +
>> +	spin_lock(&params->buffers.lock);
>> +
>> +	buf = list_first_entry_or_null(&params->buffers.queue,
>> +				       struct mali_c55_params_buf, queue);
>> +	if (buf)
>> +		list_del(&buf->queue);
>> +	spin_unlock(&params->buffers.lock);
>> +
>> +	if (!buf)
>> +		return;
>> +
>> +	buf->vb.sequence = mali_c55->isp.frame_sequence;
>> +	config = buf->config;
>> +
>> +	max_offset = config->data_size - sizeof(struct v4l2_params_block_header);
>> +
>> +	/*
>> +	 * Walk the list of parameter blocks and process them. No validation is
>> +	 * done here, as the contents of the config buffer are already checked
>> +	 * when the buffer is queued.
>> +	 */
>> +	while (block_offset < max_offset) {
>> +		const struct v4l2_params_handler *block_handler;
>> +		union mali_c55_params_block block;
>> +
>> +		block.data = &config->data[block_offset];
>> +
>> +		/* We checked the array index already in .buf_queue() */
> Not a lot seems to be done there in terms of validation.
>
> Even if that had been done, you can't trust the buffer contents as it
> remains mapped to the userspace.

Ah; a hangover of an earlier iteration before using the new v4l2-params helpers; the validation is 
done by those functions in .buf_prepare() now and the userspace buffer is memcopied to a scratch buffer.


Thanks

Dan

>> +		block_handler = &mali_c55_block_handlers[block.header->type];
>> +		block_handler->handler(mali_c55, block.header);
>> +
>> +		block_offset += block.header->size;
>> +	}
>> +
>> +	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> +}