Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/dp: ts_enum initialization inflates slot count for exact integers - [Medium] drm/msm/dp: y_frac_enum rounds up to 256 causing register truncation --
commit 70d204acda89b0781eca4707fd51efbc7d294508 Author: Abhinav Kumar <[email protected]> drm/msm/dp: add support for MST channel slot allocation DP MST streams share 64 MTP slots in a time-multiplexed manner. Add support for calculating the rate governor, slot allocation, and slot reservation in the DP controller. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 929e0f94f8ddb..02732bd4efa1b 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > @@ -2701,6 +2741,117 @@ static void msm_dp_ctrl_config_msa(struct > msm_dp_ctrl_private *ctrl, [ ... ] > + ts_enum = 256 * lanes; > + ts_denom = drm_fixp_from_fraction(256 * lanes, 1); > + ts_int = drm_fixp2int(target_sc_fixp); > + > + temp = drm_fixp2int_ceil(raw_target_sc); > + if (temp != ts_int) { > + temp = drm_fixp_from_fraction(ts_int, 1); > + temp1 = raw_target_sc - temp; > + temp2 = drm_fixp_mul(temp1, ts_denom); > + ts_enum = drm_fixp2int(temp2); > + } > + > + /* target_strm_sym */ > + ts_int_fixp = drm_fixp_from_fraction(ts_int, 1); > + ts_frac_fixp = drm_fixp_from_fraction(ts_enum, drm_fixp2int(ts_denom)); > + temp = ts_int_fixp + ts_frac_fixp; [Severity: Medium] Does this initialization inflate the target slot count when raw_target_sc evaluates to an exact integer? Since ts_enum is initialized to 256 * lanes, it means the default fraction is 1.0. When raw_target_sc is an exact integer, the condition temp != ts_int evaluates to false, causing the recalculation of ts_enum to be skipped. This leads to ts_frac_fixp incorrectly evaluating to 1.0 (instead of 0.0), which mistakenly adds an extra symbol per MTP to target_strm_sym and artificially inflates the required slot count. > + temp1 = drm_fixp_from_fraction(lanes, 1); > + target_strm_sym = drm_fixp_mul(temp, temp1); > + > + /* x_int */ > + x_int = drm_fixp2int(target_strm_sym); > + > + /* y_enum_frac */ > + temp = drm_fixp_from_fraction(x_int, 1); > + temp1 = target_strm_sym - temp; > + temp2 = drm_fixp_from_fraction(256, 1); > + y_frac_enum_fixp = drm_fixp_mul(temp1, temp2); > + > + temp1 = drm_fixp2int(y_frac_enum_fixp); > + temp2 = drm_fixp2int_ceil(y_frac_enum_fixp); > + > + y_frac_enum = (u32)((temp1 == temp2) ? temp1 : temp1 + 1); [Severity: Medium] Can this rounding logic overflow the fraction register and truncate it? If the fractional part of target_strm_sym is very close to 1.0 (e.g., 0.999), the ceiling calculation yields 256. When packed into the hardware register via rg = y_frac_enum | (x_int << 16) in msm_dp_ctrl_update_rg(), the value 256 is represented as 0x100. If the DP controller's y_frac_enum field only accepts an 8-bit fraction, writing 0x100 could overflow the field and truncate it to 0 without carrying the increment over to x_int, resulting in a subtle bandwidth deficit and potential display underflows. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
