[PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[Xiao Lu]

When an MST branch device is enumerated, drm_dp_mst_port_add_connector()
is called for every output port regardless of whether a device is actually
connected (ddps=0). This causes the driver's add_connector callback to
probe DSC capabilities via REMOTE_DPCD_READ transactions on empty ports,
which the hub NAKs with DPCD_FAIL.

These spurious sideband messages are wasteful and generate unnecessary
error log noise. More importantly, they occur repeatedly on every topology
re-enumeration triggered by CSN handling.

Skip connector creation for output ports that have no device plugged in
(ddps=0). When a device is later connected, the hub sends a Connection
Status Notification (CSN) which updates ddps=1 and triggers connector
creation through the normal hot-plug path, ensuring correct behavior
for runtime plug events.

Tested with a Parade PS8650 MST hub having unplugged downstream ports:
REMOTE_DPCD_READ NAK messages are eliminated while hot-plug and
display functionality on active ports remain unaffected.

Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7ce9e2127..14a84a838 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2448,7 +2448,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 
 	if (port->connector)
 		drm_modeset_unlock(&mgr->base.lock);
-	else if (!port->input)
+	else if (!port->input && port->ddps)
 		drm_dp_mst_port_add_connector(mstb, port);
 
 	if (send_link_addr && port->mstb) {
-- 
2.43.0

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/dp/mst: missing connector creation breaks hotplug for empty downstream ports
--

commit b7642d2c459f1deba3af19a444979a2c15cbf1e0
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/dp/mst: skip connector creation for unplugged downstream ports

This patch modifies the MST link enumeration path to skip creating DRM
connectors for downstream ports that do not have a device connected.
It aims to reduce error log noise and spurious sideband messages from
DPCD reads on empty ports.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2448,7 +2448,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>  
>  	if (port->connector)
>  		drm_modeset_unlock(&mgr->base.lock);
> -	else if (!port->input)
> +	else if (!port->input && port->ddps)
>  		drm_dp_mst_port_add_connector(mstb, port);

[Severity: High]
If we skip connector creation here for empty ports, will the hotplug path
actually create the connector when a monitor is plugged in?

Looking at drm_dp_mst_handle_conn_stat(), create_connector is only set to
true when transitioning from an input port to an output port:

    } else if (port->input && !conn_stat->input_port) {
        create_connector = true;

For standard downstream ports, port->input is 0, so create_connector
remains false during a hotplug event.

Later in drm_dp_mst_handle_conn_stat(), it checks:

    else if (create_connector)
        drm_dp_mst_port_add_connector(mstb, port);

Since create_connector evaluated to false, the dynamic connector creation
is bypassed.

Also, drm_dp_port_set_pdt() returns 0 for regular SST monitors:

        if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
            /* add i2c over sideband */
            ret = drm_dp_mst_register_i2c_bus(port);

This prevents the caller from setting dowork = true and triggering a
fallback link address reprobe.

Does this mean a display plugged into an MST hub port that was empty at
boot will remain permanently dark because its DRM connector is never
created?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917080126.1729898-1-xiaolu.xie@intel.com?part=1

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[Xie, Xiaolu]

[-- Attachment #1: Type: text/plain, Size: 3633 bytes --]

The connector creation is not permanently lost for ports that are empty
at initial enumeration. When a device is later plugged in, the hub sends
a Connection Status Notification (CSN) which triggers drm_dp_mst_handle_conn_stat().
The pdt field transitions from DP_PEER_DEVICE_NONE to the actual peer
device type, causing drm_dp_port_set_pdt() to return 1 and set dowork=true.
This queues mgr->work, which runs drm_dp_mst_link_probe_work() and
re-sends the LINK_ADDRESS to the branch device. The resulting re-enumeration
calls drm_dp_mst_handle_link_address_port() with ddps=1, which now passes
the !port->input && port->ddps check and correctly creates the connector.

Verified by testing: hub inserted with empty ports (no spurious NAK),
followed by plugging a display into a downstream port - display lights up
normally.
________________________________
From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
Sent: Thursday, September 17, 2026 4:17 PM
To: Xie, Xiaolu <xiaolu.xie@intel.com>
Cc: dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/dp/mst: missing connector creation breaks hotplug for empty downstream ports
--

commit b7642d2c459f1deba3af19a444979a2c15cbf1e0
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/dp/mst: skip connector creation for unplugged downstream ports

This patch modifies the MST link enumeration path to skip creating DRM
connectors for downstream ports that do not have a device connected.
It aims to reduce error log noise and spurious sideband messages from
DPCD reads on empty ports.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2448,7 +2448,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>
>        if (port->connector)
>                drm_modeset_unlock(&mgr->base.lock);
> -     else if (!port->input)
> +     else if (!port->input && port->ddps)
>                drm_dp_mst_port_add_connector(mstb, port);

[Severity: High]
If we skip connector creation here for empty ports, will the hotplug path
actually create the connector when a monitor is plugged in?

Looking at drm_dp_mst_handle_conn_stat(), create_connector is only set to
true when transitioning from an input port to an output port:

    } else if (port->input && !conn_stat->input_port) {
        create_connector = true;

For standard downstream ports, port->input is 0, so create_connector
remains false during a hotplug event.

Later in drm_dp_mst_handle_conn_stat(), it checks:

    else if (create_connector)
        drm_dp_mst_port_add_connector(mstb, port);

Since create_connector evaluated to false, the dynamic connector creation
is bypassed.

Also, drm_dp_port_set_pdt() returns 0 for regular SST monitors:

        if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
            /* add i2c over sideband */
            ret = drm_dp_mst_register_i2c_bus(port);

This prevents the caller from setting dowork = true and triggering a
fallback link address reprobe.

Does this mean a display plugged into an MST hub port that was empty at
boot will remain permanently dark because its DRM connector is never
created?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260917080126.1729898-1-xiaolu.xie@intel.com?part=1

[-- Attachment #2: Type: text/html, Size: 7430 bytes --]

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[lyude]

Are we sure this is a good idea? This seems like it could be a problem
for compositors and just make things more complicated in general for
userspace because now instead of a connector that can be plugged or
unplugged, we now only have connectors on MST that appear and
disappear.

Is there an actual bug being caused by the NAK transactions here?

On Thu, 2026-09-17 at 16:01 +0800, Xiao Lu wrote:
> When an MST branch device is enumerated,
> drm_dp_mst_port_add_connector()
> is called for every output port regardless of whether a device is
> actually
> connected (ddps=0). This causes the driver's add_connector callback
> to
> probe DSC capabilities via REMOTE_DPCD_READ transactions on empty
> ports,
> which the hub NAKs with DPCD_FAIL.
> 
> These spurious sideband messages are wasteful and generate
> unnecessary
> error log noise. More importantly, they occur repeatedly on every
> topology
> re-enumeration triggered by CSN handling.
> 
> Skip connector creation for output ports that have no device plugged
> in
> (ddps=0). When a device is later connected, the hub sends a
> Connection
> Status Notification (CSN) which updates ddps=1 and triggers connector
> creation through the normal hot-plug path, ensuring correct behavior
> for runtime plug events.
> 
> Tested with a Parade PS8650 MST hub having unplugged downstream
> ports:
> REMOTE_DPCD_READ NAK messages are eliminated while hot-plug and
> display functionality on active ports remain unaffected.
> 
> Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 7ce9e2127..14a84a838 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2448,7 +2448,7 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
>  
>  	if (port->connector)
>  		drm_modeset_unlock(&mgr->base.lock);
> -	else if (!port->input)
> +	else if (!port->input && port->ddps)
>  		drm_dp_mst_port_add_connector(mstb, port);
>  
>  	if (send_link_addr && port->mstb) {

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[Xiao Lu]

On Thu, 2026-09-17 at 12:49 -0400, Lyude Paul wrote:
> Are we sure this is a good idea? This seems like it could be a problem
> for compositors and just make things more complicated in general for
> userspace because now instead of a connector that can be plugged or
> unplugged, we now only have connectors on MST that appear and
> disappear.
>
> Is there an actual bug being caused by the NAK transactions here?

Thanks for the review. Two points to address your concerns:

1. We tested hot-plug/unplug on MST DFP ports with this patch applied,
   and the connectors appear and disappear correctly without any issues
   observed on the compositor side. When a device is plugged in, the
   hub sends a CSN which triggers a pdt change, causing mgr->work to
   resend LINK_ADDRESS. drm_dp_mst_handle_link_address_port() then
   creates the connector with ddps=1 at that point. The hot-plug
   lifecycle works correctly in practice. That said, if you are aware
   of a specific compositor path that relies on connectors being
   pre-created for all ports regardless of ddps, we are happy to
   investigate further.

2. The REMOTE_DPCD_READ to ports with ddps=0 is not just log noise.
   It causes a measurable lighting delay at link training time, as the
   transaction must time out or be NAK'd before the stack can proceed.
   More importantly, not all MST hubs respond gracefully to REMOTE_DPCD
   transactions on unoccupied ports. This behavior is also
   non-compliant with DP v2.1b spec, which states that REMOTE_DPCD_READ
   should only be issued to ports where ddps=1. Issuing reads to
   ddps=0 ports is the driver-side bug here.

Best regards,
Xiao Lu

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[lyude]

On Fri, 2026-09-18 at 09:29 +0800, Xiao Lu wrote:
> On Thu, 2026-09-17 at 12:49 -0400, Lyude Paul wrote:
> > Are we sure this is a good idea? This seems like it could be a
> > problem
> > for compositors and just make things more complicated in general
> > for
> > userspace because now instead of a connector that can be plugged or
> > unplugged, we now only have connectors on MST that appear and
> > disappear.
> > 
> > Is there an actual bug being caused by the NAK transactions here?
> 
> Thanks for the review. Two points to address your concerns:
> 
> 1. We tested hot-plug/unplug on MST DFP ports with this patch
> applied,
>    and the connectors appear and disappear correctly without any
> issues
>    observed on the compositor side. When a device is plugged in, the
>    hub sends a CSN which triggers a pdt change, causing mgr->work to
>    resend LINK_ADDRESS. drm_dp_mst_handle_link_address_port() then
>    creates the connector with ddps=1 at that point. The hot-plug
>    lifecycle works correctly in practice. That said, if you are aware
>    of a specific compositor path that relies on connectors being
>    pre-created for all ports regardless of ddps, we are happy to
>    investigate further.
> 
> 2. The REMOTE_DPCD_READ to ports with ddps=0 is not just log noise.
>    It causes a measurable lighting delay at link training time, as
> the
>    transaction must time out or be NAK'd before the stack can
> proceed.
>    More importantly, not all MST hubs respond gracefully to
> REMOTE_DPCD
>    transactions on unoccupied ports. This behavior is also
>    non-compliant with DP v2.1b spec, which states that
> REMOTE_DPCD_READ
>    should only be issued to ports where ddps=1. Issuing reads to
>    ddps=0 ports is the driver-side bug here.

OK - yeah, I definitely agree then that we've gotta enforce that.

This being said though - I do still think we want to keep the
connectors around, in part because it does give userspace a more
accurate idea of what ports are actually available on a hub - and it
would be a bit of a departure from how we've exposed this to userspace
for a while now. Dynamic connectors are already confusing enough to
deal with in userspace as it is.

Have you considered just keeping the connectors with ddps=0, and then
just making it so that our code for performing remote DPCD reads/writes
simply returns -EIO on ports where ddps=0? That way we still keep
connectors around without delaying or breaking anything.

> 
> Best regards,
> Xiao Lu

Re: [PATCH] drm/dp/mst: skip connector creation for unplugged downstream ports

[Xiao Lu]

On Thu, 2026-09-17 at 12:49 -0400, Lyude Paul wrote:
> OK - yeah, I definitely agree then that we've gotta enforce that.
>
> This being said though - I do still think we want to keep the
> connectors around, in part because it does give userspace a more
> accurate idea of what ports are actually available on a hub - and it
> would be a bit of a departure from how we've exposed this to userspace
> for a while now. Dynamic connectors are already confusing enough to
> deal with in userspace as it is.
>
> Have you considered just keeping the connectors with ddps=0, and then
> just making it so that our code for performing remote DPCD reads/writes
> simply returns -EIO on ports where ddps=0? That way we still keep
> connectors around without delaying or breaking anything.

Thanks, that is a much cleaner approach. v2 adopts your suggestion:
connectors are now created for all output ports regardless of ddps,
and the ddps=0 guard is moved into drm_dp_mst_dpcd_read() and
drm_dp_mst_dpcd_write(), returning -EIO immediately for unoccupied
ports. This prevents the spurious sideband transactions without any
change to connector lifetime semantics visible to userspace.

Best regards,
Xiao Lu

[PATCH v2] drm/dp/mst: reject DPCD read/write on ports with ddps=0

[Xiao Lu]

DP v2.1b specifies that REMOTE_DPCD_READ and REMOTE_DPCD_WRITE
transactions should only be issued to downstream ports where ddps=1
(device plug status indicates a device is connected). Issuing these
transactions to empty ports (ddps=0) is non-compliant and causes
problems in practice: some MST hubs NAK the request, and all hubs
introduce a delay waiting for the transaction to complete or fail
before the driver can proceed, increasing link training latency.

Guard drm_dp_mst_dpcd_read() and drm_dp_mst_dpcd_write() with a
ddps check and return -EIO immediately for unoccupied ports. This
avoids the spurious sideband transactions without changing connector
lifetime semantics — connectors for all output ports are still
created at enumeration time, regardless of ddps.

Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>

Changes in v2:
- Instead of skipping connector creation for ddps=0 ports (v1),
  keep connectors around and guard DPCD transactions at the
  drm_dp_mst_dpcd_read/write level. This avoids changing connector
  lifetime semantics visible to userspace, per Lyude's review.
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7ce9e2127..b7fce0bf8 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2161,6 +2161,9 @@ ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
 	struct drm_dp_mst_port *port = container_of(aux, struct drm_dp_mst_port,
 						    aux);
 
+	if (!port->ddps)
+		return -EIO;
+
 	return drm_dp_send_dpcd_read(port->mgr, port,
 				     offset, size, buffer);
 }
@@ -2184,6 +2187,9 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
 	struct drm_dp_mst_port *port = container_of(aux, struct drm_dp_mst_port,
 						    aux);
 
+	if (!port->ddps)
+		return -EIO;
+
 	return drm_dp_send_dpcd_write(port->mgr, port,
 				      offset, size, buffer);
 }
-- 
2.43.0

Re: [PATCH v2] drm/dp/mst: reject DPCD read/write on ports with ddps=0

[lyude]

Awesome! Happy with this being the new behavior for DPCD transactions,
but there's one thing I realized we should probably verify though
before I give an R-b: Is it possible that this can race with CSN
updates, and do we handle the race/do we need to?

For context: port->ddps is protected under drm_dp_mst_topology_mgr-
>base.lock, and we're reading out of lock here. So the situation I'm
thinking of is like this:

- Thread #1 starts a remote DPCD transaction on a port with ddps=1
  It performs the ddps check, and begins doing the actual transaction
- Thread #1 gets preempted, execution stops
- In the real world, the port is unplugged and we get a CSN
- We handle the CSN on another thread, and ack it to the MST hub
- Thread #1 starts executing again, and continues the remote DPCD
transaction without knowing the port is gone

To be clear, I don't think we can solve this by just grabbing the lock
during the `if` check. This brings the question of "are we able to
handle violating this portion of the spec if the chances of that
happening in the real world are very slim?" E.g. is a race like this
with a hub still recoverable, or is it definitely possibly fatal on
some hubs? If it is recoverable, this is probably an issue we can
ignore and I can just give an R-B since 99.9% of the time we'll prevent
DPCD transactions on unplugged ports.

If we can't, then I wonder if it's possible to introduce some sort of
ddps check within drm_dp_send_dpcd_read/drm_dp_send_dpcd_write() that
happens under a lock which we also acquire during CSN handling, and
drop the lock for it after we queue the TX (and before we try to wait
for the response from the hub). That way we could make sure that
whatever ddps value we use for possibly preventing the read/write is
always up to date with the last value ddps value that that we ack'd
from the hub is and block acknowledging any CSN updates while a DPCD
transaction is ongoing. This means a port could still be unplugged
while we're sending DPCD traffic to it of course, but that's to be
expected - as long as we haven't ACK'd the CSN yet it should be the
hub's responsibility to recover from the sudden disconnect mid-DPCD
transaction.

Does that make sense?

On Sun, 2026-09-20 at 10:21 +0800, Xiao Lu wrote:
> DP v2.1b specifies that REMOTE_DPCD_READ and REMOTE_DPCD_WRITE
> transactions should only be issued to downstream ports where ddps=1
> (device plug status indicates a device is connected). Issuing these
> transactions to empty ports (ddps=0) is non-compliant and causes
> problems in practice: some MST hubs NAK the request, and all hubs
> introduce a delay waiting for the transaction to complete or fail
> before the driver can proceed, increasing link training latency.
> 
> Guard drm_dp_mst_dpcd_read() and drm_dp_mst_dpcd_write() with a
> ddps check and return -EIO immediately for unoccupied ports. This
> avoids the spurious sideband transactions without changing connector
> lifetime semantics — connectors for all output ports are still
> created at enumeration time, regardless of ddps.
> 
> Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
> 
> Changes in v2:
> - Instead of skipping connector creation for ddps=0 ports (v1),
>   keep connectors around and guard DPCD transactions at the
>   drm_dp_mst_dpcd_read/write level. This avoids changing connector
>   lifetime semantics visible to userspace, per Lyude's review.
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 7ce9e2127..b7fce0bf8 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2161,6 +2161,9 @@ ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux
> *aux,
>  	struct drm_dp_mst_port *port = container_of(aux, struct
> drm_dp_mst_port,
>  						    aux);
>  
> +	if (!port->ddps)
> +		return -EIO;
> +
>  	return drm_dp_send_dpcd_read(port->mgr, port,
>  				     offset, size, buffer);
>  }
> @@ -2184,6 +2187,9 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux
> *aux,
>  	struct drm_dp_mst_port *port = container_of(aux, struct
> drm_dp_mst_port,
>  						    aux);
>  
> +	if (!port->ddps)
> +		return -EIO;
> +
>  	return drm_dp_send_dpcd_write(port->mgr, port,
>  				      offset, size, buffer);
>  }

Re: [PATCH v2] drm/dp/mst: reject DPCD read/write on ports with ddps=0

[Xiao Lu]

On ..., Lyude Paul wrote:
> I wonder if it's possible to introduce some sort of ddps check within
> drm_dp_send_dpcd_read/drm_dp_send_dpcd_write() that happens under a
> lock which we also acquire during CSN handling...

Thank you for the detailed explanation. I understand the proposed
approach — holding a dedicated lock around the ddps check and the
queue_tx call, with the same lock held on the CSN ack side, so the
ddps value used in the check always reflects the last ack'd CSN.

That said, after looking at this more carefully, I think the current
v2 change is sufficient for the following reasons:

1. The only paths that issue REMOTE_DPCD_READ/WRITE to downstream
   ports without first verifying ddps are triggered during connector
   probing in the driver's add_connector callback (e.g.
   intel_dp_mst_read_decompression_port_dsc_caps() in i915/xe), which
   is called from drm_dp_mst_link_probe_work(). This probe work fires
   when the MST hub UFP is first connected, on topology resume, and
   on each CSN reception. The race window — a downstream port gets
   unplugged between the ddps check and the actual AUX transaction
   during a probe — is extremely narrow in practice.

2. Our primary goal with this change is to eliminate unnecessary
   REMOTE_DPCD_READ transactions that cause lighting delay at link
   training time, rather than strict spec enforcement. We believe the
   vast majority of MST hubs already handle reads/writes to unplugged
   downstream ports gracefully. DP v2.1b Table 2-199 describes the
   DPCD_FAIL NAK with bad-param for such cases, and implementing this
   response in hub firmware should not impose significant burden on the
   hub.

In summary, this is an optimization to improve latency rather than a
correctness fix, and we consider the risk of the narrow race window
acceptable.

Best regards,
Xiao Lu

Re: [PATCH v2] drm/dp/mst: reject DPCD read/write on ports with ddps=0

[lyude]

Sounds great to me! Then this patch is:

Reviewed-by: Lyude Paul <lyude@redhat.com>

Are you able to push this upstream or would you rather I push it?

On Tue, 2026-09-22 at 11:26 +0800, Xiao Lu wrote:
> On ..., Lyude Paul wrote:
> > I wonder if it's possible to introduce some sort of ddps check
> > within
> > drm_dp_send_dpcd_read/drm_dp_send_dpcd_write() that happens under a
> > lock which we also acquire during CSN handling...
> 
> Thank you for the detailed explanation. I understand the proposed
> approach — holding a dedicated lock around the ddps check and the
> queue_tx call, with the same lock held on the CSN ack side, so the
> ddps value used in the check always reflects the last ack'd CSN.
> 
> That said, after looking at this more carefully, I think the current
> v2 change is sufficient for the following reasons:
> 
> 1. The only paths that issue REMOTE_DPCD_READ/WRITE to downstream
>    ports without first verifying ddps are triggered during connector
>    probing in the driver's add_connector callback (e.g.
>    intel_dp_mst_read_decompression_port_dsc_caps() in i915/xe), which
>    is called from drm_dp_mst_link_probe_work(). This probe work fires
>    when the MST hub UFP is first connected, on topology resume, and
>    on each CSN reception. The race window — a downstream port gets
>    unplugged between the ddps check and the actual AUX transaction
>    during a probe — is extremely narrow in practice.
> 
> 2. Our primary goal with this change is to eliminate unnecessary
>    REMOTE_DPCD_READ transactions that cause lighting delay at link
>    training time, rather than strict spec enforcement. We believe the
>    vast majority of MST hubs already handle reads/writes to unplugged
>    downstream ports gracefully. DP v2.1b Table 2-199 describes the
>    DPCD_FAIL NAK with bad-param for such cases, and implementing this
>    response in hub firmware should not impose significant burden on
> the
>    hub.
> 
> In summary, this is an optimization to improve latency rather than a
> correctness fix, and we consider the risk of the narrow race window
> acceptable.
> 
> Best regards,
> Xiao Lu

Re: [PATCH v2] drm/dp/mst: reject DPCD read/write on ports with ddps=0

[Xiao Lu]

Thank you for the review, Lyude!

I don't have commit access to drm-misc-next, so please push at your
convenience.

Best regards,
Xiao Lu