Re: [PATCH v5 5/6] 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:
- [High] Valid extcon devices are incorrectly skipped during probe due to a flawed pointer error check, breaking all existing platforms that use extcon.
- [High] Hotplug detect (HPD) is completely broken when extcon is not present because no alternative notification mechanism is implemented.
- [High] Unchecked negative error code from `phy_get_bus_width` is implicitly truncated to `u8` and used as a valid lane count, corrupting hardware configuration.
--

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

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

This commit adds support for getting PHY lane information directly from the
PHY instead of relying on extcon when a Type-C controller is present. It
preserves extcon support for cases where the Type-C controller is absent.

> 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
> @@ -157,6 +157,9 @@ static int cdn_dp_get_port_lanes(struct cdn_dp_port *port)
>  	int dptx;
>  	u8 lanes;
>  
> +	if (!edev)
> +		return phy_get_bus_width(port->phy);
> +

[Severity: High]
Can this return a negative error code (like -ENOSYS) that gets truncated?

In cdn_dp_enable_phy(), the return value is directly assigned to an 8-bit
unsigned integer:

	port->lanes = cdn_dp_get_port_lanes(port);

If phy_get_bus_width() returns a negative error, will this result in a
silently truncated out-of-bounds lane count being written to the hardware
registers?

[ ... ]

> @@ -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 permanently orphan the driver's
hotplug detect state machine?

The driver's connection state machine is entirely driven by
cdn_dp_pd_event_work(), which is only ever scheduled by the extcon notifier
callback. 

Without an alternative notification mechanism for non-extcon setups, will the
display controller ever be notified of cable insertion?

[ ... ]

> @@ -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]
Will this condition evaluate to true for all valid extcon pointers,
skipping valid extcon devices entirely?

When a valid extcon device is found, PTR_ERR() will cast that pointer
address to a long. Since a valid pointer address will not equal -ENODEV,
this check seems to evaluate to true and skip the port setup for all existing
functional extcon setups.

Should this include an IS_ERR() guard, such as
IS_ERR(extcon) && PTR_ERR(extcon) != -ENODEV?

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