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 <[email protected]> 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/[email protected]?part=2
