[PATCH v2] drm/amd/display: Route connector DDC over AUX for native DP sinks

[PATCH v2] drm/amd/display: Route connector DDC over AUX for native DP sinks

[Mario Limonciello]

On DisplayPort and eDP connectors DDC is tunneled over the AUX channel,
but the connector's "ddc" adapter ("AMDGPU DM i2c hw bus N") always
drives the physical i2c pins, which don't reach a native DP sink.  The
AUX backed adapter ("AMDGPU DM aux hw bus N") does, so userspace such as
fwupd, which uses the "ddc" sysfs link for DDC/CI, has to work around
this.

Keep a single i2c bus per connector and pick the transport per transfer
based on the detected sink: if the sink is a native DP/eDP/MST sink,
forward the transfer to the AUX adapter, otherwise (DVI/HDMI, including
DP++ with a passive adapter) use the hw i2c engine as before.

Reported-by: simonit.orig@gmail.com
Closes: https://github.com/fwupd/fwupd/pull/11308
Signed-off-by: Mario Limonciello <superm1@kernel.org>
---
Changes in v2:
- Check which i2c bus to use on the fly instead of changing static symlink to
  avoid regressing passive adapters.
- Link to v1: https://patch.msgid.link/20261102-superm1-amdgpu-ddc-v1-1-9c5cb4a4f62d@amd.com
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c  | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
index b498fc5fbb140..678caffa3d120 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
@@ -3373,6 +3373,22 @@ STATIC_IFN_KUNIT int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
 	int i;
 	int result = -EIO;
 
+	if (!i2c->oem) {
+		struct dc_link *link = ddc_service->link;
+		struct amdgpu_dm_connector *aconnector = link->priv;
+
+		/*
+		 * DDC to a native DisplayPort sink is tunneled over the AUX
+		 * channel; the physical i2c pins only reach DVI/HDMI sinks
+		 * (e.g. DP++ with a passive adapter).  Route accordingly so
+		 * the connector's single "ddc" bus works for either sink.
+		 */
+		if (aconnector && link->local_sink &&
+		    dc_is_dp_signal(link->local_sink->sink_signal) &&
+		    aconnector->dm_dp_aux.aux.ddc.algo)
+			return i2c_transfer(&aconnector->dm_dp_aux.aux.ddc, msgs, num);
+	}
+
 	if (!ddc_service->ddc_pin)
 		return result;
 

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261102-superm1-amdgpu-ddc-63f0eb10c022

Best regards,
--  
Mario Limonciello <superm1@kernel.org>

Re: [PATCH v2] drm/amd/display: Route connector DDC over AUX for native DP sinks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/amd/display: UAF race on link->local_sink in amdgpu_dm_i2c_xfer()
- [High] drm/amd/display: NULL link dereference in amdgpu_dm_i2c_xfer()
- [Medium] drm/amd/display: amdgpu_dm_i2c_xfer() misses MST and force_to_use_aux links
--

commit 7a7dcab86c8948b21ebe53eebc9df866754fd915
Author: Mario Limonciello <superm1@kernel.org>

drm/amd/display: Route connector DDC over AUX for native DP sinks

This commit updates amdgpu_dm_i2c_xfer() to route I2C transfers over the
DisplayPort AUX DDC adapter when a native DisplayPort, eDP, or MST sink is
detected on the link, while keeping the hardware I2C engine for DVI/HDMI
sinks.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index b498fc5fbb140..678caffa3d120 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> @@ -3373,6 +3373,22 @@ STATIC_IFN_KUNIT int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	int i;
>  	int result = -EIO;
>  
> +	if (!i2c->oem) {
> +		struct dc_link *link = ddc_service->link;
> +		struct amdgpu_dm_connector *aconnector = link->priv;

[Severity: High]
Can ddc_service->link be NULL here?

Previously, amdgpu_dm_i2c_xfer() checked ddc_service->ddc_pin before
dereferencing ddc_service->link. Moving this block ahead of the
ddc_service->ddc_pin check without verifying that link is non-NULL causes a
NULL pointer dereference in the existing KUnit test
dm_test_i2c_xfer_no_ddc_pin():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c:dm_test_i2c_xfer_no_ddc_pin() {
	...
	i2c->ddc_service = ddc;
	i2c_set_adapdata(&i2c->base, i2c);

	/* ddc->ddc_pin is NULL -> transfer is rejected with -EIO. */
	KUNIT_EXPECT_EQ(test, amdgpu_dm_i2c_xfer(&i2c->base, NULL, 0), -EIO);
}

In that test, i2c->oem is false and ddc->link is NULL, so evaluating
link->priv crashes immediately.

> +
> +		/*
> +		 * DDC to a native DisplayPort sink is tunneled over the AUX
> +		 * channel; the physical i2c pins only reach DVI/HDMI sinks
> +		 * (e.g. DP++ with a passive adapter).  Route accordingly so
> +		 * the connector's single "ddc" bus works for either sink.
> +		 */
> +		if (aconnector && link->local_sink &&
> +		    dc_is_dp_signal(link->local_sink->sink_signal) &&

[Severity: Critical]
What protects link->local_sink against concurrent hotplug or link detection
while amdgpu_dm_i2c_xfer() reads and dereferences it?

Userspace I2C transfers on /dev/i2c-N invoke amdgpu_dm_i2c_xfer() holding
only i2c_adap->bus_lock, without acquiring aconnector->hpd_lock or
adev->dm.dc_lock and without taking a sink reference via dc_sink_retain().

Concurrently, link detection and disconnect paths release and free
link->local_sink before clearing or updating the pointer:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:link_disconnect_sink() {
	if (link->local_sink) {
		dc_sink_release(link->local_sink);
		link->local_sink = NULL;
	}
	...
}

drivers/gpu/drm/amd/display/dc/link/link_detection.c:link_disconnect_remap() {
	dc_sink_release(link->local_sink);
	link->local_sink = prev_sink;
}

Similar unserialized releases happen in amdgpu_dm_connector_poll(),
amdgpu_dm_emulated_link_detect(), and detect_link_and_local_sink() (for
example when a temporary local_sink with refcount 1 is released on EDID
read failure, same_edid remap, or MST topology discovery). Could a
concurrent I2C transfer observe a non-NULL link->local_sink and suffer a
use-after-free or NULL pointer dereference when reading
link->local_sink->sink_signal?

> +		    aconnector->dm_dp_aux.aux.ddc.algo)
> +			return i2c_transfer(&aconnector->dm_dp_aux.aux.ddc, msgs, num);
> +	}

[Severity: Medium]
Will checking link->local_sink and dc_is_dp_signal() work for DP MST links
or links with force_to_use_aux set?

When an MST topology is started in discover_dp_mst_topology(),
link_disconnect_sink() is called immediately:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:discover_dp_mst_topology() {
	...
	if (dm_helpers_dp_mst_start_top_mgr(link->ctx,
			link, (reason == DETECT_REASON_BOOT || reason == DETECT_REASON_RESUMEFROMS3S4))) {
		link_disconnect_sink(link);
	} else {
	...
}

This sets link->local_sink to NULL while downstream sinks are stored in
link->remote_sinks[], so link->local_sink is NULL on active MST links and
transfers will not be routed to the AUX adapter as described in the commit
message.

Additionally, on connectors with link->force_to_use_aux set (such as ASICs
with no_ddc_pin where ddc_service->ddc_pin is NULL), detect_dp() sets the
transaction type to DDC_TRANSACTION_TYPE_I2C_OVER_AUX (setting
link->aux_mode = true) while dp_passive_dongle_detection() sets
sink_caps->signal to HDMI or DVI:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:detect_dp() {
	...
		if (!detect_dp_sink_caps(link)) {
			if (link->force_to_use_aux) {
				sink_caps->signal = dp_passive_dongle_detection(link->ddc, sink_caps, audio_support);
	...
}

In that case dc_is_dp_signal(link->local_sink->sink_signal) is false and
the transfer falls through to the hardware I2C path (or fails with -EIO
when ddc_pin is NULL). Would checking link->aux_mode (similar to other DDC
selection paths in amdgpu_dm) handle native DP, MST, and force_to_use_aux
links without dereferencing link->local_sink?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-superm1-amdgpu-ddc-v2-1-7bde6d004382@kernel.org?part=1