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