Re: [PATCH v7 04/18] drm/bridge: synopsys: dw-dp: Add missing reinit_completion

[sashiko-bot@kernel.org]

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

New issues:
- [High] The addition of `reinit_completion()` introduces a data race on `complete.done` and incorrectly mitigates spurious IRQs, causing a TOCTOU window due to threaded IRQ scheduling delays.

Pre-existing issues:
- [High] Lifecycle ordering bug: the AUX I2C interface is exposed to userspace before hardware is initialized, and unregistered after hardware is torn down.
- [High] A divide-by-zero vulnerability exists in dw_dp_video_enable when processing a display mode with a clock value of 1.
- [High] I2C-over-AUX reply status (NACK/DEFER) is silently discarded, causing deferred I2C transactions to falsely succeed and return garbage data.
- [High] dw_dp_aux_transfer improperly rejects short AUX reads by returning -EBUSY.
- [High] dw_dp_aux_transfer accesses the AUX hardware without ensuring the PHY is powered on.
--

--- Patch [4]: [PATCH v7 04/18] drm/bridge: synopsys: dw-dp: Add missing reinit_completion ---
commit e066df83b1559151eb45281e1a32fbe600503fc1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add missing reinit_completion

The DP AUX transfer method uses a completion triggered by an interrupt,
which can timeout. If the function runs into the timeout and the
interrupt fires afterwards, the following DP aux transfer completion
would trigger immediately without waiting for the interrupt. This in
turn means the next one would also be broken and so on.

Fix this potential issue by re-initializing the completion before
sending the AUX command.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index ca7ff544d243a..951f1e280e478 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1465,6 +1465,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	reinit_completion(&dp->complete);
> +

[Severity: High]
Does this lockless modification of the completion state race with the
threaded interrupt handler?

If an AUX transfer times out, an AUX_REPLY_EVENT interrupt follows, and
Linux schedules the threaded IRQ handler. If a new AUX transfer is initiated
concurrently, it will call reinit_completion() and start the hardware.

The delayed IRQ handler might then execute:

dw_dp_irq() {
    ...
    complete(&dp->complete);
}

Could this immediately satisfy the wait for the new transfer before the
hardware finishes, causing the new transfer to proceed with a stale or
intermediate status? Should we drain or synchronize with the IRQ thread
before issuing a new hardware command?

[Severity: High]
This is a pre-existing issue, but does dw_dp_aux_transfer() access the AUX
hardware without ensuring the PHY is powered on?

Later in dw_dp_aux_transfer():

dw_dp_aux_transfer() {
    ...
    regmap_write(dp->regmap, DW_DP_AUX_CMD, value);
    status = wait_for_completion_timeout(&dp->complete, timeout);
}

If userspace tools query an inactive monitor while the PHY is powered off,
could these hardware register writes cause timeouts or bus hangs? Should
there be a phy_power_on() block surrounding this transfer?

[Severity: High]
This is a pre-existing issue, but are we silently discarding the
I2C-over-AUX reply status?

Also in dw_dp_aux_transfer():

dw_dp_aux_transfer() {
    ...
    msg->reply = FIELD_GET(AUX_STATUS, value);
}

Because AUX_STATUS masks bits 7..4 and shifts them down, the resulting
value lacks the original bits 4 and 5. The DRM core mask
DP_AUX_I2C_REPLY_MASK expects these bits in their original positions.

Since the value will be at most 0xF, applying the 0x30 mask will extract 0
(DP_AUX_I2C_REPLY_ACK). Does this cause deferred I2C transactions or NACKs
to falsely succeed and return garbage data?

[Severity: High]
This is a pre-existing issue, but does dw_dp_aux_transfer() improperly
reject short reads?

dw_dp_aux_transfer() {
    ...
    if (count != msg->size)
        return -EBUSY;
}

If a DP sink returns fewer bytes than requested, the DRM DP helper expects
the actual number of bytes transferred so it can adjust the offset. By
returning -EBUSY, drm_dp_i2c_do_msg() will retry the exact same sized
transfer instead. Could this cause a complete failure to read EDID data
from sinks that restrict reply lengths?

[Severity: High]
This is a pre-existing issue, but does the driver expose the AUX I2C
interface to userspace before the hardware is fully initialized?

Looking at dw_dp_bind(), it registers the interface:

dw_dp_bind() {
    ...
    ret = drm_dp_aux_register(&dp->aux);
}

If userspace opens the device node and issues an ioctl before phy_init() is
called, could this lead to bus hangs or synchronous aborts when accessing
uninitialized DP registers?

Similarly, dw_dp_unbind() unregisters the AUX interface after the hardware
is torn down:

dw_dp_unbind() {
    ...
    phy_exit(dp->phy);
    drm_dp_aux_unregister(&dp->aux);
}

[Severity: High]
This is a pre-existing issue, but is there a potential divide-by-zero here?

In dw_dp_video_enable(), when configuring the threshold:

dw_dp_video_enable() {
    ...
    if (color_format == DRM_OUTPUT_COLOR_FORMAT_YCBCR420)
        t2 = (link->rate / 4) * 1000 / (mode->clock / 2);
}

If a local user configures a custom DRM mode with a clock value of 1,
(mode->clock / 2) evaluates to 0. Since drm_mode_convert_umode() does not
reject clock values of 1, could this trigger a kernel panic?

>  	switch (msg->request & ~DP_AUX_I2C_MOT) {
>  	case DP_AUX_NATIVE_WRITE:
>  	case DP_AUX_I2C_WRITE:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=4