drm/tiny: Add RAiO RA8875 display controller driver support

drivers/gpu/drm/tiny/Kconfig config DRM_PIXPAPER · line 107

[Severity: Low]
In drivers/gpu/drm/tiny/Kconfig, DRM_RA8875 selects DRM_GEM_SHMEM_HELPER, which depends on MMU in drivers/gpu/drm/Kconfig.

Without a dependency on MMU, enabling DRM_RA8875 on a !CONFIG_MMU target triggers an unmet direct dependency warning and fails to build in drm_gem_shmem_helper.c.

Should DRM_RA8875 depend on MMU as well, matching the other shmem-based drivers in drivers/gpu/drm/tiny/Kconfig?

[ ... ]

drivers/gpu/drm/tiny/ra8875.c line 260

[Severity: High]
In drivers/gpu/drm/tiny/ra8875.c, are hndr and vndr in ra8875_hw_init() being computed from the total blanking interval rather than the back porch?

In drm_display_mode_from_videomode():

    dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
    dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
    dmode->htotal = dmode->hsync_end + vm->hback_porch;

Since HSTR (and VSTR0/VSTR1) already program the front porch and HPWR (and VPWR) program the sync pulse width, RA8875_HNDR and RA8875_VNDR0/RA8875_VNDR1 program the horizontal and vertical non-display (back porch) periods.

Using mode->htotal - mode->hdisplay and mode->vtotal - mode->vdisplay counts the front porch and sync pulse width a second time. Should these be computed from mode->htotal - mode->hsync_end and mode->vtotal - mode->vsync_end instead?

[ ... ]

drivers/gpu/drm/tiny/ra8875.c line 397

[Severity: High]
In drivers/gpu/drm/tiny/ra8875.c, doesn't the atomic commit order cause ra8875_crtc_atomic_enable() to wipe out the initial plane update on every modeset?

In ra8875_probe(), ra8875->rst_gpio is requested with GPIOD_OUT_HIGH and drm->mode_config.helper_private is left unset. When drm_atomic_helper_commit() runs a modeset (including the initial fbdev modeset from drm_client_setup()), it uses the default drm_atomic_helper_commit_tail(), which executes:

    drm_atomic_helper_commit_tail()
      drm_atomic_helper_commit_planes(dev, state, 0)
        ra8875_plane_atomic_update()
          ra8875_fb_dirty()
      drm_atomic_helper_commit_modeset_enables(dev, state)
        ra8875_crtc_atomic_enable()
          ra8875_hw_init()
          ra8875_clear_screen()

As a result, ra8875_plane_atomic_update() writes the framebuffer while the chip is still held in reset, and then ra8875_crtc_atomic_enable() resets the hardware and zeroes the entire display memory in ra8875_clear_screen().

In addition, ra8875_plane_atomic_check() passes can_update_disabled = true to drm_atomic_helper_check_plane_state(), and ra8875_plane_atomic_update() does not check whether the CRTC is active.

Should drm->mode_config.helper_private be configured with .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm (so CRTC enable runs before plane updates, and plane updates are skipped when the CRTC is inactive), and can_update_disabled be set to false in ra8875_plane_atomic_check()?

[Severity: High]
Also in ra8875_crtc_atomic_enable() in drivers/gpu/drm/tiny/ra8875.c, can a modeset racing with device removal in ra8875_remove() cause a use-after-free on ra8875->txbuf and ra8875->rst_gpio?

In ra8875_probe(), ra8875->txbuf and ra8875->rst_gpio are allocated with devm_kzalloc(&spi->dev, ...) and devm_gpiod_get_optional(&spi->dev, ...), so devres frees them as soon as ra8875_remove() returns, even if a DRM file descriptor remains open.

Unlike ra8875_plane_atomic_update(), ra8875_crtc_atomic_enable() does not guard its hardware and buffer accesses with drm_dev_enter() and drm_dev_exit(). If an atomic commit enters ra8875_crtc_atomic_enable() concurrently with or after drm_dev_unplug() in ra8875_remove(), drm_dev_unplug() will not synchronize with it and ra8875_clear_screen() can memset() and SPI-transfer from the freed ra8875->txbuf buffer.

Should ra8875_crtc_atomic_enable() (and ra8875_crtc_atomic_disable()) be guarded with drm_dev_enter() and drm_dev_exit()?

[ ... ]

drivers/gpu/drm/tiny/ra8875.c line 455

[Severity: Medium]
In ra8875_fb_dirty() in drivers/gpu/drm/tiny/ra8875.c, drm_fb_swab() reads from the shadow-mapped framebuffer with the CPU, but drm_gem_begin_shadow_fb_access() only vmaps the buffer without calling dma_buf_begin_cpu_access().

When displaying an imported PRIME DMA-BUF on a non-cache-coherent system, can omitting drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) and drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE) around the CPU read result in stale cache reads and display corruption?

[Severity: Medium]
Also in ra8875_fb_dirty() in drivers/gpu/drm/tiny/ra8875.c, doesn't passing ra8875->txbuf + 1 as the destination buffer to drm_fb_swab() perform unaligned 16-bit stores on every pixel?

In ra8875_probe(), ra8875->txbuf is allocated with devm_kzalloc(), which is even-aligned, making ra8875->txbuf + 1 an odd address. For DRM_FORMAT_RGB565, drm_fb_swab() calls drm_fb_swab16_line(), which casts the destination pointer directly to u16 * and writes *dbuf16++ = swab16(*sbuf16++) without put_unaligned().

Can this trigger alignment faults on architectures requiring natural alignment as well as UBSAN alignment warnings?

[ ... ]

drivers/gpu/drm/tiny/ra8875.c line 502

[Severity: Critical]
As reported by Sashiko on v4 [1], can a userspace-supplied display mode overflow ra8875->txbuf in ra8875_clear_screen()?

In ra8875_probe() in drivers/gpu/drm/tiny/ra8875.c, ra8875->txbuf is allocated for a fixed maximum width of RA8875_MAX_WIDTH (800):

    buf_size = 1 + RA8875_TXBUF_ROWS * RA8875_MAX_WIDTH * 2;
    ra8875->txbuf = devm_kzalloc(&spi->dev, buf_size, GFP_KERNEL);

While drm->mode_config.max_width restricts framebuffer sizes, a userspace DRM master can submit a custom CRTC mode via KMS (for example with the primary plane disabled). Because ra8875_mode_valid() only checks that the horizontal timings are multiples of 8 and does not call drm_crtc_helper_mode_valid_fixed(crtc, mode, &ra8875->mode) or bound mode->hdisplay to RA8875_MAX_WIDTH, committing a mode with mode->hdisplay > 800 causes ra8875_crtc_atomic_enable() -> ra8875_clear_screen() to memset() and SPI-write 1 + 16 * mode->hdisplay * 2 bytes past the end of ra8875->txbuf.

Additionally, neither ra8875_probe() nor ra8875_mode_valid() verifies that the horizontal and vertical front porch, sync pulse, and back porch lengths are strictly positive, so a zero-length porch or sync pulse will underflow when subtracting 1 in ra8875_hw_init().

Should ra8875_mode_valid() validate the mode with drm_crtc_helper_mode_valid_fixed() and verify that all porch and sync pulse intervals are non-zero?

[ ... ]

drivers/gpu/drm/tiny/ra8875.c line 593

[Severity: Medium]
In ra8875_probe() in drivers/gpu/drm/tiny/ra8875.c, the driver requests the optional reset GPIO, but never acquires or enables the vdd-supply regulator that is marked as required in Documentation/devicetree/bindings/display/panel/raio,ra8875.yaml.

If vdd-supply is backed by a switchable regulator that is disabled at boot or turned off by the regulator core as unused, won't the RA8875 remain unpowered when ra8875_hw_init() attempts SPI transfers? Should ra8875_probe() enable the vdd supply (for example via devm_regulator_get_enable(dev, "vdd"))?

[1] https://lore.kernel.org/r/[email protected]

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2