| 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
| ||
|
Message-ID: <ihqj5zq34qyilswcyiiiefur4bm2k4v4sd7tujh6oawtw5gwra@h63zl25azvgm>
Date: Fri, 9 May 2025 10:00:38 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 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
Subject: Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver
Hi Sakari
On Thu, May 08, 2025 at 09:00:54PM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, May 08, 2025 at 06:28:41PM +0200, Jacopo Mondi wrote:
> > 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 ?
>
> How about refactoring the two users to use a common framework first and
> then using it in the third? :-) The more copies there are, the greater the
> task to unify them.
>
Sorry, I probably wasn't clear.
When upstreaming the third user of extensible parameters I will move
the common parts to the framework and port these two existing users to
use the new helpers.
It's not my intention to upstream a third user and then port all of
them to use a common helper later on ;)
> --
> Regards,
>
> Sakari Ailus
Powered by blists - more mailing lists