Re: [PATCH v1 2/4] drm/ms91xx: Add register access and connector support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] drm/ms91xx: mismatched interlace handling in ms91xx_decode_custom_timing()
- [Medium] drm/ms91xx: unchecked block index and u16 overflow in ms91xx_read_edid()
- [Medium] drm/ms91xx: odd hactive in ms91xx_decode_custom_timing() leaks uninit memory
- [Low] drm/ms91xx: multiple modes marked DRM_MODE_TYPE_PREFERRED on connector
--

commit 557c9cb985634091a4b0753b2e8063cde49e88a5
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/ms91xx: Add register access and connector support

This commit adds USB control message helpers for reading registers and flash
memory, display power and mode programming sequences, custom flash timing
decoding, and DRM connector and EDID probing for the ms91xx driver.

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_connector.c b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
> new file mode 100644
> index 0000000000000..d04f818fa09f2
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
> @@ -0,0 +1,263 @@

[ ... ]

> +static int ms91xx_read_edid(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct ms91xx_device *ms91xx = data;
> +	int offset = block * EDID_LENGTH;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < len; i++) {
> +		u16 address = MS91XX_REG_EDID_BASE + offset + i;
> +
> +		ret = ms91xx_read_byte(ms91xx, address);
> +		if (ret < 0)
> +			return ret;
> +		buf[i] = ret;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Should ms91xx_read_edid() check the bounds of block before computing the
register address?

In ms91xx_connector_get_modes(), drm_edid_read_custom() calls
_drm_do_get_edid(), which reads block 0 and then requests extension blocks up
to the count advertised by the sink (up to 255 extensions).

Because ms91xx_read_edid() does not limit block, any block >= 2 advances
address past the EDID region at MS91XX_REG_EDID_BASE (0xc000) into device
registers in 0xc100..0xffff, such as MS91XX_REG_MODE_SEQUENCE_2 (0xc620),
MS912X_REG_CHIP_ID (0xf000), and MS913X_REG_CHIP_ID (0xff00).

When block >= 128 (offset >= 0x4000), MS91XX_REG_EDID_BASE + offset + i
overflows the 16-bit address variable and wraps around to 0x0000..0x3fff,
reading low registers including MS91XX_REG_MODE_SEQUENCE_0 (0x0030) and
MS91XX_REG_MODE_SEQUENCE_1 (0x0033).

In addition, edid_block_read() retries each invalid block 4 times when
ms91xx_read_edid() returns 0, and each ms91xx_read_byte() performs two
synchronous USB control transfers, so an EDID advertising 255 extensions can
trigger up to 261,120 USB control transfers while holding mode_config.mutex.

[ ... ]

> +static int ms91xx_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	const struct drm_edid *edid;
> +	int ret = ms91xx_add_custom_mode(connector);
> +
> +	if (ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS ||
> +	    ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS_SVIDEO ||
> +	    ms91xx->port_type == MS91XX_VIDEO_PORT_UNKNOWN)
> +		return ret + ms91xx_add_cvbs_svideo_modes(connector);
> +
> +	if (ms91xx->port_type == MS91XX_VIDEO_PORT_YPBPR)
> +		return ret + ms91xx_add_ypbpr_modes(connector);
> +
> +	edid = drm_edid_read_custom(connector, ms91xx_read_edid, ms91xx);
> +	if (!edid)
> +		return ret + ms91xx_add_default_hdmi_vga_modes(connector);
> +
> +	if (drm_edid_connector_update(connector, edid) >= 0)
> +		ret += drm_edid_connector_add_modes(connector);
> +	drm_edid_free(edid);
> +	return ret;
> +}

[Severity: Low]
Can ms91xx_connector_get_modes() end up marking multiple modes on the same
connector with DRM_MODE_TYPE_PREFERRED?

In ms91xx_decode_custom_timing(), every decoded custom mode (up to two modes
when the "modify2" flash marker is present) is unconditionally assigned:

	display_mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;

When ms91xx_connector_get_modes() runs, ms91xx_add_custom_mode() adds all
decoded custom modes with DRM_MODE_TYPE_PREFERRED set, and then
ms91xx_connector_get_modes() unconditionally calls
ms91xx_add_cvbs_svideo_modes(), ms91xx_add_ypbpr_modes(),
ms91xx_add_default_hdmi_vga_modes(), or drm_edid_connector_add_modes(), each
of which also sets DRM_MODE_TYPE_PREFERRED on another mode.

Per struct drm_display_mode in include/drm/drm_modes.h, there should only be
one preferred mode per connector at any given time, and having multiple
preferred modes causes drm_mode_sort() to tie-break among them by resolution
(hdisplay * vdisplay).

[ ... ]

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_registers.c b/drivers/gpu/drm/ms91xx/ms91xx_registers.c
> new file mode 100644
> index 0000000000000..84cbdc78002b1
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_registers.c
> @@ -0,0 +1,386 @@

[ ... ]

> +static int
> +ms91xx_decode_custom_timing(struct ms91xx_custom_mode *custom_mode,
> +			    const struct ms91xx_custom_timing_record *record)
> +{
> +	struct drm_display_mode *display_mode = &custom_mode->display_mode;
> +	u16 hsyncwidth = le16_to_cpu(record->hsyncwidth);
> +	u16 vsyncwidth = le16_to_cpu(record->vsyncwidth);
> +	u16 hoffset = le16_to_cpu(record->hoffset);
> +	u16 voffset = le16_to_cpu(record->voffset);
> +	u16 hactive = le16_to_cpu(record->hactive);
> +	u16 vactive = le16_to_cpu(record->vactive);
> +	u16 htotal = le16_to_cpu(record->htotal);
> +	u16 vtotal = le16_to_cpu(record->vtotal);
> +	u16 pixclk = le16_to_cpu(record->pixclk);
> +	u16 vfreq = le16_to_cpu(record->vfreq);
> +	unsigned int hblank, vblank;
> +	int refresh;
> +
> +	if (!hactive || hactive > MS91XX_MAX_WIDTH ||
> +	    !vactive || vactive > MS91XX_MAX_HEIGHT ||
> +	    htotal <= hactive || vtotal <= vactive || !pixclk || !vfreq)
> +		return -EINVAL;

[Severity: Medium]
Should ms91xx_decode_custom_timing() reject odd hactive values (hactive % 2 != 0)?

In ms91xx_set_resolution(), the hardware is configured exclusively for packed
4:2:2 UYVY (MS91XX_PIXFMT_UYVY), which encodes horizontal pixels in pairs.

If a custom timing record in flash has an odd hactive and is selected with
fb->width == hactive, ms91xx_fb_send_rect() (added in the following patch)
computes:

	x = ALIGN_DOWN(rect->x1, 2);
	width = min_t(int, ALIGN(rect->x2, 2), fb->width) - x;

When rect->x2 == fb->width, min_t() clamps to the odd fb->width, producing an
odd width. ms91xx_fb_xrgb8888_to_yuv422() then calls
drm_fb_xrgb8888_to_uyvy(), which advances the destination buffer by width * 2
bytes per line while drm_fb_xrgb8888_to_uyvy_line() only writes
round_down(width, 2) * 2 bytes per line, leaving 2 trailing bytes of
uninitialized vmalloc_32() memory per scanline in transfer_buffer to be sent
over USB.

> +
> +	hblank = htotal - hactive;
> +	vblank = vtotal - vactive;
> +	if (!hsyncwidth || hsyncwidth > hoffset || hoffset > hblank ||
> +	    !vsyncwidth || vsyncwidth > voffset || voffset > vblank)
> +		return -EINVAL;
> +
> +	memset(display_mode, 0, sizeof(*display_mode));
> +	display_mode->clock = pixclk * 10;
> +	display_mode->hdisplay = hactive;
> +	display_mode->hsync_start = htotal - hoffset;
> +	display_mode->hsync_end = display_mode->hsync_start + hsyncwidth;
> +	display_mode->htotal = htotal;
> +	display_mode->vdisplay = vactive;
> +	display_mode->vsync_start = vtotal - voffset;
> +	display_mode->vsync_end = display_mode->vsync_start + vsyncwidth;
> +	display_mode->vtotal = vtotal;
> +	display_mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	display_mode->flags =
> +		record->polarity & MS91XX_TIMING_POSITIVE_HSYNC ?
> +			DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
> +	display_mode->flags |=
> +		record->polarity & MS91XX_TIMING_POSITIVE_VSYNC ?
> +			DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
> +	if (!(record->polarity & MS91XX_TIMING_PROGRESSIVE))
> +		display_mode->flags |= DRM_MODE_FLAG_INTERLACE;

[Severity: Medium]
How are interlaced custom timings intended to be handled between
ms91xx_decode_custom_timing() and ms91xx_connector_init()?

When !(record->polarity & MS91XX_TIMING_PROGRESSIVE),
ms91xx_decode_custom_timing() sets DRM_MODE_FLAG_INTERLACE on display_mode and
stores the entry in ms91xx->custom_modes[].

However, ms91xx_connector_init() does not set
ms91xx->connector.interlace_allowed = true. When
drm_helper_probe_single_connector_modes() validates the modes added by
ms91xx_add_custom_mode(), drm_mode_validate_flag() marks any
DRM_MODE_FLAG_INTERLACE mode as MODE_NO_INTERLACE and drm_mode_prune_invalid()
deletes it from the connector mode list.

Meanwhile, the decoded entry remains in ms91xx->custom_modes[] and struct
ms91xx_mode only records width, height, hz, and mode without tracking
interlacing:

	custom_mode->mode.width = hactive;
	custom_mode->mode.height = vactive;
	custom_mode->mode.hz = refresh;
	custom_mode->mode.mode = record->vic;

As a result, when a progressive mode with the same width, height, and refresh
rate is later validated or enabled in ms91xx_get_mode(), it will match the
pruned interlaced entry in ms91xx->custom_modes[].

> +
> +	refresh = DIV_ROUND_CLOSEST(vfreq, 100);
> +	if (!refresh || drm_mode_vrefresh(display_mode) != refresh)
> +		return -EINVAL;
> +
> +	drm_mode_set_name(display_mode);
> +	custom_mode->mode.width = hactive;
> +	custom_mode->mode.height = vactive;
> +	custom_mode->mode.hz = refresh;
> +	custom_mode->mode.mode = record->vic;
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019154820.68659-1-hjf3108@gmail.com?part=2