Re: [PATCH v2 7/8] media: qcom: camss: csid: Add support for CSID 1190

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] media: qcom: camss: Integer underflow in __csid_configure_rx() corrupts CSID_CSI2_RX_CFG0
- [Medium] media: qcom: camss: Data race on aup_update and rup_update between process and hardirq context
- [Medium] media: camss: CSID 1190 5th RDI stream inaccessible due to MSM_CSID_PADS_NUM limit
- [Low] media: qcom: camss: Macro MSM_CSID_MAX_SRC_STREAMS_1190 implicitly captures local variable
--

commit 479f67e9b9b628614ef82c2426c608857a19f333
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: csid: Add support for CSID 1190

This commit adds support for the CSID 1190 hardware found on Hawi SoC.
It introduces hardware resource descriptions, hardware-specific register
programming, reset sequence, and BUF_DONE interrupt handling. CSID 1190 splits
RUP and AUP updates into separate registers and uses a SET register to commit
them.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1190.c b/drivers/media/platform/qcom/camss/camss-csid-1190.c
> new file mode 100644
> index 0000000000000..999733aa8ae63
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1190.c
[ ... ]
> +/* CSI2 RX Configuration */
> +#define CSID_CSI2_RX_CFG0			0x880
> +#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES		0
[ ... ]
> +#define MSM_CSID_MAX_SRC_STREAMS_1190		(csid_is_lite(csid) ? 4 : 5)

[Severity: Low]
This isn't a bug, but this macro implicitly captures a local variable named
csid from the caller's scope. This masks dependencies and breaks if the local
variable is renamed. Could this be refactored to take csid as an explicit
macro argument?

[ ... ]
> +static void __csid_aup_rup_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +	csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}

[Severity: Medium]
Are these read-modify-write operations safe from data races?

In hardirq context, csid_isr() calls __csid_aup_rup_clear() which clears bits
in these same variables using non-atomic operations (&= ~).

In process context, __csid_aup_update() and __csid_reg_update() also use
non-atomic operations (|=) to set bits:

drivers/media/platform/qcom/camss/camss-csid-1190.c:__csid_aup_update() {
    csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
    ...
}

If an INFO_RUP_DONE hardware interrupt fires while the process-context loop is
preparing the next stream's bits, could the two execution paths race,
potentially clobbering the interrupt handler's bit clear or the stream
configuration's bit set?

[ ... ]
> +static void __csid_configure_rx(struct csid_device *csid,
> +				struct csid_phy_config *phy)
> +{
> +	struct camss *camss = csid->camss;
> +	u8 lane_cnt = csid->phy.lane_cnt;
> +	int val;
> +
> +	if (!lane_cnt)
> +		lane_cnt = 4;
> +
> +	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;

[Severity: High]
Does this code underflow if phy->lane_cnt is 0?

While lane_cnt is correctly guarded against being 0 by falling back to 4, the
bitshift operation uses the unmodified phy->lane_cnt struct member instead of
the corrected lane_cnt variable.

If phy->lane_cnt is 0, 0 - 1 promotes to an integer -1. Shifting -1 left by 0
evaluates to 0xFFFFFFFF, which would write an all-ones mask to the
CSID_CSI2_RX_CFG0 register, completely corrupting its configuration and all
reserved bits.

[ ... ]
> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> +{
> +	u8 i, k;
> +
> +	__csid_configure_rx(csid, &csid->phy);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_1190; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {

[Severity: Medium]
Is the 5th RDI stream accessible?

MSM_CSID_MAX_SRC_STREAMS_1190 evaluates to 5 for non-lite hardware, intending
to support 5 RDI streams. However, the driver globally defines
MSM_CSID_PADS_NUM as 5 (1 sink + 4 sources) in camss-csid.h.

This restricts the V4L2 media entity to 4 source pads (indices 1 to 4). Since
the stream configuration relies on the csid->phy.en_vc bitmask (populated
during link setup based on pad indices), BIT(4) can never be set. Is the 5th
iteration of this loop effectively dead code, rendering the 5th RDI stream
inaccessible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-hawi-maili-camss-v2-0-fc93222da89c@oss.qualcomm.com?part=7