Re: [PATCH 3/3] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Division by zero kernel panic if a display mode has zero horizontal blanking.
- [High] Integer underflow leading to hardware register corruption when horizontal blanking is too small.
- [Low] Build failure on 32-bit architectures due to 64-bit division.
--

--- Patch [3]: [PATCH 3/3] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing ---
commit eb9d34f1c88e6933d5822b8331847848414cfe4d
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing

The NWL MIPI Host controller registers specifies the horizontal front
porch, sync pulse, and back porch in DSI packet payload size in bytes (ie.
not in pixel clocks).

The formula implemented here converts the hfp, hsa, and hbp to DSI packet
bytes and then subtracts the number of packet header bytes.

> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index c9af6a3d47e1d..ed61580a35457 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -317,9 +322,54 @@ static int nwl_dsi_config_dpi(struct nwl_dsi *dsi)
>  			      dsi->mode.hdisplay);
>  	}
>  
> -	nwl_dsi_write(dsi, NWL_DSI_HFP, hfront_porch);
> -	nwl_dsi_write(dsi, NWL_DSI_HBP, hback_porch);
> -	nwl_dsi_write(dsi, NWL_DSI_HSA, hsync_len);
> +	pclk_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, dsi->mode.clock * 1000);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "pclk_period: %llu\n", pclk_period);
> +
> +	hs_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, dsi->phy_cfg.mipi_dphy.hs_clk_rate);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hs_period: %llu\n", hs_period);
> +
> +	/*
> +	 * Calculate the bytes needed, according to the RM formula:
> +	 * Time of DPI event = time to transmit x number of bytes on the DSI
> +	 * interface
> +	 * dpi_event_size * dpi_pclk_period = dsi_bytes * 8 * hs_bit_period /
> +	 * num_lanes
> +	 * ===>
> +	 * dsi_bytes = dpi_event_size * dpi_pclk_period * num_lanes /
> +	 * (8 * hs_bit_period)
> +	 */
> +	hfp = hfront_porch * pclk_period * dsi->lanes / (8 * hs_period);
> +	hbp = hback_porch * pclk_period * dsi->lanes / (8 * hs_period);
> +	hsa = hsync_len * pclk_period * dsi->lanes / (8 * hs_period);

[Severity: Low]
Will this cause a build failure on 32-bit architectures?

The variables pclk_period and hs_period are 64-bit unsigned integers
(unsigned long long), and using the native division operator here instead of
kernel macros like div64_u64() can result in an undefined reference to
'__udivdi3' when compiling on 32-bit systems.

> +
> +	/* Make sure horizontal blankins are even numbers */
> +	hfp = roundup(hfp, 2);
> +	hbp = roundup(hbp, 2);
> +	hsa = roundup(hsa, 2);
> +
> +	/*
> +	 * We need to subtract the packet header length: 32
> +	 * In order to make sure we don't get negative values,
> +	 * subtract a proportional value to the total length of the
> +	 * horizontal blanking duration.
> +	 */
> +	h_blank = hfp + hbp + hsa;
> +
> +	pkt_len = roundup(((hfp * 100 / h_blank) * 32) / 100, 2);

[Severity: Critical]
Could h_blank be zero here?

If userspace configures a display mode where hdisplay == hsync_start ==
hsync_end == htotal (meaning zero horizontal blanking), the porches and
sync length will be zero. This makes hfp, hbp, and hsa zero, causing h_blank
to evaluate to 0.

Since the mode parameters are not restricted from being zero during validation,
could a custom mode submitted via DRM atomic commits trigger a division by
zero kernel panic here?

> +	pkt_hdr_len = pkt_len;
> +	hfp -= pkt_len;
> +
> +	pkt_len = roundup(((hbp * 100 / h_blank) * 32) / 100, 2);
> +	pkt_hdr_len += pkt_len;
> +	hbp -= pkt_len;
> +
> +	hsa -= (32 - pkt_hdr_len);
> +
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "Register values: hfp=%d hbp=%d hsa=%d\n", hfp, hbp, hsa);
> +
> +	nwl_dsi_write(dsi, NWL_DSI_HFP, hfp);
> +	nwl_dsi_write(dsi, NWL_DSI_HBP, hbp);
> +	nwl_dsi_write(dsi, NWL_DSI_HSA, hsa);

[Severity: High]
Can these subtractions cause integer underflow if the horizontal blanking is
too small?

If userspace configures a mode with a valid but very small horizontal blanking
(where the calculated h_blank is less than the equivalent of 32 bytes),
subtracting up to 32 bytes proportionally could cause hfp, hbp, or hsa to
underflow and become negative.

Would those underflowed negative values then be implicitly cast to large u32
integers when written directly to the DSI hardware registers here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-nwl-dsi-fixes-v1-0-927d918a86ad@geanix.com?part=3