Re: [PATCH v10 04/21] drm/bridge: synopsys: dw-dp: Document missing reset line deassert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing reinit_completion in dw_dp_aux_transfer causes AUX transfer corruption after timeouts.
- [High] Sleeping delay functions are used in atomic commit paths, violating DRM subsystem guidelines.
--

--- Patch [4]: [PATCH v10 04/21] drm/bridge: synopsys: dw-dp: Document missing reset line deassert ---
commit c7f9030ef61e21ab471dde4c6e59c410a28a4503
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Document missing reset line deassert

This commit documents an issue where the SoC will hang if the reset line
is automatically deasserted during probe using
devm_reset_control_get_exclusive_deasserted(). It leaves the existing
devm_reset_control_get() call in place to avoid the hang.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index dd951fd447227..fd7663013faaa 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2095,6 +2095,10 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  		return ERR_CAST(dp->hdcp_clk);
>  	}
>  
> +	/*
> +	 * This reset line is deasserted by default; asserting it hangs the SoC if the
> +	 * related power-domain is still active.
> +	 */
>  	dp->rstc = devm_reset_control_get(dev, NULL);
>  	if (IS_ERR(dp->rstc)) {
>  		dev_err_probe(dev, PTR_ERR(dp->rstc), "failed to get reset control\n");

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the DP driver
I noticed a potential atomic context violation in dw_dp_link_train_fast():

    ret = dw_dp_link_train_set_pattern(dp, DP_TRAINING_PATTERN_1);
    if (ret)
        goto out;

    usleep_range(500, 1000);

Can this sleep function be reached from atomic commit paths?

Looking at dw_dp_bridge_atomic_enable(), it appears this function
is called during the modeset. The DRM subsystem guide indicates that
atomic enable callbacks run in an atomic context where sleeping
is forbidden, which could lead to scheduling while atomic panics.

Would it be better to replace usleep_range() with a non-sleeping
delay like udelay() or ndelay() in this path?

[Severity: High]
This is another pre-existing issue, but I noticed a potential problem with
how wait_for_completion_timeout() is used in dw_dp_aux_transfer():

    value |= FIELD_PREP(AUX_CMD_TYPE, msg->request);
    value |= FIELD_PREP(AUX_ADDR, msg->address);
    regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

    status = wait_for_completion_timeout(&dp->complete, timeout);

Could this lead to stale data being read? 

If a previous AUX transfer times out and the hardware completes the
interrupt shortly after, the completion's internal counter would increment.
A subsequent call to dw_dp_aux_transfer() might return immediately from
wait_for_completion_timeout() without waiting for the new transaction,
causing the driver to read garbage data from the AUX status registers.

Should there be a call to reinit_completion(&dp->complete) before
writing to DW_DP_AUX_CMD?

[Severity: High]
This is a pre-existing issue as well, but I noticed another potential
atomic context violation in dw_dp_reset():

    disable_irq(dp->irq);
    regmap_update_bits(dp->regmap, DW_DP_SOFT_RESET_CTRL, CONTROLLER_RESET,
               FIELD_PREP(CONTROLLER_RESET, 1));
    usleep_range(10, 20);
    regmap_update_bits(dp->regmap, DW_DP_SOFT_RESET_CTRL, CONTROLLER_RESET,
               FIELD_PREP(CONTROLLER_RESET, 0));

    dw_dp_init_hw(dp);
    regmap_read_poll_timeout(dp->regmap, DW_DP_HPD_STATUS, val,
                 FIELD_GET(HPD_HOT_PLUG, val), 200, 200000);

Does dw_dp_bridge_atomic_disable() eventually call this reset function?

Since dw_dp_bridge_atomic_disable() is an atomic commit callback,
using usleep_range() and regmap_read_poll_timeout() (which sleeps)
here might cause system panics during a hot path. 

Might we need to switch to non-sleeping equivalents or move this
initialization out of the atomic commit path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c303d6@collabora.com?part=4