Re: [PATCH v2 3/5] media: qcom: camss: Match VFE lite clocks by name prefix

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: qcom: camss: vfe_match_clock_names() matches vfe_lite_* clocks
--

commit 6c95d1dba299c3fc78f8daef87dbf333ccf58408
Author: Andrew Gigena <mail@andrewgigena.dev>

media: qcom: camss: Match VFE lite clocks by name prefix

This commit updates vfe_match_clock_names() to match any clock starting
with "vfe_lite" when the VFE is a lite instance, so that numbered VFE-lite
clocks (such as vfe_lite0 and vfe_lite1) have their rates configured even
when vfe->id does not match the lite instance index.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 2680a9914765f..9866ac09eefd7 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -914,6 +914,7 @@ static int vfe_match_clock_names(struct vfe_device *vfe,
>  	return (!strcmp(clock->name, vfe_name) ||
>  		!strcmp(clock->name, vfe_lite_name) ||
>  		!strcmp(clock->name, "vfe_lite") ||
> +		(vfe->res->is_lite && !strncmp(clock->name, "vfe_lite", 8)) ||

[Severity: High]
Does an 8-byte prefix match on "vfe_lite" in vfe_match_clock_names() also
match auxiliary VFE-lite clocks such as "vfe_lite_src", "vfe_lite_ahb",
"vfe_lite_csid", and "vfe_lite_cphy_rx" (whereas full VFE instances only
match exact "vfe%d" names and ignore "vfe%d_src", "vfe%d_fast_ahb",
"vfe%d_csid", and "vfe%d_cphy_rx")?

When starting a VFE-lite stream, vfe_get() calls vfe_set_clock_rates() on
initial power-up and vfe_check_clock_rates() on subsequent calls, and both
treat every clock matching vfe_match_clock_names() with non-zero
clock_rate entries as a core VFE clock that must support min_rate.

On SDM845, vfe_res_845 in drivers/media/platform/qcom/camss/camss.c lists
"vfe_lite_src" after "vfe_lite":

	.clock = { "camnoc_axi", "cpas_ahb", "slow_ahb_src",
			"soc_ahb", "vfe_lite",
			"vfe_lite_src", "csi2",
			"csi2_src"},
	.clock_rate = { { 0 },
			{ 0 },
			{ 80000000 },
			{ 0 },
			{ 19200000, 100000000, 320000000, 404000000, 480000000, 600000000 },
			{ 320000000 },

In drivers/clk/qcom/camcc-sdm845.c, cam_cc_ife_lite_clk ("vfe_lite") is a
child branch of cam_cc_ife_lite_clk_src ("vfe_lite_src") with
CLK_SET_RATE_PARENT. Because "vfe_lite_src" now matches
vfe_match_clock_names():
- When min_rate >= 320 MHz, vfe_set_clock_rates() finds no frequency above
  min_rate in the single-entry { 320000000 } table for "vfe_lite_src" and
  fails with -EINVAL ("Pixel clock is too high for VFE").
- When min_rate < 320 MHz or min_rate == 0 (where "vfe_lite" picks 600
  MHz), the subsequent loop iteration for "vfe_lite_src" calls
  clk_set_rate() with 320 MHz on the parent RCG, overwriting the rate just
  set via "vfe_lite".

Similarly, on SM8550 (vfe_res_8550), SA8775P / QCS8300 (vfe_res_8775p),
and X1E80100 (vfe_res_x1e80100), the VFE-lite resource tables define
non-zero clock_rate entries for "vfe_lite_ahb", "vfe_lite_csid", and
"vfe_lite_cphy_rx" capping at 400 MHz while "vfe_lite" goes up to 480 MHz
or 600 MHz:

	.clock = { "cpas_ahb", "gcc_axi_hf", "cpas_vfe_lite",
		   "vfe_lite_ahb", "vfe_lite_csid", "vfe_lite_cphy_rx",
		   "vfe_lite", "camnoc_axi"},
	.clock_rate = {
		{ 0 },
		{ 0 },
		{ 0 },
		{ 300000000, 400000000, 400000000, 400000000 },
		{ 400000000, 400000000, 400000000, 400000000 },
		{ 400000000, 400000000, 400000000, 400000000 },
		{ 480000000, 600000000, 600000000, 600000000 },

This causes two issues on those platforms:
- When min_rate > 400 MHz, vfe_set_clock_rates() fails with -EINVAL
  because those auxiliary clocks have no rate above 400 MHz.
- In drivers/clk/qcom/camcc-sm8550.c and drivers/clk/qcom/camcc-sa8775p.c,
  cam_cc_ife_lite_ahb_clk ("vfe_lite_ahb") is parented to
  cam_cc_slow_ahb_clk_src, which maxes out at 80 MHz in hardware. When
  vfe_get() is called a second time while vfe->power_count > 0 (for
  example from csid_set_power() -> parent_dev_ops->get() during
  v4l2_pipeline_pm_get(), or when enabling a second RDI line),
  vfe_check_clock_rates() checks clk_get_rate(clock->clk) < min_rate on
  "vfe_lite_ahb", sees 80 MHz, and returns -EBUSY whenever min_rate > 80
  MHz.

Would it be safer to ensure the character following "vfe_lite" is not '_'
(e.g. checking clock->name[8] != '_' or isdigit(clock->name[8])) so that
only "vfe_lite" and numbered "vfe_liteN" clocks are matched?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sm8150-v7.4-rc1-camss-v2-0-39ee37787b00@andrewgigena.dev?part=3