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
