Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The patch makes the deprecated and optional `samsung,lane-count` property a strict requirement, causing an unconditional probe failure if it is omitted. Pre-existing issues: - [High] Missing handling for absent DT properties causes lane count and link rate to be incorrectly clamped to 0, breaking link training. -- commit 65a66d32582ccfce55e0e84f53bc63077d25103c Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Add validation for samsung,lane-count property Add validity check for samsung,lane-count to ensure DT-provided lane count values are specification-compliant. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 8cf6b73bceac0..f23c2ae6e10d0 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c [ ... ] > @@ -1260,8 +1261,11 @@ static int analogix_dp_dt_parse_pdata(struct > analogix_dp_device *dp) > */ > of_property_read_u32(dp_node, "samsung,link-rate", > &video_info->max_link_rate); > - of_property_read_u32(dp_node, "samsung,lane-count", > - &video_info->max_lane_count); > + ret = of_property_read_u32(dp_node, "samsung,lane-count", > + &video_info->max_lane_count); > + if (ret || > !drm_dp_lane_count_is_valid(video_info->max_lane_count)) > + return dev_err_probe(dp->dev, ret ? ret : -EINVAL, > + "failed to parse > samsung,lane-count\n"); [Severity: High] Does this make the optional and deprecated samsung,lane-count property a strict requirement? If samsung,lane-count is absent from the device tree, of_property_read_u32() returns -EINVAL. This causes the condition to evaluate to true, aborting the probe with an error. According to the device tree bindings (Documentation/devicetree/bindings/display/samsung/samsung,exynos5-dp.yaml), this property is marked as deprecated and explicitly optional because the lane count can be read from the monitor. Does this patch break compatibility with device trees that rightfully omit this deprecated property? [Severity: High] This is a pre-existing issue, but there is an underlying problem in analogix_dp_full_link_train() when these optional properties are omitted: analogix_dp_full_link_train() { ... /* Setup TX lane count & rate */ if (dp->link_train.lane_count > max_lanes) dp->link_train.lane_count = max_lanes; if (dp->link_train.link_rate > max_rate) dp->link_train.link_rate = max_rate; ... } If the deprecated samsung,lane-count or samsung,link-rate properties are rightfully omitted from the device tree, their corresponding limits remain 0. The driver then executes the above clamp logic, which incorrectly clamps the valid DPCD values to 0. This completely breaks DP link training and results in a non-functional display. Should the driver ignore the limits if they are 0 (e.g., checking if max_lanes is non-zero before clamping), allowing it to correctly fall back to the monitor's DPCD capabilities? Fixing the probe failure above will just expose this runtime breakage. > break; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
