Re: [PATCH v6 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 3 potential issue(s) to consider:

New issues:
- [High] The link frequency calculation for C-PHY in `camss_get_link_freq()` uses mathematically inverted constants, resulting in a vastly incorrect and excessively high link frequency.
- [High] A guaranteed NULL pointer dereference occurs in `camss_get_link_freq()` when the pipeline is configured to use the internal Test Pattern Generator (TPG).

Pre-existing issues:
- [High] Unvalidated Device Tree endpoint port leads to an out-of-bounds pointer write.
--

commit 021fbf4d7c8dcb172343f7479e2fdcd4714092bc
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
> --- 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]
Does this code introduce a regression when the test pattern generator is used?
When the csid is linked to the internal test pattern generator,
csiphy->cfg.csi2 is NULL because the generator is an internal virtual block.
Will this cause an invalid pointer to be evaluated here and passed to
camss_get_link_freq(), leading to a crash when it dereferences
lane_cfg->num_data?

>  	const struct csid_format_info *fmt;

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> --- 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 regression invert the multiplier and divisor constants for C-PHY?
The V4L2 documentation and standard C-PHY formula define the pixel rate
calculation as pixel_rate = link_freq * 2 * nr_of_lanes * 16 / 7 /
bits_per_sample.

By passing mul = 16 * bpp and div = 14 * num_lanes to v4l2_get_link_freq(),
the resulting calculated frequency appears to be over 5 times higher than the
correct value, which might render C-PHY unusable.

[Severity: High]
This isn't a bug introduced by this patch, but does camss_parse_endpoint_node()
properly validate the Device Tree endpoint port? Currently, vep.base.port is
assigned directly to csd->interface.csiphy_id without bounds checking:

camss_parse_endpoint_node() {
    ...
    csd->interface.csiphy_id = vep.base.port;
    ...
}

Then later in camss_subdev_notifier_bound(), this unchecked ID is used as
an index:

camss_subdev_notifier_bound() {
    ...
    u8 id = csd->interface.csiphy_id;
    struct csiphy_device *csiphy = &camss->csiphy[id];

    csiphy->cfg.csi2 = &csd->interface.csi2;
    ...
}

Can a malformed Device Tree cause an arbitrary out-of-bounds pointer write
here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-qcom-cphy-v6-0-e50de0b557a8@ixit.cz?part=7