Re: [PATCH v9 53/61] drm/tests: connector: Add HDMI max_tmds_char_rate init coverage

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Flawed KUnit test logic causes TMDS character rate limit rejection tests to pass for the wrong reason.
- [Medium] Dangling pointer created by storing the address of a stack-allocated structure into a persistent device-managed object.
--

--- Patch [53]: [PATCH v9 53/61] drm/tests: connector: Add HDMI max_tmds_char_rate init coverage ---
commit 7359a144a698a961aa27ac0fd6a0ec01ec7928f5
Author: Cristian Ciocaltea <[email protected]>

drm/tests: connector: Add HDMI max_tmds_char_rate init coverage

This commit adds KUnit tests for the new drmm_connector_hdmi_init()
extension to infer and validate the connector's max_tmds_char_rate based
on the advertised HDMI specification version.

> diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
> index 74342880b3015..f14cd741b05e4 100644
> --- a/drivers/gpu/drm/tests/drm_connector_test.c
> +++ b/drivers/gpu/drm/tests/drm_connector_test.c

[ ... ]

> @@ -1236,6 +1264,154 @@ KUNIT_ARRAY_PARAM(drm_connector_hdmi_init_type_invalid,
> +static void drm_test_connector_hdmi_init_max_tmds_rate_inferred(struct kunit *test)
> +{
> +	struct drm_connector_init_priv *priv = test->priv;
> +	const struct drm_connector_hdmi_init_max_tmds_rate_inferred_case *params =
> +		test->param_value;
> +	struct drm_connector_hdmi_funcs hdmi_funcs;
> +	int ret;
> +
> +	hdmi_funcs = params->ver < HDMI_VERSION_2_0 ?
> +			dummy_hdmi_funcs : dummy_hdmi_funcs_scrambler;
> +	hdmi_funcs.supported_hdmi_ver = params->ver;
> +
> +	ret = drmm_connector_hdmi_init(&priv->drm, &priv->connector,
> +				       &dummy_funcs,
> +				       &hdmi_funcs,
> +				       DRM_MODE_CONNECTOR_HDMIA,
> +				       &priv->ddc);

[Severity: Medium]
Does this create a dangling pointer?

drmm_connector_hdmi_init() stores the provided hdmi_funcs pointer in
connector->hdmi.funcs.

Since hdmi_funcs is allocated on the stack here (and in other new tests),
connector->hdmi.funcs becomes a dangling pointer when the test function
returns. Even if deferred device-managed cleanup doesn't dereference it
today, is it safe to leave this pointing to destroyed stack frames?

[ ... ]

> +/*
> + * Test that the registration of an HDMI connector providing a max TMDS
> + * character rate that exceeds the limit inferred from the advertised HDMI
> + * specification version fails.
> + */
> +static void drm_test_connector_hdmi_init_max_tmds_rate_exceeds(struct kunit *test)
> +{
> +	struct drm_connector_init_priv *priv = test->priv;
> +	struct drm_connector_hdmi_funcs hdmi_funcs = dummy_hdmi_funcs_scrambler;
> +	int ret;
> +
> +	hdmi_funcs.supported_hdmi_ver = HDMI_VERSION_1_4;
> +	hdmi_funcs.supported_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> +
> +	ret = drmm_connector_hdmi_init(&priv->drm, &priv->connector,
> +				       &dummy_funcs,
> +				       &hdmi_funcs,
> +				       DRM_MODE_CONNECTOR_HDMIA,
> +				       &priv->ddc);
> +	KUNIT_EXPECT_LT(test, ret, 0);
> +}

[Severity: Medium]
Are these tests passing for the wrong reason?

Because dummy_hdmi_funcs_scrambler provides scrambler callbacks, setting
hdmi_funcs.supported_hdmi_ver = HDMI_VERSION_1_4 causes
drmm_connector_hdmi_init() to return -EINVAL early:

	if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
		drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
		return -EINVAL;
	}

This bypasses the TMDS rate logic completely, meaning the expectation
KUNIT_EXPECT_LT(test, ret, 0) succeeds without actually testing the intended
rate limit logic.

[ ... ]

> +/*
> + * Test that the registration of an HDMI connector providing a non-zero max
> + * TMDS character rate without an HDMI specification version fails, as the
> + * version-inferred limit defaults to zero and any positive override would
> + * exceed it.
> + */
> +static void drm_test_connector_hdmi_init_max_tmds_rate_no_version(struct kunit *test)
> +{
> +	struct drm_connector_init_priv *priv = test->priv;
> +	struct drm_connector_hdmi_funcs hdmi_funcs = dummy_hdmi_funcs_scrambler;
> +	int ret;
> +
> +	hdmi_funcs.supported_hdmi_ver = HDMI_VERSION_UNKNOWN;
> +	hdmi_funcs.supported_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> +
> +	ret = drmm_connector_hdmi_init(&priv->drm, &priv->connector,
> +				       &dummy_funcs,
> +				       &hdmi_funcs,
> +				       DRM_MODE_CONNECTOR_HDMIA,
> +				       &priv->ddc);
> +	KUNIT_EXPECT_LT(test, ret, 0);
> +}

[Severity: Medium]
Does this test have the exact same logic flaw as above? 

It uses dummy_hdmi_funcs_scrambler with HDMI_VERSION_UNKNOWN, causing early
rejection due to the scrambler checks rather than testing the TMDS rate
validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=53