Re: [PATCH v6 09/12] media: microchip-isc: add SAMA7G5 hue and saturation controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] V4L2 controls are improperly gated due to deferred pipeline configuration, causing incorrect active states and state desynchronization when inactive controls are modified.

Pre-existing issues:
- [High] `mutex_destroy(&isc->awb_mutex)` is called on an uninitialized or already destroyed mutex during async error and unbind paths.
--

commit a918c53cb3531019eb23300da7b6bcbdebef4df4
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: add SAMA7G5 hue and saturation controls

This commit exposes hue and saturation as V4L2 controls for the SAMA7G5
CBHS block. The controls are gated based on the has_cbhs flag and are
initialized to neutral values at probe.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -859,6 +859,46 @@ static int isc_try_configure_pipeline(struct isc_device *isc)

[ ... ]

> +static void isc_update_cbhs_ctrls(struct isc_device *isc)
> +{
> +	bool cbhs_active = isc->config.bits_pipeline & CBHS_ENABLE;
> +	bool chroma_active = cbhs_active && isc_format_is_yuv(isc->config.fourcc);
> +
> +	if (isc->brightness_ctrl)
> +		v4l2_ctrl_activate(isc->brightness_ctrl, cbhs_active);
> +	if (isc->contrast_ctrl)
> +		v4l2_ctrl_activate(isc->contrast_ctrl, cbhs_active);
> +	if (isc->hue_ctrl)
> +		v4l2_ctrl_activate(isc->hue_ctrl, chroma_active);
> +	if (isc->saturation_ctrl)
> +		v4l2_ctrl_activate(isc->saturation_ctrl, chroma_active);
> +}

[ ... ]

> @@ -902,6 +942,7 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>  	/* make the try configuration active */
>  	isc->config = isc->try_config;
>  	isc->fmt = isc->try_fmt;
> +	isc_update_cbhs_ctrls(isc);

[Severity: Medium]
Does this leave the CBHS controls incorrectly deactivated before streaming
starts?

Because isc->config.bits_pipeline is only updated during link validation
(VIDIOC_STREAMON), it appears bits_pipeline will be zero or stale here
in isc_set_fmt().

If the controls remain incorrectly inactive, and userspace tries to set
them, isc_s_ctrl() might quietly reject the change:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_ctrl() {
    ...
    if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
        return 0;
    ...
}

Will this silently ignore the update while the V4L2 core caches the new
value, causing the driver's internal hardware state to permanently diverge
from the V4L2 framework's cached state?


[Severity: High]
This is a pre-existing issue, but can the async error handling path cause
a double-destroy or uninitialized-destroy panic?

If an early operation such as v4l2_device_register_subdev_nodes() fails
in isc_async_complete(), it returns an error before initializing
isc->awb_mutex. The V4L2 async core handles this failure by unbinding the
subdevices, which triggers isc_async_unbind().

isc_async_unbind() unconditionally calls:

drivers/media/platform/microchip/microchip-isc-base.c:isc_async_unbind() {
    ...
    mutex_destroy(&isc->awb_mutex);
    ...
}

Could this lead to a kernel oops on an uninitialized mutex if
CONFIG_DEBUG_MUTEXES is enabled? Additionally, if isc_async_complete()
fails after initialization, it explicitly destroys the mutex in its error
block, causing isc_async_unbind() to destroy it a second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-microchip-isc-fixes-v6-0-8c3d7474a768@microchip.com?part=9