[PATCH] media: atomisp: ov2722: flush buffered writes before they overflow

[PATCH] media: atomisp: ov2722: flush buffered writes before they overflow

[Pengpeng Hou]

__ov2722_buf_reg_array() appends 8-bit or 16-bit values to the buffered
register-write payload and only checks whether it should flush after the
new value has already been written. When ctrl->index points at the last
byte of the fixed 30-byte data buffer and the next register is 16-bit,
the helper writes one byte past the end of the local buffer before the
flush threshold check runs.

Check whether the next value fits before writing it. If not, flush the
current buffered write first and then append the new value.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 .../media/atomisp/i2c/atomisp-ov2722.c        | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index 2c41c496daa6..691192d096fe 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -169,26 +169,42 @@ static int __ov2722_buf_reg_array(struct i2c_client *client,
 				  const struct ov2722_reg *next)
 {
 	int size;
+	int ret;
 	__be16 *data16;
 
 	switch (next->type) {
 	case OV2722_8BIT:
 		size = 1;
-		ctrl->buffer.data[ctrl->index] = (u8)next->val;
 		break;
 	case OV2722_16BIT:
 		size = 2;
-		data16 = (void *)&ctrl->buffer.data[ctrl->index];
-		*data16 = cpu_to_be16((u16)next->val);
 		break;
 	default:
 		return -EINVAL;
 	}
 
+	if (ctrl->index + size > OV2722_MAX_WRITE_BUF_SIZE) {
+		ret = __ov2722_flush_reg_array(client, ctrl);
+		if (ret)
+			return ret;
+	}
+
 	/* When first item is added, we need to store its starting address */
 	if (ctrl->index == 0)
 		ctrl->buffer.addr = next->reg;
 
+	switch (next->type) {
+	case OV2722_8BIT:
+		ctrl->buffer.data[ctrl->index] = (u8)next->val;
+		break;
+	case OV2722_16BIT:
+		data16 = (void *)&ctrl->buffer.data[ctrl->index];
+		*data16 = cpu_to_be16((u16)next->val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	ctrl->index += size;
 
 	/*
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] media: atomisp: ov2722: flush buffered writes before they overflow

[Dan Carpenter]

On Mon, Mar 23, 2026 at 08:17:30PM +0800, Pengpeng Hou wrote:
> __ov2722_buf_reg_array() appends 8-bit or 16-bit values to the buffered
> register-write payload and only checks whether it should flush after the
> new value has already been written. When ctrl->index points at the last
> byte of the fixed 30-byte data buffer and the next register is 16-bit,
> the helper writes one byte past the end of the local buffer before the
> flush threshold check runs.
> 
> Check whether the next value fits before writing it. If not, flush the
> current buffered write first and then append the new value.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

The patch is wrong and just adds dead code.

The function is called in a loop.  The buffer has 30 bytes.  We
write either 1 or 2 bytes.  When the index reaches 28 at the end
of the function then we write the buffer and reset the ctrl->index to
zero.  So the new check for if ctrl->index is larger than 28 or 29
at the start of the function will never trigger.

We never actually write to the last two bytes in the array.  So the
original code is off by one in that sense, I suppose?  The >= should
be > as you wrote your code.  Not a huge deal?

This patch has no information about how the bug was identified or how
the patch tested.  If you put a note to say that "This was found
via static analysis" it changes how we review the code.  Or a note to
say that "this has not been tested" then that also is good information
for reviewers.

There is also no Fixes tag.  Adding a Fixes tag helps you figure out
how the bug was introduced or what the original author may have been
thinking.  It's also necessary for the review process and the stable
process.

regards,
dan carpenter

Re: [PATCH] media: atomisp: ov2722: flush buffered writes before they overflow

[Andy Shevchenko]

On Mon, Mar 23, 2026 at 04:18:04PM +0300, Dan Carpenter wrote:
> On Mon, Mar 23, 2026 at 08:17:30PM +0800, Pengpeng Hou wrote:
> > __ov2722_buf_reg_array() appends 8-bit or 16-bit values to the buffered
> > register-write payload and only checks whether it should flush after the
> > new value has already been written. When ctrl->index points at the last
> > byte of the fixed 30-byte data buffer and the next register is 16-bit,
> > the helper writes one byte past the end of the local buffer before the
> > flush threshold check runs.
> > 
> > Check whether the next value fits before writing it. If not, flush the
> > current buffered write first and then append the new value.
> > 
> > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> 
> The patch is wrong and just adds dead code.

I believe the idea is not to touch sensor drivers in atomisp at all.
They all should be heavily lifted and updated and moved to generic
folder for all v4l2 cases. I believe Hans had some plans, not sure
what is the state with that now (I know he is busy with something
else).

-- 
With Best Regards,
Andy Shevchenko