Re: [PATCH v20 09/13] media: qcom: camss: tpg: Add support for v2.4.0 TPG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] media: camss: CSID 1080 fallback to CSIPHY breaks graph isolation
- [Low] media: camss: tpg_reset evaluates hw_version before it is initialized
- [Low] media: qcom: camss: data race on tpg->testgen.mode across subdevices
--

commit 74ebce0fb447af79287e1df16be3097446ecc373
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: tpg: Add support for v2.4.0 TPG

Add support for the TPG found on Kaanapali. This updates the CSID configuration
for TPG routing and adds version checks for the TPG hardware registers.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> index 46e443e05195a..851b3cd712f52 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-1080.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.c

[ ... ]

> @@ -148,8 +151,16 @@ static void __csid_configure_rx(struct csid_device *csid,
>  
>  	val = (lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>  	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> -	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> -	       << CSI2_RX_CFG0_PHY_NUM_SEL;
> +
> +	if (camss->tpg && csid->tpg_linked &&
> +	    camss->tpg[phy->csiphy_id].testgen.mode != TPG_PAYLOAD_MODE_DISABLED) {

[Severity: Low]
Is there a data race when reading testgen.mode here?

The CSID subdevice directly reads this locklessly during pipeline
configuration, but the TPG subdevice updates testgen.mode via tpg_s_ctrl()
which only holds the TPG's local ctrls->lock.

Unprivileged userspace could concurrently issue VIDIOC_S_CTRL and
VIDIOC_STREAMON on the respective subdevices, causing a pipeline configuration
mismatch.

> +		val |= FIELD_PREP(CSI2_RX_CFG0_TPG_MUX_SEL, phy->csiphy_id + 1);
> +		val |= CSI2_RX_CFG0_TPG_MUX_EN;
> +	} else {
> +		val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> +			<< CSI2_RX_CFG0_PHY_NUM_SEL;

[Severity: Medium]
Could this fallback break graph isolation?

If the user-space client links a TPG to the CSID but leaves the TPG test
pattern control set to DISABLED, it falls back to this else block.

This configures the CSID RX PHY mux to read from physical CSIPHY
phy->csiphy_id. If that physical CSIPHY is concurrently active (for example,
streaming to another CSID), won't its data leak into this CSID, receiving
stream data from a physical CSIPHY that is not linked to it in the V4L2
media graph?

> +	}
> +
>  	writel(val, csid->base + CSID_CSI2_RX_CFG0);
>  
>  	val = CSI2_RX_CFG1_ECC_CORRECTION_EN;

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
> index d29de5f93c18e..cabbb2638cf38 100644
> --- a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
> +++ b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c

[ ... ]

> @@ -164,18 +167,26 @@ static int tpg_stream_on(struct tpg_device *tpg)
>  	}
>  
>  	/* Global TPG control */
> -	val = FIELD_PREP(TPG_CTRL_TEST_EN, 1) |
> -	      FIELD_PREP(TPG_CTRL_NUM_ACTIVE_LANES, lane_cnt - 1) |
> +	val = FIELD_PREP(TPG_CTRL_NUM_ACTIVE_LANES, lane_cnt - 1) |
>  	      FIELD_PREP(TPG_CTRL_NUM_ACTIVE_VC, last_vc);
> -	writel(val, tpg->base + TPG_CTRL);
> +
> +	if (tpg->hw_version < TPG_HW_VER_2_4_0) {
> +		val |= FIELD_PREP(TPG_CTRL_TEST_EN, 1);
> +		writel(val, tpg->base + TPG_CTRL);
> +	} else {
> +		writel(val, tpg->base + TPG_CTRL);
> +		writel(TPG_CTRL_CMD_TEST_EN, tpg->base + TPG_CTRL_CMD);
> +	}
>  
>  	return 0;
>  }
>  
>  static int tpg_reset(struct tpg_device *tpg)
>  {
> -	writel(0, tpg->base + TPG_CTRL);
> -	writel(1, tpg->base + TPG_CLEAR);
> +	if (tpg->hw_version < TPG_HW_VER_2_4_0)
> +		writel(0, tpg->base + TPG_CTRL);

[Severity: Low]
Will this evaluate to true on the first power-up for v2.4.0 hardware?

Looking at tpg_set_power() in camss-tpg.c:

	tpg->res->hw_ops->reset(tpg);
	tpg->res->hw_ops->hw_version(tpg);

reset() is invoked prior to hw_version(). Since the tpg_device structure is
zero-allocated and hw_version is not initialized during probe, tpg->hw_version
is 0 on the first power-up.

Doesn't this cause the driver to execute the very register write it was
intended to skip?

> +
> +	writel(TPG_CTRL_CMD_HW_RESET, tpg->base + TPG_CTRL_CMD);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-kaanapali-camss-v20-0-63e98ce9ac0c@oss.qualcomm.com?part=9