Re: [PATCH v6 04/12] media: microchip-isc: fix PM runtime leak in AWB work handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `isc_s_awb_ctrl()`, `isc_update_awb_ctrls()` and `isc_update_profile()` can access unclocked memory-mapped registers, causing an external abort.
- [High] In `isc_async_unbind()`, `mutex_destroy(&isc->awb_mutex)` is called before synchronously cancelling `awb_work`, leading to use-after-free or invalid lock operations.
- [Medium] The identical PM runtime leak fixed in microchip-isc-base.c still exists in the equivalent atmel-isc-base.c staging driver.
- [Medium] Data race on internal AWB state and unprotected hardware register updates in `isc_s_awb_ctrl`.
- [Low] Unconditional assignment of `dev_pm_ops` pointers generates dead code when PM is disabled.
--

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

    media: microchip-isc: fix PM runtime leak in AWB work handler
    
    Early return when streaming stops skips pm_runtime_put_sync(),
    leaking the reference and preventing runtime suspend.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index b19c5a63b4bdc..f61a5d5a3e045 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -1429,7 +1429,7 @@ static void isc_awb_work(struct work_struct *w)
>  	/* streaming is not active anymore */
>  	if (isc->stop) {
>  		mutex_unlock(&isc->awb_mutex);
> -		return;
> +		goto out_pm_put;
>  	}
>  
>  	isc_update_profile(isc);
> @@ -1440,6 +1440,7 @@ static void isc_awb_work(struct work_struct *w)
>  	if (ctrls->awb)
>  		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
>  
> +out_pm_put:
>  	pm_runtime_put_sync(isc->dev);
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but does this identical PM runtime
leak still exist in the equivalent atmel-isc-base.c staging driver?

The staging driver (drivers/staging/media/deprecated/atmel/atmel-isc-base.c)
contains a copy of isc_awb_work() with the same early return bug:

drivers/staging/media/deprecated/atmel/atmel-isc-base.c:isc_awb_work() {
    ...
	if (isc->stop) {
		mutex_unlock(&isc->awb_mutex);
		return;
	}
    ...
}

Should the staging driver be patched as well to prevent a resource leak there?

[Severity: High]
This is a pre-existing issue, but can isc_s_awb_ctrl() and
isc_update_profile() access unclocked memory-mapped registers, causing an
external abort?

isc_s_awb_ctrl() executes independently of streaming state and without
pm_runtime protection. When called while the hardware is runtime suspended,
isc_update_awb_ctrls() will perform an MMIO regmap_write() to unclocked
hardware registers:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_awb_ctrl() {
    ...
    isc_update_awb_ctrls(isc);
    ...
}

On many ARM architectures, accessing unclocked registers causes an external
abort which immediately panics the kernel.

Furthermore, if called concurrently during teardown, isc_stop_streaming()
releases awb_mutex and drops the PM reference via pm_runtime_put_sync()
before returning. Since videobuf2 only clears q->streaming = 0 after
stop_streaming returns, vb2_is_streaming() can evaluate to true while the
hardware is runtime suspended.

This triggers an additional call to isc_update_profile() which polls
unclocked hardware indefinitely.

[Severity: High]
This isn't a bug introduced by this patch, but does isc_async_unbind() destroy
awb_mutex before synchronously cancelling the awb_work, leading to an invalid
lock operation or use-after-free?

Looking at isc_async_unbind():

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

cancel_work_sync() blocks until the pending isc_awb_work completes. While it
waits, the work handler executes and attempts to lock isc->awb_mutex, which
has already been destroyed. The identical bug appears to exist in the staging
driver's isc_async_unbind() as well.

[Severity: Medium]
This is a pre-existing issue, but does isc_s_awb_ctrl() have a data race and
unprotected hardware register updates?

The V4L2 ioctl handler isc_s_awb_ctrl() modifies internal AWB controls
(ctrls->gain, ctrls->offset, ctrls->awb) holding the V4L2 lock, while the work
handler isc_awb_work() (via isc_wb_update()) reads and modifies these exact
fields locklessly:

drivers/media/platform/microchip/microchip-isc-base.c:isc_wb_update() {
    ...
    ctrls->gain[c] = (3 * ctrls->gain[c] + gain) / 4;
    ...
}

In addition to corrupting the values, isc_s_awb_ctrl() unconditionally calls
isc_update_awb_ctrls() to write to hardware registers without holding
isc->awb_lock. The work handler explicitly documents that awb_lock is strictly
required to prevent the DMA Done IRQ from latching a partially updated state.

Can this omission result in incorrect white balance configurations taking effect
for a frame?

[Severity: Low]
This isn't a bug introduced by this patch, but are the dev_pm_ops structures
assigned unconditionally in the platform drivers, generating dead code when PM
is disabled?

In microchip-sama5d2-isc.c and microchip-sama7g5-isc.c:

drivers/media/platform/microchip/microchip-sama5d2-isc.c:microchip_isc_driver {
    ...
    .pm = &microchip_isc_dev_pm_ops,
    ...
}

This violates the PM subsystem rule which requires the pm_ptr() wrapper to
ensure the compiler can optimize away the structures when CONFIG_PM is
disabled. Without it, the callbacks and the structures generate dead code.

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