linux-kernel - Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ee2qcz3ckhcvd6v5mt6cjbqdysipucqokpud76meilhplhcso@im62bwviw7x4>
Date: Thu, 8 May 2025 18:28:41 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: keke.li@...ogic.com, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-media@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kieran.bingham@...asonboard.com, 
	laurent.pinchart@...asonboard.com, dan.scally@...asonboard.com, jacopo.mondi@...asonboard.com
Subject: Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver

Hi Sakari

On Thu, May 08, 2025 at 03:44:47PM +0000, Sakari Ailus wrote:
> Hi Keke, Jacopo,
>
> On Sun, Apr 27, 2025 at 02:27:16PM +0800, Keke Li via B4 Relay wrote:
> > diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > new file mode 100644
> > index 000000000000..0e0b5d61654a
> > --- /dev/null
> > +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
>
> ...
>
> > +static int c3_isp_params_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct c3_isp_params_buffer *buf = to_c3_isp_params_buffer(vbuf);
> > +	struct c3_isp_params *params = vb2_get_drv_priv(vb->vb2_queue);
> > +	struct c3_isp_params_cfg *cfg = buf->cfg;
> > +	struct c3_isp_params_cfg *usr_cfg = vb2_plane_vaddr(vb, 0);
> > +	size_t payload_size = vb2_get_plane_payload(vb, 0);
> > +	size_t header_size = offsetof(struct c3_isp_params_cfg, data);
> > +	size_t block_offset = 0;
> > +	size_t cfg_size;
> > +
> > +	/* Payload size can't be greater than the destination buffer size */
> > +	if (payload_size > params->vfmt.fmt.meta.buffersize) {
> > +		dev_dbg(params->isp->dev,
> > +			"Payload size is too large: %zu\n", payload_size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Payload size can't be smaller than the header size */
> > +	if (payload_size < header_size) {
> > +		dev_dbg(params->isp->dev,
> > +			"Payload size is too small: %zu\n", payload_size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Use the internal scratch buffer to avoid userspace modifying
> > +	 * the buffer content while the driver is processing it.
> > +	 */
> > +	memcpy(cfg, usr_cfg, payload_size);
> > +
> > +	/* Only v0 is supported at the moment */
> > +	if (cfg->version != C3_ISP_PARAMS_BUFFER_V0) {
> > +		dev_dbg(params->isp->dev,
> > +			"Invalid params buffer version: %u\n", cfg->version);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Validate the size reported in the parameter buffer header */
> > +	cfg_size = header_size + cfg->data_size;
> > +	if (cfg_size != payload_size) {
> > +		dev_dbg(params->isp->dev,
> > +			"Data size %zu and payload size %zu are different\n",
> > +			cfg_size, payload_size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Walk the list of parameter blocks and validate them */
> > +	cfg_size = cfg->data_size;
> > +	while (cfg_size >= sizeof(struct c3_isp_params_block_header)) {
> > +		const struct c3_isp_params_block_header *block;
> > +		const struct c3_isp_params_handler *handler;
> > +
> > +		block = (struct c3_isp_params_block_header *)
> > +			&cfg->data[block_offset];
> > +
> > +		if (block->type >= ARRAY_SIZE(c3_isp_params_handlers)) {
> > +			dev_dbg(params->isp->dev,
> > +				"Invalid params block type\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (block->size > cfg_size) {
> > +			dev_dbg(params->isp->dev,
> > +				"Block size is greater than cfg size\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if ((block->flags & (C3_ISP_PARAMS_BLOCK_FL_ENABLE |
> > +				     C3_ISP_PARAMS_BLOCK_FL_DISABLE)) ==
> > +		    (C3_ISP_PARAMS_BLOCK_FL_ENABLE |
> > +		     C3_ISP_PARAMS_BLOCK_FL_DISABLE)) {
> > +			dev_dbg(params->isp->dev,
> > +				"Invalid parameters block flags\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		handler = &c3_isp_params_handlers[block->type];
> > +		if (block->size != handler->size) {
> > +			dev_dbg(params->isp->dev,
> > +				"Invalid params block size\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		block_offset += block->size;
> > +		cfg_size -= block->size;
> > +	}
> > +
> > +	if (cfg_size) {
> > +		dev_dbg(params->isp->dev,
> > +			"Unexpected data after the params buffer end\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
>
> The above looks very much like rkisp1_params_prepare_ext_params() in the

slightly similar, yes :)

> Rockchip ISP driver. Instead of copying all this non-trivial code in
> verbatim here, could you instead refactor this so both the drivers could
> use the same implementation?
>

Yeah, that's the plan.

We have more drivers in the pipeline using extensible parameters and this
code (and possibily other parts) will certainly be factored out.

My plan is to add at one more user in and the do move common parts to
the framework. Would this work for you ?


> The types are different so macros will be likely needed.
>
> --
> Regards,
>
> Sakari Ailus