Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] video/hdmi: hdmi_infoframe_pack_only() missing EMP support - [Medium] video/hdmi: missing bounds check for VTEM base_refresh_rate - [Low] video/hdmi: VTEM pack returns -EINVAL on insufficient buffer --
commit 453def06fd91297425481a25d9a912cdc54c08f8 Author: Derek Foreman <[email protected]> video/hdmi: Add VTEM EMP packing VTEM EMPs are a type of infoframe that is required for Variable Refresh Rate. Add code to pack them. > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 45b42f14a7503..ed9abc7ad2285 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -983,6 +983,10 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, > length = hdmi_vendor_any_infoframe_pack(&frame->vendor, > buffer, size); > break; > + case HDMI_INFOFRAME_TYPE_EMP: > + length = hdmi_emp_infoframe_vtem_pack(&frame->vtem, > + buffer, size); > + break; [Severity: High] This adds EMP support to hdmi_infoframe_pack(), but does hdmi_infoframe_pack_only() also need an update? Drivers that rely exclusively on hdmi_infoframe_pack_only() (such as i915 and SDVO) will hit a WARN(1) backtrace and fail with -EINVAL if they attempt to pack an EMP (VTEM) infoframe. [ ... ] > +ssize_t hdmi_emp_infoframe_vtem_pack(struct hdmi_emp_infoframe_vtem *vtem, > + void *buffer, size_t size) [Severity: High] To be supported in hdmi_infoframe_pack_only(), wouldn't this function need to accept a const pointer? hdmi_infoframe_pack_only() guarantees a const union hdmi_infoframe *frame, but the lack of a const qualifier on vtem prevents it from being safely called without casting away constness. > +{ > + u8 *ptr = buffer; > + > + if (size < HDMI_INFOFRAME_SIZE(VTEM)) > + return -EINVAL; [Severity: Low] Should this return -ENOSPC instead of -EINVAL when the provided buffer is smaller than the required frame size? All other packing functions in drivers/video/hdmi.c (e.g., hdmi_avi_infoframe_pack_only(), hdmi_drm_infoframe_pack_only()) explicitly return -ENOSPC when size is insufficient. Returning -EINVAL might break error handling logic in callers that expect -ENOSPC. > + > + /* 4 bits max */ > + if (vtem->fva_factor_m1 > 0xF) > + return -EINVAL; > + [Severity: Medium] Does base_refresh_rate also need a bounds check here? The base_refresh_rate field is a u16 and can hold values up to 65535, but it is restricted to 10 bits. If a caller supplies a value greater than 1023, it will be silently truncated later in this function rather than gracefully rejected. Could this result in the hardware transmitting an incorrect VRR refresh rate to the display? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
