| 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: <CAPsOcun2Pkn5ojW_QgR3m2M4aZ2pu4bnebaNUE3hHej+b86m_A@mail.gmail.com>
Date: Wed, 14 Jan 2026 23:07:54 +0530
From: Karthikey Kadati <karthikey3608@...il.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Hans de Goede <hansg@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Andy Shevchenko <andy@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: atomisp: remove private white balance IOCTLs
Hi Sakari,
You are absolutely right.
I investigated the code and confirmed that `integer_bits` in
`atomisp_wb_config` is essential for the hardware's gain calculation
(shift logic) and varies by configuration. Mapping this to standard
V4L2 controls (like `V4L2_CID_RED_BALANCE`) would mask this parameter,
making the interface unusable or forcing incorrect assumptions.
I also see that `ATOMISP_IOC_S_PARAMETERS` already exists and accepts
`struct atomisp_parameters`, which includes `atomisp_wb_config`. This
appears to be the "parameter buffer" mechanism you referred to, which
correctly handles the full configuration context.
Attempting to convert these specific private IOCTLs to standard
controls was the wrong approach, as they seem to be redundant legacy
interfaces better served by the existing parameter buffer.
I will drop this patch from the series.
Thanks,
Karthikey
On Mon, 12 Jan 2026 at 16:30, Sakari Ailus <sakari.ailus@...ux.intel.com> wrote:
>
> Hi Karthikey,
>
> On Wed, Dec 31, 2025 at 10:54:27AM +0530, Karthikey D Kadati wrote:
> > This patch resolves a MUST-FIX graduation blocker identified in the
> > atomisp TODO by removing the private ATOMISP_IOC_G_ISP_WHITE_BALANCE
> > and ATOMISP_IOC_S_ISP_WHITE_BALANCE and replacing them with standard
> > V4L2 control handling.
> >
> > The private IOCTLs were used to set white balance parameters. This
> > functionality is now mapped to the standard V4L2 controls
> > V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE.
> >
> > A helper function `atomisp_v4l2_set_wb` is introduced to translate the
> > V4L2 control values to the driver's internal configuration format.
> >
> > Signed-off-by: Karthikey D Kadati <karthikey3608@...il.com>
> > ---
> > .../media/atomisp/include/linux/atomisp.h | 5 +-
> > .../staging/media/atomisp/pci/atomisp_ioctl.c | 49 ++++++++++++++++---
> > 2 files changed, 44 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
> > index 3c8fa3f58..fcf116cc4 100644
> > --- a/drivers/staging/media/atomisp/include/linux/atomisp.h
> > +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
> > @@ -741,10 +741,7 @@ enum atomisp_burst_capture_options {
> > _IOW('v', BASE_VIDIOC_PRIVATE + 15, struct atomisp_ctc_table)
> >
> > /* white balance Correction */
> > -#define ATOMISP_IOC_G_ISP_WHITE_BALANCE \
> > - _IOR('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config)
> > -#define ATOMISP_IOC_S_ISP_WHITE_BALANCE \
> > - _IOW('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config)
> > +
> >
> > /* fpn table loading */
> > #define ATOMISP_IOC_S_ISP_FPN_TABLE \
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > index bb8b2f221..5c0a1d92b 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > @@ -1083,6 +1083,38 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
> > * applications initialize the id and value fields of a struct v4l2_control
> > * and call this ioctl.
> > */
> > +static int atomisp_v4l2_set_wb(struct atomisp_sub_device *asd, int id,
> > + int value)
> > +{
> > + struct atomisp_device *isp = asd->isp;
> > + struct atomisp_wb_config config;
> > + int ret;
> > +
> > + if (atomisp_css_get_wb_config(asd, &config)) {
>
> I'm not sure this makes sense. How will the caller know the value of
> integer_bits?
>
> I'd think the IOCTL interface of this driver should probably be largely
> removed, to be replaced by the parameter buffer.
>
> > + dev_err(isp->dev, "%s: can't get wb config\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + switch (id) {
> > + case V4L2_CID_BLUE_BALANCE:
> > + config.b = value << (16 - 8 - config.integer_bits + 1);
> > + break;
> > + case V4L2_CID_RED_BALANCE:
> > + config.r = value << (16 - 8 - config.integer_bits + 1);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = atomisp_white_balance_param(asd, 1, &config);
> > + if (ret) {
> > + dev_err(isp->dev, "%s: set wb config failed\n", __func__);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int atomisp_s_ctrl(struct file *file, void *fh,
> > struct v4l2_control *control)
> > {
> > @@ -1122,6 +1154,17 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
> > case V4L2_CID_ATOMISP_LOW_LIGHT:
> > ret = atomisp_low_light(asd, 1, &control->value);
> > break;
> > + case V4L2_CID_AUTO_WHITE_BALANCE:
> > + /*
> > + * TODO: Auto White Balance is not supported yet.
> > + * It is currently handled by the ISP.
> > + */
> > + ret = 0;
> > + break;
> > + case V4L2_CID_RED_BALANCE:
> > + case V4L2_CID_BLUE_BALANCE:
> > + ret = atomisp_v4l2_set_wb(asd, control->id, control->value);
> > + break;
> > default:
> > ret = -EINVAL;
> > break;
> > @@ -1484,13 +1527,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
> > err = atomisp_ctc(asd, 1, arg);
> > break;
> >
> > - case ATOMISP_IOC_G_ISP_WHITE_BALANCE:
> > - err = atomisp_white_balance_param(asd, 0, arg);
> > - break;
> >
> > - case ATOMISP_IOC_S_ISP_WHITE_BALANCE:
> > - err = atomisp_white_balance_param(asd, 1, arg);
> > - break;
> >
> > case ATOMISP_IOC_G_3A_CONFIG:
> > err = atomisp_3a_config_param(asd, 0, arg);
>
> --
> Regards,
>
> Sakari Ailus
Powered by blists - more mailing lists