Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/msm/dp: bitwise OR of STATUS5 and STATUS7 without masking in ISR --
commit 987123505b6cfcc4664c72feae6b070ad91b79a8 Author: Abhinav Kumar <[email protected]> drm/msm/dp: add support for sending VCPF packets in DP controller This commit adds support for sending VC Payload Fill (VCPF) sequences in the MSM DP controller when stream symbols are absent. It introduces interrupt handling for streams 0-3 using the STATUS5 and STATUS7 registers to signal VCPF completion. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 9a0c9737e6d00..929e0f94f8ddb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > @@ -379,6 +398,34 @@ static void msm_dp_ctrl_config_psr_interrupt(struct > msm_dp_ctrl_private *ctrl) > msm_dp_write_ahb(ctrl, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); > } > > +static u32 msm_dp_ctrl_get_mst_vcpf_interrupt_0_1(struct msm_dp_ctrl_private > *ctrl) > +{ > + u32 intr, intr_ack; > + > + intr = msm_dp_read_ahb(ctrl, REG_DP_INTR_STATUS5); > + intr &= ~DP_INTERRUPT_STATUS5_MASK; > + intr_ack = (intr & DP_INTERRUPT_STATUS5) > + << DP_INTERRUPT_STATUS_ACK_SHIFT; > + msm_dp_write_ahb(ctrl, REG_DP_INTR_STATUS5, > + intr_ack | DP_INTERRUPT_STATUS5_MASK); > + > + return intr; > +} > + > +static u32 msm_dp_ctrl_get_mst_vcpf_interrupt_2_3(struct msm_dp_ctrl_private > *ctrl) > +{ > + u32 intr, intr_ack; > + > + intr = msm_dp_read_ahb(ctrl, REG_DP_INTR_STATUS7); > + intr &= ~DP_INTERRUPT_STATUS7_MASK; > + intr_ack = (intr & DP_INTERRUPT_STATUS7) > + << DP_INTERRUPT_STATUS_ACK_SHIFT; > + msm_dp_write_ahb(ctrl, REG_DP_INTR_STATUS7, > + intr_ack | DP_INTERRUPT_STATUS7_MASK); > + > + return intr; > +} [ ... ] > @@ -2801,6 +2877,16 @@ irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl > *msm_dp_ctrl, > ret = IRQ_HANDLED; > } > > + isr = msm_dp_ctrl_get_mst_vcpf_interrupt_0_1(ctrl); > + if (ctrl->num_pixel_clks > DP_STREAM_2) > + isr |= msm_dp_ctrl_get_mst_vcpf_interrupt_2_3(ctrl); > + > + if (isr & (DP_INTR_MST_DP0_VCPF_SENT | DP_INTR_MST_DP1_VCPF_SENT | > + DP_INTR_MST_DP2_VCPF_SENT | DP_INTR_MST_DP3_VCPF_SENT)) { [Severity: Medium] Does this bitwise OR of two different status registers cause false positives? The msm_dp_ctrl_get_mst_vcpf_interrupt_*() functions return the unmasked register values. Since the values from REG_DP_INTR_STATUS5 and REG_DP_INTR_STATUS7 are ORed together here, bits from one register could overlap with the VCPF bits checked for the other. For example, if an unrelated interrupt sets bit 18 in REG_DP_INTR_STATUS5, and it gets ORed into `isr`, the condition will see bit 18 set and assume it's DP_INTR_MST_DP2_VCPF_SENT (which is bit 18 of REG_DP_INTR_STATUS7). This could cause a spurious completion of ctrl->idle_comp during a VCPF push, allowing the driver to proceed while the hardware is still executing the pattern. Should the return values of msm_dp_ctrl_get_mst_vcpf_interrupt_*() be masked against their respective VCPF bits before returning, or should they be checked independently instead of ORing them together? > + complete(&ctrl->idle_comp); > + ret = IRQ_HANDLED; > + } > + > /* DP aux isr */ > isr = msm_dp_ctrl_get_aux_interrupt(ctrl); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
