Re: [PATCH v6 02/12] media: microchip-isc: fix WB offset and gain register field masking

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe hardware register access and data race in AWB control handler
--

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

media: microchip-isc: fix WB offset and gain register field masking

ISC_WB_O_* and ISC_WB_G_* pack two 13-bit fields per register. Sign
extension from negative offsets corrupts the upper field. Mask both
fields to 13 bits before packing.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index a7cdc743fda71..45b94f1e89d86 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -61,18 +61,23 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
>  
>  	/* In here we set our actual hw pipeline config */
>  
> +	/*
> +	 * Mask offset fields to 13 bits. Sign extension of negative s32
> +	 * values would otherwise corrupt the adjacent field.
> +	 */
>  	regmap_write(isc->regmap, ISC_WB_O_RGR,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
> +		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
> +		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>  	regmap_write(isc->regmap, ISC_WB_O_BGB,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
> +		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
> +		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
> +	/* Gains are 13-bit unsigned fields [12:0] and [28:16] */
>  	regmap_write(isc->regmap, ISC_WB_G_RGR,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
> +		     (ctrls->gain[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
> +		     ((ctrls->gain[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>  	regmap_write(isc->regmap, ISC_WB_G_BGB,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
> +		     (ctrls->gain[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
> +		     ((ctrls->gain[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at how this function
is called, can this cause a bus fault if the device is suspended?

When userspace modifies an AWB control, isc_s_awb_ctrl() is executed and
calls isc_update_awb_ctrls() unconditionally:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_awb_ctrl() {
        ...
        if (ctrl->cluster[ISC_CTRL_GB_OFF]->is_new)
                ctrls->offset[ISC_HIS_CFG_MODE_GB] = isc->gb_off_ctrl->val;

        isc_update_awb_ctrls(isc);

        mutex_lock(&isc->awb_mutex);
        ...
}

This happens outside of any vb2_is_streaming() check and without pm runtime
protection like pm_runtime_resume_and_get(), which means we might access
MMIO registers while the AHB clock is disabled.

Additionally, this call is made without holding the awb_lock. Does this
create a data race with isc_awb_work() that would allow concurrent register
updates to tear the hardware pipeline configuration?

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