Re: [PATCH v7 12/35] drm/msm/dp: add support for sending VCPF packets in DP controller

[sashiko-bot@kernel.org]

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 <quic_abhinavk@quicinc.com>

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/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=12