| 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: <81e2d12c-342b-4b88-88a0-3e24115541aa@kernel.org>
Date: Sun, 4 Jan 2026 19:38:48 +0100
From: Hans de Goede <hansg@...nel.org>
To: Karthikey D Kadati <karthikey3608@...il.com>, mchehab@...nel.org
Cc: sakari.ailus@...ux.intel.com, linux-media@...r.kernel.org,
linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: atomisp: replace shadow zoom structs with
v4l2_rect and fix error paths
Hi Karthikey,
On 4-Jan-26 17:40, Karthikey D Kadati wrote:
> This patch addresses a TODO graduation blocker by removing private zoom
> structures (`atomisp_zoom_point` and `atomisp_zoom_region`) in favor of
> standard V4L2 types (`v4l2_rect`).
>
> It also improves error propagation for IRQs and XNR configuration, ensuring
> that failures are detected and reported. Additionally, it consolidates
> memory allocation boilerplate into a safer helper function
> (`atomisp_alloc_stat_bufs_list`) that includes a proper error-unwind path
> to prevent memory leaks during partial allocation failures.
It looks like you are doing a lot of unrelated changes in a single
patch. Please split this up into multiple patches each only making
a single change:
1. Replace shadow zoom structs with v4l2_rect
2. Add atomisp_alloc_stat_bufs_list() helper function (and use it)
3. Error handling fixes
Regards,
Hans
>
> Signed-off-by: Karthikey D Kadati <karthikey3608@...il.com>
> ---
> .../media/atomisp/include/linux/atomisp.h | 19 +--
> .../staging/media/atomisp/pci/atomisp_cmd.c | 144 ++++++++---------
> .../staging/media/atomisp/pci/atomisp_ioctl.c | 147 +++++++++++++-----
> 3 files changed, 183 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
> index fcf116cc4..e86f636d2 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
> @@ -326,27 +326,14 @@ struct atomisp_resolution {
> u32 height; /** Height */
> };
>
> -/*
> - * This specifies the coordinates (x,y)
> - */
> -struct atomisp_zoom_point {
> - s32 x; /** x coordinate */
> - s32 y; /** y coordinate */
> -};
> +#include <linux/videodev2.h>
>
> -/*
> - * This specifies the region
> - */
> -struct atomisp_zoom_region {
> - struct atomisp_zoom_point
> - origin; /* Starting point coordinates for the region */
> - struct atomisp_resolution resolution; /* Region resolution */
> -};
> +/* Use v4l2_rect instead of shadow structures */
>
> struct atomisp_dz_config {
> u32 dx; /** Horizontal zoom factor */
> u32 dy; /** Vertical zoom factor */
> - struct atomisp_zoom_region zoom_region; /** region for zoom */
> + struct v4l2_rect zoom_region; /** region for zoom */
> };
>
> struct atomisp_parm {
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 327836372..76a65f379 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -874,7 +874,9 @@ void atomisp_assert_recovery_work(struct work_struct *work)
> if (!isp->asd.streaming)
> goto out_unlock;
>
> - atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
> + if (atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF,
> + false))
> + dev_err_once(isp->dev, "atomisp_css_irq_enable failed\n");
>
> spin_lock_irqsave(&isp->lock, flags);
> isp->asd.streaming = false;
> @@ -925,8 +927,9 @@ void atomisp_assert_recovery_work(struct work_struct *work)
>
> atomisp_csi2_configure(&isp->asd);
>
> - atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF,
> - atomisp_css_valid_sof(isp));
> + if (atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF,
> + atomisp_css_valid_sof(isp)))
> + dev_err_once(isp->dev, "atomisp_css_irq_enable failed\n");
>
> if (atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_AUTO, true) < 0)
> dev_dbg(isp->dev, "DFS auto failed while recovering!\n");
> @@ -1196,9 +1199,7 @@ int atomisp_xnr(struct atomisp_sub_device *asd, int flag,
> return 0;
> }
>
> - atomisp_css_capture_enable_xnr(asd, !!*xnr_enable);
> -
> - return 0;
> + return atomisp_css_capture_enable_xnr(asd, !!*xnr_enable);
> }
>
> /*
> @@ -1764,15 +1765,13 @@ int atomisp_calculate_real_zoom_region(struct atomisp_sub_device *asd,
> return -EINVAL;
> }
>
> - if (dz_config->zoom_region.resolution.width
> - == asd->sensor_array_res.width
> - || dz_config->zoom_region.resolution.height
> - == asd->sensor_array_res.height) {
> + if (dz_config->zoom_region.width == asd->sensor_array_res.width ||
> + dz_config->zoom_region.height == asd->sensor_array_res.height) {
> /*no need crop region*/
> - dz_config->zoom_region.origin.x = 0;
> - dz_config->zoom_region.origin.y = 0;
> - dz_config->zoom_region.resolution.width = eff_res.width;
> - dz_config->zoom_region.resolution.height = eff_res.height;
> + dz_config->zoom_region.left = 0;
> + dz_config->zoom_region.top = 0;
> + dz_config->zoom_region.width = eff_res.width;
> + dz_config->zoom_region.height = eff_res.height;
> return 0;
> }
>
> @@ -1783,18 +1782,18 @@ int atomisp_calculate_real_zoom_region(struct atomisp_sub_device *asd,
> */
>
> if (!IS_ISP2401) {
> - dz_config->zoom_region.origin.x = dz_config->zoom_region.origin.x
> - * eff_res.width
> - / asd->sensor_array_res.width;
> - dz_config->zoom_region.origin.y = dz_config->zoom_region.origin.y
> - * eff_res.height
> - / asd->sensor_array_res.height;
> - dz_config->zoom_region.resolution.width = dz_config->zoom_region.resolution.width
> - * eff_res.width
> - / asd->sensor_array_res.width;
> - dz_config->zoom_region.resolution.height = dz_config->zoom_region.resolution.height
> - * eff_res.height
> - / asd->sensor_array_res.height;
> + dz_config->zoom_region.left =
> + (s32)((long long)dz_config->zoom_region.left *
> + eff_res.width / asd->sensor_array_res.width);
> + dz_config->zoom_region.top =
> + (s32)((long long)dz_config->zoom_region.top *
> + eff_res.height / asd->sensor_array_res.height);
> + dz_config->zoom_region.width =
> + (u32)((long long)dz_config->zoom_region.width *
> + eff_res.width / asd->sensor_array_res.width);
> + dz_config->zoom_region.height =
> + (u32)((long long)dz_config->zoom_region.height *
> + eff_res.height / asd->sensor_array_res.height);
> /*
> * Set same ratio of crop region resolution and current pipe output
> * resolution
> @@ -1821,62 +1820,67 @@ int atomisp_calculate_real_zoom_region(struct atomisp_sub_device *asd,
> - asd->sensor_array_res.width
> * out_res.height / out_res.width;
> h_offset = h_offset / 2;
> - if (dz_config->zoom_region.origin.y < h_offset)
> - dz_config->zoom_region.origin.y = 0;
> + if (dz_config->zoom_region.top < h_offset)
> + dz_config->zoom_region.top = 0;
> else
> - dz_config->zoom_region.origin.y = dz_config->zoom_region.origin.y - h_offset;
> + dz_config->zoom_region.top = dz_config->zoom_region.top - h_offset;
> w_offset = 0;
> } else {
> w_offset = asd->sensor_array_res.width
> - asd->sensor_array_res.height
> * out_res.width / out_res.height;
> w_offset = w_offset / 2;
> - if (dz_config->zoom_region.origin.x < w_offset)
> - dz_config->zoom_region.origin.x = 0;
> + if (dz_config->zoom_region.left < w_offset)
> + dz_config->zoom_region.left = 0;
> else
> - dz_config->zoom_region.origin.x = dz_config->zoom_region.origin.x - w_offset;
> + dz_config->zoom_region.left =
> + dz_config->zoom_region.left - w_offset;
> h_offset = 0;
> }
> - dz_config->zoom_region.origin.x = dz_config->zoom_region.origin.x
> - * eff_res.width
> - / (asd->sensor_array_res.width - 2 * w_offset);
> - dz_config->zoom_region.origin.y = dz_config->zoom_region.origin.y
> - * eff_res.height
> - / (asd->sensor_array_res.height - 2 * h_offset);
> - dz_config->zoom_region.resolution.width = dz_config->zoom_region.resolution.width
> - * eff_res.width
> - / (asd->sensor_array_res.width - 2 * w_offset);
> - dz_config->zoom_region.resolution.height = dz_config->zoom_region.resolution.height
> - * eff_res.height
> - / (asd->sensor_array_res.height - 2 * h_offset);
> - }
> -
> - if (out_res.width * dz_config->zoom_region.resolution.height
> - > dz_config->zoom_region.resolution.width * out_res.height) {
> - dz_config->zoom_region.resolution.height =
> - dz_config->zoom_region.resolution.width
> - * out_res.height / out_res.width;
> + dz_config->zoom_region.left =
> + (s32)((long long)dz_config->zoom_region.left *
> + eff_res.width /
> + (asd->sensor_array_res.width - 2 * w_offset));
> + dz_config->zoom_region.top =
> + (s32)((long long)dz_config->zoom_region.top *
> + eff_res.height /
> + (asd->sensor_array_res.height - 2 * h_offset));
> + dz_config->zoom_region.width =
> + (u32)((long long)dz_config->zoom_region.width *
> + eff_res.width /
> + (asd->sensor_array_res.width - 2 * w_offset));
> + dz_config->zoom_region.height =
> + (u32)((long long)dz_config->zoom_region.height *
> + eff_res.height /
> + (asd->sensor_array_res.height - 2 * h_offset));
> + }
> +
> + if ((long long)out_res.width * dz_config->zoom_region.height >
> + (long long)dz_config->zoom_region.width * out_res.height) {
> + dz_config->zoom_region.height =
> + (u32)((long long)dz_config->zoom_region.width *
> + out_res.height / out_res.width);
> } else {
> - dz_config->zoom_region.resolution.width =
> - dz_config->zoom_region.resolution.height
> - * out_res.width / out_res.height;
> + dz_config->zoom_region.width =
> + (u32)((long long)dz_config->zoom_region.height *
> + out_res.width / out_res.height);
> }
> dev_dbg(asd->isp->dev,
> "%s crop region:(%d,%d),(%d,%d) eff_res(%d, %d) array_size(%d,%d) out_res(%d, %d)\n",
> - __func__, dz_config->zoom_region.origin.x,
> - dz_config->zoom_region.origin.y,
> - dz_config->zoom_region.resolution.width,
> - dz_config->zoom_region.resolution.height,
> + __func__, dz_config->zoom_region.left,
> + dz_config->zoom_region.top,
> + dz_config->zoom_region.width,
> + dz_config->zoom_region.height,
> eff_res.width, eff_res.height,
> asd->sensor_array_res.width,
> asd->sensor_array_res.height,
> out_res.width, out_res.height);
>
> - if ((dz_config->zoom_region.origin.x +
> - dz_config->zoom_region.resolution.width
> + if ((dz_config->zoom_region.left +
> + dz_config->zoom_region.width
> > eff_res.width) ||
> - (dz_config->zoom_region.origin.y +
> - dz_config->zoom_region.resolution.height
> + (dz_config->zoom_region.top +
> + dz_config->zoom_region.height
> > eff_res.height))
> return -EINVAL;
>
> @@ -1901,10 +1905,10 @@ static bool atomisp_check_zoom_region(
>
> config.width = asd->sensor_array_res.width;
> config.height = asd->sensor_array_res.height;
> - w = dz_config->zoom_region.origin.x +
> - dz_config->zoom_region.resolution.width;
> - h = dz_config->zoom_region.origin.y +
> - dz_config->zoom_region.resolution.height;
> + w = dz_config->zoom_region.left +
> + dz_config->zoom_region.width;
> + h = dz_config->zoom_region.top +
> + dz_config->zoom_region.height;
>
> if ((w <= config.width) && (h <= config.height) && w > 0 && h > 0)
> flag = true;
> @@ -1912,10 +1916,10 @@ static bool atomisp_check_zoom_region(
> /* setting error zoom region */
> dev_err(asd->isp->dev,
> "%s zoom region ERROR:dz_config:(%d,%d),(%d,%d)array_res(%d, %d)\n",
> - __func__, dz_config->zoom_region.origin.x,
> - dz_config->zoom_region.origin.y,
> - dz_config->zoom_region.resolution.width,
> - dz_config->zoom_region.resolution.height,
> + __func__, dz_config->zoom_region.left,
> + dz_config->zoom_region.top,
> + dz_config->zoom_region.width,
> + dz_config->zoom_region.height,
> config.width, config.height);
>
> return flag;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index 5c0a1d92b..bb277f5a3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -678,13 +678,104 @@ static int atomisp_g_fmt_cap(struct file *file, void *fh,
> return atomisp_try_fmt_cap(file, fh, f);
> }
>
> +static int atomisp_alloc_stat_bufs_list(struct atomisp_sub_device *asd,
> + u16 stream_id,
> + struct list_head *head,
> + int count,
> + int type)
> +{
> + struct atomisp_s3a_buf *s3a_buf;
> + struct atomisp_dis_buf *dis_buf;
> + struct atomisp_metadata_buf *md_buf;
> + int ret;
> +
> + while (count--) {
> + switch (type) {
> + case IA_CSS_BUFFER_TYPE_3A_STATISTICS:
> + s3a_buf = kzalloc(sizeof(*s3a_buf), GFP_KERNEL);
> + if (!s3a_buf)
> + goto error;
> +
> + ret = atomisp_css_allocate_stat_buffers(asd, stream_id,
> + s3a_buf, NULL,
> + NULL);
> + if (ret) {
> + kfree(s3a_buf);
> + goto error;
> + }
> + list_add_tail(&s3a_buf->list, head);
> + break;
> + case IA_CSS_BUFFER_TYPE_DIS_STATISTICS:
> + dis_buf = kzalloc(sizeof(*dis_buf), GFP_KERNEL);
> + if (!dis_buf)
> + goto error;
> +
> + ret = atomisp_css_allocate_stat_buffers(asd, stream_id,
> + NULL, dis_buf,
> + NULL);
> + if (ret) {
> + kfree(dis_buf);
> + goto error;
> + }
> + list_add_tail(&dis_buf->list, head);
> + break;
> + case IA_CSS_BUFFER_TYPE_METADATA:
> + md_buf = kzalloc(sizeof(*md_buf), GFP_KERNEL);
> + if (!md_buf)
> + goto error;
> +
> + ret = atomisp_css_allocate_stat_buffers(asd, stream_id,
> + NULL, NULL,
> + md_buf);
> + if (ret) {
> + kfree(md_buf);
> + goto error;
> + }
> + list_add_tail(&md_buf->list, head);
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +
> +error:
> + while (!list_empty(head)) {
> + switch (type) {
> + case IA_CSS_BUFFER_TYPE_3A_STATISTICS:
> + s3a_buf = list_entry(head->next,
> + struct atomisp_s3a_buf, list);
> + atomisp_css_free_3a_buffer(s3a_buf);
> + list_del(&s3a_buf->list);
> + kfree(s3a_buf);
> + break;
> + case IA_CSS_BUFFER_TYPE_DIS_STATISTICS:
> + dis_buf = list_entry(head->next,
> + struct atomisp_dis_buf, list);
> + atomisp_css_free_dis_buffer(dis_buf);
> + list_del(&dis_buf->list);
> + kfree(dis_buf);
> + break;
> + case IA_CSS_BUFFER_TYPE_METADATA:
> + md_buf = list_entry(head->next,
> + struct atomisp_metadata_buf, list);
> + atomisp_css_free_metadata_buffer(md_buf);
> + list_del(&md_buf->list);
> + kfree(md_buf);
> + break;
> + }
> + }
> + return -ENOMEM;
> +}
> +
> int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
> uint16_t stream_id)
> {
> struct atomisp_device *isp = asd->isp;
> - struct atomisp_s3a_buf *s3a_buf = NULL, *_s3a_buf;
> - struct atomisp_dis_buf *dis_buf = NULL, *_dis_buf;
> - struct atomisp_metadata_buf *md_buf = NULL, *_md_buf;
> + struct atomisp_dis_buf *dis_buf, *_dis_buf;
> + struct atomisp_s3a_buf *s3a_buf, *_s3a_buf;
> + struct atomisp_metadata_buf *md_buf, *_md_buf;
> int count;
> struct ia_css_dvs_grid_info *dvs_grid_info =
> atomisp_css_get_dvs_grid_info(&asd->params.curr_grid_info);
> @@ -695,37 +786,20 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
> count = ATOMISP_CSS_Q_DEPTH +
> ATOMISP_S3A_BUF_QUEUE_DEPTH_FOR_HAL;
> dev_dbg(isp->dev, "allocating %d 3a buffers\n", count);
> - while (count--) {
> - s3a_buf = kzalloc(sizeof(struct atomisp_s3a_buf), GFP_KERNEL);
> - if (!s3a_buf)
> - goto error;
> -
> - if (atomisp_css_allocate_stat_buffers(
> - asd, stream_id, s3a_buf, NULL, NULL)) {
> - kfree(s3a_buf);
> - goto error;
> - }
> -
> - list_add_tail(&s3a_buf->list, &asd->s3a_stats);
> - }
> + if (atomisp_alloc_stat_bufs_list(asd, stream_id,
> + &asd->s3a_stats, count,
> + IA_CSS_BUFFER_TYPE_3A_STATISTICS))
> + goto error;
> }
>
> if (list_empty(&asd->dis_stats) && dvs_grid_info &&
> dvs_grid_info->enable) {
> count = ATOMISP_CSS_Q_DEPTH + 1;
> dev_dbg(isp->dev, "allocating %d dis buffers\n", count);
> - while (count--) {
> - dis_buf = kzalloc(sizeof(struct atomisp_dis_buf), GFP_KERNEL);
> - if (!dis_buf)
> - goto error;
> - if (atomisp_css_allocate_stat_buffers(
> - asd, stream_id, NULL, dis_buf, NULL)) {
> - kfree(dis_buf);
> - goto error;
> - }
> -
> - list_add_tail(&dis_buf->list, &asd->dis_stats);
> - }
> + if (atomisp_alloc_stat_bufs_list(asd, stream_id,
> + &asd->dis_stats, count,
> + IA_CSS_BUFFER_TYPE_DIS_STATISTICS))
> + goto error;
> }
>
> for (i = 0; i < ATOMISP_METADATA_TYPE_NUM; i++) {
> @@ -736,19 +810,10 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
> ATOMISP_METADATA_QUEUE_DEPTH_FOR_HAL;
> dev_dbg(isp->dev, "allocating %d metadata buffers for type %d\n",
> count, i);
> - while (count--) {
> - md_buf = kzalloc(sizeof(struct atomisp_metadata_buf),
> - GFP_KERNEL);
> - if (!md_buf)
> - goto error;
> -
> - if (atomisp_css_allocate_stat_buffers(
> - asd, stream_id, NULL, NULL, md_buf)) {
> - kfree(md_buf);
> - goto error;
> - }
> - list_add_tail(&md_buf->list, &asd->metadata[i]);
> - }
> + if (atomisp_alloc_stat_bufs_list(asd, stream_id,
> + &asd->metadata[i], count,
> + IA_CSS_BUFFER_TYPE_METADATA))
> + goto error;
> }
> }
> return 0;
Powered by blists - more mailing lists