Re: [PATCH v4 4/5] drm/rockchip: cdn-dp: Support handle lane info without extcon

[sashiko-bot@kernel.org]

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

New issues:
- [High] The error-handling condition `PTR_ERR(extcon) != -ENODEV` incorrectly evaluates to true for all valid extcon pointers, completely skipping port initialization for platforms with an extcon.

Pre-existing issues:
- [High] The driver incorrectly uses `devm_` APIs inside a component `bind` function, leading to lingering notifiers and use-after-free upon component unbind.
- [High] `cdn_dp_pd_event_work` drops and reacquires a lock during execution but fails to re-evaluate the hardware's suspend state, potentially accessing powered-down hardware.
--

--- Patch [4]: [PATCH v4 4/5] drm/rockchip: cdn-dp: Support handle lane info without extcon ---
commit 7d9954b4a176d603a3c1df5fb3c1b4c9ddaa5b21
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

drm/rockchip: cdn-dp: Support handle lane info without extcon

This patch adds support for getting PHY lane info without the help of extcon.
If a Type-C controller is present, the lane info can be obtained from the PHY
instead of extcon. The extcon device is still supported if the Type-C
controller is not present.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 68aa66915e8ac..1c1c1116b5ebe 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c

[ ... ]

> @@ -1030,6 +1036,9 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  	for (i = 0; i < dp->ports; i++) {
>  		port = dp->port[i];
>  
> +		if (!port->extcon)
> +			continue;
> +
>  		port->event_nb.notifier_call = cdn_dp_pd_event;
>  		ret = devm_extcon_register_notifier(dp->dev, port->extcon,
>  						    EXTCON_DISP_DP,
>  						    &port->event_nb);

[Severity: High]
This isn't a bug introduced by this patch, but does using devm_ APIs in a
component bind function lead to lingering notifiers?

When the DRM master is unbound, cdn_dp_unbind() is called. However, resources
allocated with devm_ (like this extcon notifier and the DRM bridge) are tied
to the platform device's lifetime, not the component.

If a user unbinds the DRM master device via sysfs, and then plugs or unplugs
the DisplayPort cable, could the active notifier fire and schedule the driver's
workqueue on destroyed state, causing a use-after-free?

[ ... ]

> @@ -1122,14 +1131,14 @@ static int cdn_dp_probe(struct platform_device *pdev)
>  		    PTR_ERR(phy) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
>  
> -		if (IS_ERR(extcon) || IS_ERR(phy))
> +		if (IS_ERR(phy) || PTR_ERR(extcon) != -ENODEV)

[Severity: High]
Does this error-handling condition evaluate to true for all valid extcon
pointers?

When the driver probes on a platform with an extcon,
extcon_get_edev_by_phandle() returns a valid pointer. This condition then
checks if the valid pointer's address is not equal to -ENODEV, which is always
true.

Could this skip port initialization for all valid extcon devices, leaving
dp->ports = 0 and causing the driver to unconditionally fail with -EINVAL?

>  			continue;
>  
>  		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
workqueue and system suspend?

In cdn_dp_pd_event_work(), dp->lock is held and dp->suspended is verified.
Then, cdn_dp_request_firmware() is called:

drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_request_firmware() {
    ...
    /* Drop the lock before getting the firmware to avoid blocking boot */
    mutex_unlock(&dp->lock);
    ...
}

During this window, if the system suspends, cdn_dp_suspend() acquires
dp->lock, sets dp->suspended = true, and powers off the hardware. When
cdn_dp_request_firmware() finishes, it reacquires the lock and returns to
cdn_dp_pd_event_work().

Execution then resumes without rechecking if the system suspended in the
meantime:

drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_pd_event_work() {
    ...
    ret = cdn_dp_request_firmware(dp);
    if (ret)
        goto out;

    dp->connected = true;
    
    if (!cdn_dp_connected_port(dp)) {
        ...
    } else if (!dp->active) {
        ...
        ret = cdn_dp_enable(dp);
    ...
}

Could this cause the worker thread to call cdn_dp_enable() and access hardware
registers that have been powered down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727064408.101-1-kernel@airkyi.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy