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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/ms91xx: unbounded EDID block offset and u16 wrap in ms91xx_read_edid()
- [Medium] drm/ms91xx: multiple preferred modes set in ms91xx_connector_get_modes()
- [Medium] drm/ms91xx: unplug -ENODEV treated as unknown status in ms91xx_detect()
--

commit 32cd5aee0f634799386d1af3debfec7f0d6318ac
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/ms91xx: Add register access and connector support

This commit adds HID register and flash access helpers, custom timing
record decoding, and DRM connector mode enumeration and detection for
MacroSilicon MS912x and MS913x USB display adapters.

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

[ ... ]

> +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]
Can ms91xx_read_edid() in drivers/gpu/drm/ms91xx/ms91xx_connector.c read
past the EDID register window and wrap address around u16 when a sink
reports EDID extension blocks?

When ms91xx_connector_get_modes() calls drm_edid_read_custom(),
_drm_do_get_edid() can request up to 255 extension blocks. Because
MS91XX_REG_EDID_BASE is 0xc000 in drivers/gpu/drm/ms91xx/ms91xx.h, higher
block indices read across unrelated registers such as
MS91XX_REG_MODE_SEQUENCE_2 (0xc620), MS912X_REG_CHIP_ID (0xf000),
MS913X_CUSTOM_TIMING_BASE (0xfc50), and MS913X_REG_CHIP_ID (0xff00).

Once block reaches 128 (offset >= 0x4000), MS91XX_REG_EDID_BASE + offset + i
overflows u16 and wraps into 0x0000..0x3fff, reading low control and status
registers such as MS91XX_REG_MODE_SEQUENCE_0 (0x0030) through
MS91XX_REG_MODE_SEQUENCE_1 (0x0033). Should ms91xx_read_edid() bound-check
block and offset + len against the hardware EDID buffer size before reading?

> +
> +static int ms91xx_add_cea_modes(struct drm_connector *connector,
> +				const u8 *vics, size_t num_vics)
> +{
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +	int count = 0;
> +
> +	drm_edid_connector_update(connector, NULL);
> +
> +	for (i = 0; i < num_vics; i++) {
> +		mode = drm_display_mode_from_cea_vic(connector->dev, vics[i]);
> +		if (!mode)
> +			continue;
> +
> +		if (!count)
> +			mode->type |= DRM_MODE_TYPE_PREFERRED;
> +		drm_mode_probed_add(connector, mode);
> +		count++;
> +	}
> +
> +	return count;
> +}

[ ... ]

> +static int
> +ms91xx_add_default_hdmi_vga_modes(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *mode_config = &connector->dev->mode_config;
> +	int count;
> +
> +	drm_edid_connector_update(connector, NULL);
> +	count = drm_add_modes_noedid(connector, mode_config->max_width,
> +				     mode_config->max_height);
> +	if (count)
> +		drm_set_preferred_mode(connector, 1024, 768);
> +
> +	return count;
> +}
> +
> +static int ms91xx_add_custom_mode(struct drm_connector *connector)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +	int count = 0;
> +
> +	for (i = 0; i < ms91xx->num_custom_modes; i++) {
> +		mode = drm_mode_duplicate(
> +			connector->dev,
> +			&ms91xx->custom_modes[i].display_mode);
> +		if (!mode)
> +			continue;
> +
> +		drm_mode_probed_add(connector, mode);
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
> +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: Medium]
When custom timings are present in flash, does ms91xx_connector_get_modes()
in drivers/gpu/drm/ms91xx/ms91xx_connector.c end up marking multiple modes
on the connector with DRM_MODE_TYPE_PREFERRED?

In drivers/gpu/drm/ms91xx/ms91xx_registers.c, ms91xx_read_custom_timing()
marks the first custom mode as preferred:

    if (ms91xx->num_custom_modes)
            ms91xx->custom_modes[0].display_mode.type |= DRM_MODE_TYPE_PREFERRED;

During connector probing, ms91xx_connector_get_modes() calls
ms91xx_add_custom_mode(), which duplicates custom_modes[0].display_mode onto
connector->probed_modes with DRM_MODE_TYPE_PREFERRED set. It then proceeds to
call 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.

When drm_helper_probe_single_connector_modes() calls drm_mode_sort(), ties
between modes with DRM_MODE_TYPE_PREFERRED are broken by hdisplay * vdisplay,
causing any fallback, CEA, or EDID preferred mode with a larger resolution
to sort ahead of custom_modes[0]. Should only one mode be marked preferred?

> +
> +static enum drm_connector_status ms91xx_detect(struct drm_connector *connector,
> +					       bool force)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	int status = ms91xx_read_byte(ms91xx, MS91XX_REG_DISPLAY_STATUS);
> +
> +	if (status < 0)
> +		return connector_status_unknown;
> +
> +	return status == 1 ? connector_status_connected :
> +			     connector_status_disconnected;
> +}

[Severity: Medium]
When the USB device is unplugged, ms91xx_read_byte() in
drivers/gpu/drm/ms91xx/ms91xx_registers.c returns -ENODEV (either when
drm_dev_enter() fails or when usb_control_msg_send() fails on disconnect).

In drivers/gpu/drm/ms91xx/ms91xx_connector.c, ms91xx_detect() maps all
status < 0 values to connector_status_unknown instead of
connector_status_disconnected. In drm_helper_probe_single_connector_modes(),
connector_status_unknown continues to call ms91xx_connector_get_modes(),
which then fails drm_edid_read_custom() and populates fallback modes via
ms91xx_add_default_hdmi_vga_modes() (or CEA/custom modes) on the unplugged
device rather than pruning modes.

Should ms91xx_detect() return connector_status_disconnected when status is
-ENODEV (or when !drm_dev_enter())?

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