Re: [PATCH 3/3] drm/bridge: add TH1520 DesignWare MIPI DSI host controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/bridge: missing GENERIC_PHY_MIPI_DPHY and MFD_SYSCON select in Kconfig
- [Medium] drm/bridge: uninitialized ret in th1520_dw_mipi_dsi_probe() error path
- [Medium] drm/bridge: 32-bit signed overflow in calculate_lane_mbps()
--

commit 1daf55a66c6c757f635f5bb068b6e103e284e178
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm/bridge: add TH1520 DesignWare MIPI DSI host controller

This commit adds a glue driver for the Synopsys DesignWare MIPI DSI host
controller and internal D-PHY on the T-Head TH1520 SoC.

> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 4a57d49b4c6d3..b005623ba26bf 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -375,6 +375,16 @@ config DRM_THEAD_TH1520_DW_HDMI
>  	  Choose this to enable support for the internal HDMI bridge found
>  	  on the T-Head TH1520 SoC.
>  
> +config DRM_THEAD_TH1520_DW_MIPI_DSI
> +	tristate "T-Head TH1520 DesignWare MIPI DSI bridge"
> +	depends on OF
> +	depends on COMMON_CLK
> +	depends on ARCH_THEAD || COMPILE_TEST
> +	select DRM_DW_MIPI_DSI
> +	help
> +	  Choose this to enable support for the internal MIPI DSI bridge
> +	  found on the T-Head TH1520 SoC.

[Severity: Medium]
Should DRM_THEAD_TH1520_DW_MIPI_DSI in drivers/gpu/drm/bridge/Kconfig also
select GENERIC_PHY_MIPI_DPHY and MFD_SYSCON?

In drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c, calculate_lane_mbps() calls
phy_mipi_dphy_get_default_config(), which has no inline stub fallback when
CONFIG_GENERIC_PHY_MIPI_DPHY is disabled. Because GENERIC_PHY_MIPI_DPHY is a
promptless symbol and DRM_DW_MIPI_DSI does not select it, building with
CONFIG_DRM_THEAD_TH1520_DW_MIPI_DSI enabled will fail at link time if no
other enabled driver selects GENERIC_PHY_MIPI_DPHY.

Similarly, th1520_dw_mipi_dsi_probe() calls syscon_regmap_lookup_by_phandle()
and dsi_phy_init() calls regmap_update_bits(). When CONFIG_MFD_SYSCON is not
enabled, syscon_regmap_lookup_by_phandle() falls back to a stub returning
ERR_PTR(-ENOTSUPP), causing probe to unconditionally fail.

> diff --git a/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c b/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c
> new file mode 100644
> index 0000000000000..e5c7c5d6dcc71
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c

[ ... ]

> +static unsigned int
> +calculate_lane_mbps(const struct drm_display_mode *mode,
> +		    unsigned long mode_flags,
> +		    u32 lanes, u32 format)
> +{
> +	int bpp = mipi_dsi_pixel_format_to_bpp(format);
> +	union phy_configure_opts phy_opts;
> +
> +	/*
> +	 * FIXME: the extra bandwidth headroom logic mimics what the Rockchip
> +	 * driver does, and test shows that it's necessary for the hardware
> +	 * to work with the mainline dw-mipi-dsi common code.
> +	 */
> +	phy_mipi_dphy_get_default_config(mode->clock * 1000 * 10 / 8,
> +					 bpp, lanes,
> +					 &phy_opts.mipi_dphy);

[Severity: Medium]
Can the pixel clock multiplication in calculate_lane_mbps() overflow a 32-bit
signed int?

Since mode->clock in struct drm_display_mode is an int (in kHz) and 1000, 10,
and 8 are signed int literals, mode->clock * 1000 * 10 is evaluated in 32-bit
signed arithmetic before being converted to the unsigned long parameter of
phy_mipi_dphy_get_default_config().

When mode->clock is 214749 kHz or higher, mode->clock * 1000 * 10 exceeds
INT_MAX. With 4 lanes and up to 2500 Mbps per lane in hsfreqrange_table, the
hardware supports 24-bpp modes up to 333333 kHz, which will wrap to a
negative value, sign-extend to a large unsigned long, and be rejected by
dsi_mode_valid() and dsi_get_lane_mbps(). Higher out-of-range pixel clocks
(such as 594000 kHz) can also wrap around to a small positive value and
falsely pass dsi_mode_valid().

Would using 1000UL (or mul_u32_u32()) avoid this overflow?

> +
> +	return DIV_ROUND_UP(phy_opts.mipi_dphy.hs_clk_rate, USEC_PER_SEC);
> +}

[ ... ]

> +static int th1520_dw_mipi_dsi_probe(struct platform_device *pdev)
> +{
> +	const struct dsi_chip_data *cdata;
> +	struct device *dev = &pdev->dev;
> +	struct th1520_dw_mipi_dsi *dsi;
> +	struct resource *res;
> +	void __iomem *base;
> +	int i, ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +
> +	dsi->syscon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> +	if (IS_ERR(dsi->syscon)) {
> +		ret = PTR_ERR(dsi->syscon);
> +		dev_err_probe(dev, ret, "Unable to get system controller regmap\n");
> +		return ret;
> +	}
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base)) {
> +		dev_err_probe(dev, ret, "Unable to get DSI registers\n");
> +		return PTR_ERR(base);\n> +	}

[Severity: Medium]
In th1520_dw_mipi_dsi_probe(), if syscon_regmap_lookup_by_phandle() succeeds
and devm_platform_get_and_ioremap_resource() fails, isn't ret uninitialized
when passed to dev_err_probe()?

Should this pass PTR_ERR(base) (or return dev_err_probe(dev, PTR_ERR(base),
...)) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-th1520-dw-mipi-dsi-v1-0-753f34fd304f@iscas.ac.cn?part=3