Re: [PATCH v7 7/8] media: qcom: camss: Account for C-PHY when calculating link frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Deterministic NULL pointer dereference when calculating link frequency for the CSID linked to the internal Test Pattern Generator (TPG).
- [High] The C-PHY link frequency calculation inverses the symbol rate ratio, resulting in a severely inflated link frequency.
--

commit 0a60c2346d24cbca656cf347fe3da5342c1b9381
Author: David Heidelberg <david@ixit.cz>

media: qcom: camss: Account for C-PHY when calculating link frequency

Ensure that the link frequency divider correctly accounts for C-PHY
operation. The divider differs between D-PHY and C-PHY, as described
in the MIPI CSI-2 specification.

Link: https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate

> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index bcc34ac9dd212..7415e811082da 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -539,15 +539,18 @@ const struct csid_format_info *csid_get_fmt_entry(const struct csid_format_info
>  static int csid_set_clock_rates(struct csid_device *csid)
>  {
>  	struct device *dev = csid->camss->dev;
> +	struct csiphy_device *csiphy = &csid->camss->csiphy[csid->phy.csiphy_id];
> +	struct csiphy_lanes_cfg *lane_cfg = &csiphy->cfg.csi2->lane_cfg;

[Severity: High]
Will this cause a NULL pointer dereference when the CSID is linked to the
internal Test Pattern Generator (TPG)?

When the CSID is linked to the internal TPG, csid->phy.csiphy_id is set to
the TPG index, and the pipeline does not have a sensor bound. If
csiphy->cfg.csi2 is NULL, lane_cfg evaluates to an invalid pointer (an
offset from NULL).

This invalid pointer is then passed to camss_get_link_freq(), which
unconditionally dereferences lane_cfg->num_data before verifying if a sensor
pad actually exists. This would trigger a kernel panic. Before this patch,
the code passed csid->phy.lane_cnt by value, safely returning -ENODEV for
TPG and gracefully defaulting to a zero fallback frequency.

>  	const struct csid_format_info *fmt;
> +
>  	s64 link_freq;
>  	int i, j;
>  	int ret;
>  
>  	fmt = csid_get_fmt_entry(csid->res->formats->formats, csid->res->formats->nformats,
>  				 csid->fmt[MSM_CSIPHY_PAD_SINK].code);
> -	link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp,
> -					csid->phy.lane_cnt);
> +
> +	link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp, lane_cfg);

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 072c428e25166..db4e14a84a95f 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -4623,20 +4631,24 @@ struct media_pad *camss_find_sensor_pad(struct media_entity *entity)
>   * camss_get_link_freq - Get link frequency from sensor
>   * @entity: Media entity in the current pipeline
>   * @bpp: Number of bits per pixel for the current format
> - * @lanes: Number of lanes in the link to the sensor
> + * @lane_cfg: CSI2 lane configuration
>   *
>   * Return link frequency on success or a negative error code otherwise
>   */
>  s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> -			unsigned int lanes)
> +			struct csiphy_lanes_cfg *lane_cfg)
>  {
>  	struct media_pad *sensor_pad;
> +	u8 num_lanes = lane_cfg->num_data;
> +	bool cphy = lane_cfg->phy_cfg == V4L2_MBUS_CSI2_CPHY;
> +	unsigned int div = num_lanes * 2 * (cphy ? CAMSS_CPHY_DIVISOR :
> +						     CAMSS_DPHY_DIVISOR);
>  
>  	sensor_pad = camss_find_sensor_pad(entity);
>  	if (!sensor_pad)
>  		return -ENODEV;
>  
> -	return v4l2_get_link_freq(sensor_pad, bpp, 2 * lanes);
> +	return v4l2_get_link_freq(sensor_pad, CAMSS_COMMON_PHY_DIVIDENT * bpp, div);

[Severity: High]
Does this calculation invert the C-PHY link frequency ratio?

According to the MIPI C-PHY specification and V4L2 documentation, 16 bits encode
into 7 symbols, meaning the symbol rate (link frequency) should be:
(pixel_rate * bpp * 7) / (16 * num_lanes).

Here, the multiplier passed to v4l2_get_link_freq() is (16 * bpp), and the
divisor is (num_lanes * 2 * 7) for C-PHY. This effectively multiplies by 16
and divides by 14, instead of multiplying by 7 and dividing by 16.

This logic appears to inflate the calculated link frequency for C-PHY by a
factor of roughly 2.6x, which could lead to pixel clock configuration failures
or physical hardware malfunction.

>  }
>  
>  /*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-qcom-cphy-v7-0-426c37e9008f@ixit.cz?part=7