Re: [PATCH v3 4/8] media: uapi: Add NXP NEOISP user interface header file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] UAPI Compilation Failure due to undefined struct `v4l2_isp_block_header`
- [Medium] Implicit compiler padding (holes) in UAPI structures
--

commit 07ff92ed00f28aef4b4e762772b07f402cd3490d
Author: Antoine Bouyer <antoine.bouyer@nxp.com>

media: uapi: Add NXP NEOISP user interface header file

Add user space api header file for meta data structures definitions.
This header describes parameters buffer for the ISP blocks control by
userspace, and statistics buffer for userspace and IPA handling.

> diff --git a/include/uapi/linux/media/nxp/nxp_neoisp.h b/include/uapi/linux/media/nxp/nxp_neoisp.h
> new file mode 100644
> index 0000000000000..2dafa19df3242
> --- /dev/null
> +++ b/include/uapi/linux/media/nxp/nxp_neoisp.h

[ ... ]

> +struct neoisp_obwb_cfg_s {
> +	__u8 ctrl_obpp;
> +	__u16 r_ctrl_gain;
> +	__u16 r_ctrl_offset;

[Severity: Medium]
Is there an implicit 1-byte compiler padding hole after ctrl_obpp to align
the __u16 r_ctrl_gain field?

UAPI guidelines generally require explicit reserved fields to fill all
holes. This guarantees a stable ABI and prevents uninitialized kernel memory
from potentially leaking to userspace.

[ ... ]

> +struct neoisp_ir_compress_cfg_s {
> +	__u8 ctrl_obpp;
> +	__u32 knee_point1_kneepoint;

[Severity: Medium]
Does this introduce a 3-byte implicit padding hole after ctrl_obpp to align
the __u32 knee_point1_kneepoint field?

[ ... ]

> +struct neoisp_bnr_cfg_s {
> +	__u8 ctrl_debug;
> +	__u8 ctrl_obpp;
> +	__u8 ctrl_nhood;
> +	__u8 ypeak_peak_outsel;
> +	__u8 ypeak_peak_sel;
> +	__u16 ypeak_peak_low;

[Severity: Medium]
Are there implicit padding holes in this structure, such as a 1-byte hole
after ypeak_peak_sel?

[ ... ]

> +struct neoisp_rgb2yuv_cfg_s {
> +	__u16 gain_ctrl_rgain;
> +	__u16 gain_ctrl_bgain;
> +	__s16 mat_rxcy[NEO_RGB2YUV_MATRIX_SIZE][NEO_RGB2YUV_MATRIX_SIZE];
> +	__s32 csc_offsets[NEO_RGB2YUV_MATRIX_SIZE];
> +};

[Severity: Medium]
Is there a 2-byte implicit padding hole after the 18-byte mat_rxcy array to
align the __s32 csc_offsets field?

[ ... ]

> +struct neoisp_ee_cfg_s {
> +	__u8 ctrl_debug;
> +	__u8 maskgain_gain;
> +	__u32 coring_coring;

[Severity: Medium]
Does this leave a 2-byte implicit padding hole after maskgain_gain?

[ ... ]

> +struct neoisp_pipe_conf_cfg_es {
> +	struct v4l2_isp_block_header header;
> +	struct neoisp_pipe_conf_cfg_s cfg;
> +} __attribute__((aligned(8)));

[Severity: High]
Will embedding struct v4l2_isp_block_header by value cause an incomplete
type compilation error?

The struct v4l2_isp_block_header does not appear to be defined anywhere,
as the generic V4L2 ISP framework defines v4l2_isp_params_block_header
instead. This seems to break compilation for both the driver and any
userspace program including this header.

[ ... ]

> +struct neoisp_ctemp_reg_stats_es {
> +	struct v4l2_isp_block_header header;
> +	struct neoisp_ctemp_reg_stats_s stat;
> +} __attribute__((aligned(8)));

[Severity: Medium]
Does forcing 8-byte alignment here add 4 bytes of implicit trailing padding
to this statistics wrapper structure?

Adding explicit padding fields might be needed to prevent uninitialized
kernel memory from leaking to userspace via the statistics buffer.

[ ... ]

> +struct neoisp_nr_reg_stats_es {
> +	struct v4l2_isp_block_header header;
> +	struct neoisp_nr_reg_stats_s stat;
> +} __attribute__((aligned(8)));

[Severity: Medium]
Does this also contain 4 bytes of implicit trailing padding due to the
explicit 8-byte alignment requirement?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612132039.2089051-1-antoine.bouyer@nxp.com?part=4