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 <[email protected]> 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/[email protected]?part=5
