Re: [PATCH v6 5/8] media: Documentation: uapi: Add qcom-camss ISP params documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The C example code in the documentation uses a bitwise OR operation on an uninitialized structure field and fails to initialize padding and offset fields.
- [Low] The gamma LUT initialization in the documentation example directly contradicts the formula defined in the uAPI header.
--

commit 3ecf0034d91da8edcad212fbbdb3de261d905083
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: Documentation: uapi: Add qcom-camss ISP params documentation

Add documentation for the Qualcomm CAMSS ISP parameters metadata format.
The format is built on the V4L2 extensible ISP parameters system and is
designed to be shared by any CAMSS ISP engine exposing a parameters
metadata output video node.

> diff --git a/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst b/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst
> new file mode 100644
> index 0000000000000..da3dd5f3a5899
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst
[ ... ]
> +	struct camss_params_ope_wb_gain *wb =
> +		(struct camss_params_ope_wb_gain *)data;
> +
> +	wb->header.type = CAMSS_PARAMS_OPE_WB_GAIN;
> +	wb->header.flags |= V4L2_ISP_PARAMS_FL_BLOCK_ENABLE;

[Severity: Low]
Is this bitwise OR operation safe in this example?

Since the struct is cast directly from the buffer, the flags field is
uninitialized memory. Using a read-modify-write operation here leads to
undefined behavior for userspace applications copying this example. Could
this be changed to a direct assignment?

> +	wb->header.size = sizeof(struct camss_params_ope_wb_gain);
> +
> +	/* Unity gain on all three channels (15uQ10, 1024 = 1.0) */
> +	wb->g_gain = 1024;
> +	wb->b_gain = 1024;
> +	wb->r_gain = 1024;

[Severity: Low]
Does this initialization cover all required fields?

This code sets the gain fields but leaves mandatory zero padding and offset
fields like g_sub and g_add uninitialized. Applications copying this example
might unintentionally pass garbage offsets to the kernel ISP driver.

[ ... ]
> +	struct camss_params_ope_gamma *gamma =
> +		(struct camss_params_ope_gamma *)data;
> +
> +	gamma->header.type = CAMSS_PARAMS_OPE_GAMMA;
> +	gamma->header.flags |= V4L2_ISP_PARAMS_FL_BLOCK_ENABLE;
> +	gamma->header.size = sizeof(struct camss_params_ope_gamma);
> +
> +	/* Identity curve (pass-through, gamma 1.0) */
> +	for (unsigned int i = 0; i < CAMSS_OPE_GAMMA_LUT_SIZE; i++)
> +		gamma->glut[i] = gamma->blut[i] = gamma->rlut[i] = 257 * i;

[Severity: Low]
Is this gamma LUT initialization formula accurate?

This calculation uses 257 * i, which contradicts the official uAPI header in
include/uapi/linux/qcom-camss-config.h where the identity curve formula is
explicitly defined as lut[i] = i.

Could this contradiction cause confusion for developers implementing V4L2
userspace applications?

> +
> +	data += sizeof(struct camss_params_ope_gamma);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-camss-isp-ope-v6-0-6b915b9c5131@oss.qualcomm.com?part=5