Re: [PATCH v6 6/7] drm/rockchip: cdn-dp: Support handle lane info without extcon

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Logic error in pointer error checking breaks probe for all existing boards using extcon.
- [High] Missing hotplug detection (HPD) mechanism when extcon is absent.
--

--- Patch [6]: [PATCH v6 6/7] drm/rockchip: cdn-dp: Support handle lane info without extcon ---
commit 2ec5f110f1557ebaec82cbe6b39d4e2ab7f4a88f
Author: Chaoyi Chen <[email protected]>

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

This patch add support for get PHY lane info without help of extcon.

There is no extcon needed if the Type-C controller is present. In this
case, the lane info can be get from PHY instead of extcon.

The extcon device should still be supported if 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 190cce9530c82..74e1f5422a6e2 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c

[ ... ]

> @@ -1027,6 +1033,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;
> +

[Severity: High]
Does skipping the notifier registration when extcon is absent break hotplug 
detection? 

The driver relies on cdn_dp_pd_event_work() to transition dp->connected to 
true, load firmware, and train the link. Without the extcon notifier, 
event_work is never scheduled on hotplug. 

Since cdn_dp_bridge_detect() simply returns the static dp->connected state:

drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_bridge_detect() {
    mutex_lock(&dp->lock);
    if (dp->connected)
        status = connector_status_connected;
    mutex_unlock(&dp->lock);
    ...
}

Could KMS polling always see the port as disconnected when hardware is 
configured without an extcon device?

>  		port->event_nb.notifier_call = cdn_dp_pd_event;
>  		ret = devm_extcon_register_notifier(dp->dev, port->extcon,
>  						    EXTCON_DISP_DP,

[ ... ]

> @@ -1119,14 +1128,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)
>  			continue;
>  

[Severity: Critical]
Will this condition accidentally skip port initialization for all valid extcon
pointers?

If extcon is a valid pointer, PTR_ERR(extcon) evaluates to its memory address,
which will never equal -ENODEV. This causes the condition to evaluate to true,
triggering the continue statement and bypassing port setup completely for any
hardware that has an extcon device.

>  		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>  		if (!port)
>  			return -ENOMEM;
>  
> -		port->extcon = extcon;
> +		port->extcon = IS_ERR(extcon) ? NULL : extcon;
>  		port->phy = phy;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy