Re: [PATCH v2 10/10] drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

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

New issues:
- [High] Bypassing force_hpd initialization breaks AUX channel and link training

Pre-existing issues:
- [High] Sleeping in atomic context during bridge enable
--

--- Patch [10]: [PATCH v2 10/10] drm/bridge: analogix_dp: Handle HPD notification from downstream bridge ---
commit 9728fd12d41ed8de2891a5a3947c23bae76130a5
Author: Damon Ding <[email protected]>

drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

The patch implements the .hpd_notify bridge callback to receive connection
status updates from downstream bridges. It caches the connection status to
short-circuit analogix_dp_detect_hpd() when a downstream bridge has already
confirmed the connection.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index f7db5b5a3f917..14a78aea48bd7 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -68,6 +68,13 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>  {
>  	int timeout_loop = 0;
>  
> +	/*
> +	 * Trust connection status from downstream bridge (e.g.,
> +	 * display-connector with hpd-gpios).
> +	 */
> +	if (dp->plat_data->next_bridge && dp->connection_notified)
> +		return 0;

[Severity: High]
Does returning early here bypass the analogix_dp_force_hpd(dp) call needed
to unblock the AUX channel?

If the downstream bridge manages HPD, the native HPD pin is typically
unconnected, making the hardware HPD_STATUS register read 0. The Analogix
DP hardware blocks all AUX transactions when HPD_STATUS is 0.

By returning 0 early, analogix_dp_force_hpd(dp) is skipped, leaving the
F_HPD bit cleared and the AUX channel blocked. This could cause link
training to fail because it cannot read DPCD over the AUX channel.

>  	while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
>  		if (analogix_dp_get_plug_in_status(dp) == 0)
>  			return 0;
>  
>  		timeout_loop++;
>  		usleep_range(1000, 1100);

[Severity: High]
This isn't a bug introduced by this patch, but does this usleep_range()
violate atomic context requirements?

When the core DRM framework calls analogix_dp_bridge_atomic_enable() during
a non-blocking atomic commit, it executes analogix_dp_set_bridge() which
then calls analogix_dp_detect_hpd():

analogix_dp_bridge_atomic_enable()
  analogix_dp_set_bridge()
    analogix_dp_detect_hpd()
      usleep_range(1000, 1100);

Since atomic enable/disable callbacks execute in atomic context, sleeping
functions like usleep_range() are not allowed and can cause kernel
warnings (BUG: scheduling while atomic) or system instability.

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