Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm: drm_dp_lttpr_count() negative error code evaluated as true - [Low] drm/display: Missing <drm/display/drm_dp_tunnel.h> include in drm_dp_helper.c - [Low] drm/display: API doc mismatch in drm_dp_read_edp_sink_rates() --
commit fd61f2a21aaa6e7f861570be192ba592894c3a4d Author: Kory Maincent <[email protected]> drm: Move sink link rates reading helpers out of i915 The commit moves sink link rates reading helpers from i915 into generic DRM helpers for reuse. It leaves Intel-specific quirks in the driver. > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c > b/drivers/gpu/drm/display/drm_dp_helper.c > index e623ccb4c1d8a..8ef096a0138e4 100644 > --- a/drivers/gpu/drm/display/drm_dp_helper.c > +++ b/drivers/gpu/drm/display/drm_dp_helper.c [ ... ] > @@ -4912,3 +4912,240 @@ int drm_dp_max_dprx_data_rate(int max_link_rate, int > max_lanes) [ ... ] > +#if IS_ENABLED(CONFIG_DRM_DISPLAY_DP_TUNNEL) > + > +static u8 drm_dprx_128b132b_link_rates(const struct drm_dp_tunnel *tunnel, > + u8 uhbr_rates) > +{ > + if (!drm_dp_tunnel_bw_alloc_is_enabled(tunnel)) [Severity: Low] Does this code build correctly when CONFIG_DRM_DISPLAY_DP_TUNNEL is enabled? The patch adds calls to drm_dp_tunnel_bw_alloc_is_enabled(), drm_dp_tunnel_128b132b_dprx_rates(), and drm_dp_tunnel_128b132b_supported() in drm_dp_helper.c. These are defined in <drm/display/drm_dp_tunnel.h>. However, neither drm_dp_helper.c nor its included headers appear to include this header, which could result in implicit function declaration errors. > + return uhbr_rates; > + > + static_assert(DP_TUNNELING_10GBPS_PER_LANE_SUPPORT == DP_UHBR10 && > + DP_TUNNELING_13_5GBPS_PER_LANE_SUPPORT == DP_UHBR13_5 && > + DP_TUNNELING_20GBPS_PER_LANE_SUPPORT == DP_UHBR20); > + > + return drm_dp_tunnel_128b132b_dprx_rates(tunnel); > +} [ ... ] > +static int > +drm_dp_get_128b132b_link_rates(const u8 > lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE], > + const struct drm_dp_tunnel *tunnel, > + u8 uhbr_rates, > + int link_rates[DP_MAX_128B132B_SUPPORTED_RATES]) > +{ > + int i = 0; > + > + if (tunnel) > + uhbr_rates = drm_dprx_128b132b_link_rates(tunnel, uhbr_rates); > + > + if (drm_dp_lttpr_count(lttpr_common_caps)) { [Severity: Medium] Will this evaluate negative error codes as a positive repeater count? drm_dp_lttpr_count() can return negative error codes (like -EINVAL or -ERANGE) for invalid DPCD LTTPR counts. This condition evaluates those negative errors as true, falsely assuming a repeater is present. If the DPCD revision lttpr_common_caps[0] is < 0x20, the else branch executes and clears uhbr_rates = 0, wrongly discarding the sink's supported UHBR rates. > + /* We have a repeater */ > + if (lttpr_common_caps[0] >= 0x20 && > + dp_lttpr_common_cap(lttpr_common_caps, > + > DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER) & > + DP_PHY_REPEATER_128B132B_SUPPORTED) { [ ... ] > +int drm_dp_read_dpcd_sink_rates(struct drm_dp_aux *aux, > + const u8 dpcd[DP_RECEIVER_CAP_SIZE], > + const u8 > lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE], > + const struct drm_dp_desc *desc, > + const struct drm_dp_tunnel *tunnel, > + int sink_rates[DP_MAX_SUPPORTED_RATES]) > +{ [ ... ] > + /* > + * Sink rates for 128b/132b. If set, sink should support all 8b/10b > + * rates and 10 Gbps. > + */ > + if (drm_dprx_supports_128b132b(tunnel, dpcd)) > + i += drm_dp_get_128b132b_link_rates(lttpr_common_caps, > + tunnel, > + uhbr_rates, > + &sink_rates[i]); [Severity: Medium] Could this result in a NULL pointer dereference inside drm_dp_get_128b132b_link_rates()? Earlier in drm_dp_read_dpcd_sink_rates(), lttpr_common_caps is allowed to be NULL: if (lttpr_common_caps) max_lttpr_rate = drm_dp_lttpr_max_link_rate(lttpr_common_caps); However, it is passed blindly to drm_dp_get_128b132b_link_rates() here. If a caller passes a NULL lttpr_common_caps, drm_dp_lttpr_count() will attempt to access it, triggering a NULL pointer dereference. > + > + return i; > +} > +EXPORT_SYMBOL_GPL(drm_dp_read_dpcd_sink_rates); > + > +/** > + * drm_dp_read_edp_sink_rates() - read the sink rates from the eDP DPCD [ ... ] > +int drm_dp_read_edp_sink_rates(struct drm_dp_aux *aux, u8 edp_dpcd_rev, > + int sink_rates[DP_MAX_SUPPORTED_RATES]) > +{ > + int num_sink_rates; > + > + if (edp_dpcd_rev < DP_EDP_14) > + return 0; > + > + num_sink_rates = drm_dp_read_dpcd_supported_link_rates(aux, sink_rates); > + if (num_sink_rates < 0) { > + drm_dbg_kms(aux->drm_dev, > + "%s: Unable to read eDP supported link rates > (%d)\n", > + aux->name, num_sink_rates); > + return 0; [Severity: Low] Does this implementation match its API documentation? The kernel-doc for drm_dp_read_edp_sink_rates() explicitly states: "Returns: The number of supported sink rates, 0 if the sink does not support the DP_SUPPORTED_LINK_RATES register, negative error code otherwise." However, this implementation traps the negative error code internally and explicitly returns 0 on failure. This creates a confusing API contract for drivers using this newly exported helper. > + } > + > + return num_sink_rates; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
