Re: [PATCH v4 1/8] drm/connector: report out-of-band IRQ_HPD events

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

Pre-existing issues:
- [High] State machine race condition causes premature HPD (and IRQ) signaling before Alt Mode configuration completes.
- [High] DisplayPort is erroneously reported as connected (with IRQ) when CONFIGURE command is NAKed.
--

commit 3c420485b2e1143aa3ad7bdf4a86786959837b5a
Author: Dmitry Baryshkov <[email protected]>

drm/connector: report out-of-band IRQ_HPD events

The DisplayPort standard defines a special kind of events called IRQ,
used to notify DP Source about the events on the Sink side. This commit
renames drm_connector_oob_hotplug_event() to drm_connector_dp_oob_status()
and extends its interface to report IRQ events to the DisplayPort Sink drivers.

> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 263a89c5f3243..ff3659b8f5a2c 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -187,9 +187,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
>  				dp->pending_irq_hpd = true;
>  		}
>  	} else {
> -		drm_connector_oob_hotplug_event(dp->connector_fwnode,
> +		drm_connector_dp_oob_status(dp->connector_fwnode,
>  						hpd ? connector_status_connected :
> -						      connector_status_disconnected);
> +						      connector_status_disconnected,
> +						(hpd && irq_hpd) ? DRM_CONNECTOR_DP_IRQ_HPD :
> +								   DRM_CONNECTOR_NO_EXTRA_STATUS);

[Severity: High]
This is a pre-existing issue, but since we are modifying the hotplug
notification here, can a state machine race condition cause premature HPD
signaling before Alt Mode configuration completes?

If an Attention VDM arrives before the ACK for the DP_CMD_CONFIGURE,
dp_altmode_attention() will process it immediately because dp_altmode_work()
already set dp->state to DP_STATE_IDLE after sending the async command.

In dp_altmode_status_update(), the check for
!(con & DP_CONF_CURRENTLY(dp->data.conf)) evaluates to false because
dp->data.conf was already updated prior to sending the configure command.

Execution then falls into this else block, which calls
drm_connector_dp_oob_status() with connector_status_connected prematurely.

Does this cause DRM to begin link training before the physical MUX is
configured (which only happens in dp_altmode_notify() when the ACK arrives),
leading to a failed link and non-functional display?

>  		dp->hpd = hpd;
>  		sysfs_notify(&dp->alt->dev.kobj, "displayport", "hpd");

[ ... ]

> @@ -211,8 +213,11 @@ static int dp_altmode_configured(struct dp_altmode *dp)
>  	 * configuration is complete to signal HPD.
>  	 */
>  	if (dp->pending_hpd) {
> -		drm_connector_oob_hotplug_event(dp->connector_fwnode,
> -						connector_status_connected);
> +		drm_connector_dp_oob_status(dp->connector_fwnode,
> +						connector_status_connected,
> +						dp->pending_irq_hpd ?
> +						DRM_CONNECTOR_DP_IRQ_HPD :
> +						DRM_CONNECTOR_NO_EXTRA_STATUS);

[Severity: High]
This isn't a bug introduced by this patch, but does this logic erroneously
report a connected state even when the configure command is NAKed?

When the peer NAKs the DP_CMD_CONFIGURE command, dp_altmode_vdm() handles
the NAK by resetting dp->data.conf = 0 and then calling dp_altmode_configured().

If a prior STATUS_UPDATE VDM arrived with HPD high, dp->pending_hpd
will still be true.

Because this block checks dp->pending_hpd without verifying if the
configuration actually succeeded (for example, by checking if
dp->data.conf != 0), it unconditionally calls drm_connector_dp_oob_status()
with connector_status_connected.

Meanwhile, dp_altmode_notify() sets the MUX to TYPEC_STATE_USB because
dp->data.conf is 0.

Could this cause DRM to attempt DP link training over an unrouted AUX
channel, resulting in repeated timeouts and desynchronized state?

>  		sysfs_notify(&dp->alt->dev.kobj, "displayport", "hpd");
>  		dp->pending_hpd = false;
>  		if (dp->pending_irq_hpd) {

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic