Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] Incorrect error checking logic in `cdn_dp_probe` skips port initialization for valid extcon devices, breaking DisplayPort for legacy boards.
- [High] Removing the `extcon` notifier without providing an alternative mechanism completely breaks hotplug detection for Type-C connections without an extcon device.
--

--- Patch [4]: [PATCH v3 4/5] drm/rockchip: cdn-dp: Support handle lane info without extcon ---
commit 2789d7ccb0bb45f82d439538cf9aeaac1da9c5e9
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.
There is no extcon needed if the Type-C controller is present. The extcon
device should still be 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 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 leave the driver
without any hotplug detection mechanism?

The driver relies on the cdn_dp_pd_event notifier (registered via extcon) to
schedule dp->event_work, which probes the hardware, updates the connection
state, and calls drm_bridge_hpd_notify().

Without an alternative HPD mechanism when extcon is absent, won't hotplug
events be silently ignored, causing displays to only be detected if plugged in
during boot or resume?

>  		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: High]
Does evaluating PTR_ERR(extcon) != -ENODEV without first checking if extcon
is an error pointer break initialization for valid extcon devices?

When a valid extcon device is successfully found, extcon is a valid kernel
pointer. Casting this pointer to long via PTR_ERR() yields a memory address
that is not equal to -ENODEV.

Since the condition incorrectly evaluates to true, won't the loop continue
and skip port allocation for all valid extcon devices, breaking DisplayPort
on existing platforms that use extcon?

>  
>  		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;
>  		port->dp = dp;
>  		port->id = i;

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

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