Re: [PATCH v2 4/4] drm/fl2000: Add KMS display pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/fl2000: USB reset callbacks race unlocked
  drm_mode_config_reset() with ioctls
- [Medium] drm/fl2000: nonzero return from fl2000_resume() in post_reset
  unbinds driver
- [Medium] drm/fl2000: fl2000_drm_resume() leaves active CRTC
  unprogrammed if suspend fails
- [Medium] drm/fl2000: unscheduled intr->work in
  fl2000_intr_read_status() strands HDMI IRQs
--

commit 7aa37254cbf5a2f4dd511499c2b10b545e13febc
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add KMS display pipeline

This commit adds the DRM/KMS display pipeline for the Fresco Logic FL2000
USB display adapter, including plane/CRTC/encoder setup, HDMI and VGA
connectors, interrupt handling, and suspend/resume support.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_drm.c b/drivers/gpu/drm/fl2000/fl2000_drm.c
> new file mode 100644
> index 0000000000000..04efcf583d7de
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_drm.c

[ ... ]

> +int fl2000_drm_suspend(struct fl2000_device *fdev)
> +{
> +	int ret;
> +
> +	/* No hotplug events while going to sleep */
> +	fl2000_intr_suspend(fdev->intr);
> +
> +	/* Saves the state, turns the outputs off and stops the stream */
> +	ret = drm_mode_config_helper_suspend(&fdev->drm);
> +	if (ret) {
> +		drm_err(&fdev->drm, "Cannot save the display state (%d)\n", ret);
> +		fl2000_stream_disable(fdev->stream);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * fl2000_drm_resume() - bring the display back after system sleep or a USB reset
> + * @fdev: the DRM device
> + *
> + * Called once for the device, when all of it is awake again, also after a reset-resume. The chips
> + * may have lost power: they are set up as at bind time before the saved state is restored.
> + *
> + * Return: 0 on success, a negative error code otherwise
> + */
> +int fl2000_drm_resume(struct fl2000_device *fdev)
> +{
> +	struct regmap *regmap = fdev->regmap;
> +	int ret = 0;
> +
> +	/* First, as the IT66121 is reached through the FL2000 I2C engine */
> +	fl2000_regmap_drop_cache(regmap);
> +
> +	it66121_resume(fdev->bridge);
> +
> +	/* The same order as in fl2000_drm_create() */
> +	fl2000_chip_setup(fdev);
> +
> +	/* There is nothing to restore if saving the state failed */
> +	if (fdev->drm.mode_config.suspend_state) {
> +		ret = drm_mode_config_helper_resume(&fdev->drm);
> +		if (ret)
> +			drm_err(&fdev->drm, "Cannot restore the display after sleep (%d)\n", ret);
> +	}

[Severity: Medium]
If drm_mode_config_helper_suspend() fails in fl2000_drm_suspend(), does
fl2000_drm_resume() leave an active CRTC unprogrammed and its stream
stopped?

When drm_mode_config_helper_suspend() fails, fdev->drm.mode_config.suspend_state
remains NULL and the DRM software state stays enabled, while
fl2000_drm_suspend() disables fdev->stream.  On resume, it66121_resume()
and fl2000_chip_setup() reset both chips, clearing the PLL, timings, and
HDMI transmitter state, but because suspend_state is NULL,
drm_mode_config_helper_resume() is skipped.  The hardware stays unprogrammed
and the stream stays disabled while fdev->crtc.state remains active, so
subsequent page flips or plane updates do not trigger a modeset and the
display remains blank.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_drv.c b/drivers/gpu/drm/fl2000/fl2000_drv.c
> index 31d1535530a6e..663fb5daa05ae 100644
> --- a/drivers/gpu/drm/fl2000/fl2000_drv.c
> +++ b/drivers/gpu/drm/fl2000/fl2000_drv.c

[ ... ]

> +static int fl2000_resume(struct usb_interface *interface)
>  {
> -	dev_dbg(&interface->dev, "resume\n");
> +	struct fl2000_devs *devs = usb_get_intfdata(interface);
> +
> +	if (!devs || !devs->suspended)
> +		return 0;
> +
> +	devs->suspended = false;
> +
> +	return fl2000_drm_resume(devs->fdev);
> +}
> +
> +/*
> + * A reset of the USB device (the storage function of the adapter asks for one when it times out)
> + * is the same as far as the display goes. Nonzero would make the USB core unbind the driver.
> + */
> +static int fl2000_pre_reset(struct usb_interface *interface)
> +{
> +	fl2000_suspend(interface, PMSG_SUSPEND);
>  
>  	return 0;
>  }
>  
>  static struct usb_driver fl2000_driver = {
>  	.name = KBUILD_MODNAME,
>  	.probe = fl2000_probe,
>  	.disconnect = fl2000_disconnect,
>  	.suspend = fl2000_suspend,
>  	.resume = fl2000_resume,
> +	.reset_resume = fl2000_resume,
> +	.pre_reset = fl2000_pre_reset,
> +	.post_reset = fl2000_resume,

[Severity: High]
Can calling drm_mode_config_helper_suspend() and
drm_mode_config_helper_resume() from .pre_reset and .post_reset cause a
use-after-free on DRM modeset states when a USB reset runs concurrently
with userspace ioctls?

Unlike system sleep, usb_reset_device() runs while userspace is active.
drm_mode_config_helper_resume() -> drm_atomic_helper_resume() calls
drm_mode_config_reset() before acquiring modeset locks:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_resume() {
	...
	drm_mode_config_reset(dev);

	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
	...
}

drm_mode_config_reset() destroys and reallocates plane->state, crtc->state,
and connector->state without holding any modeset locks:

CPU0 (usb_reset_device)           CPU1 (compositor ioctl)
fl2000_resume()
  fl2000_drm_resume()
    drm_mode_config_helper_resume()
      drm_atomic_helper_resume()
                                  drm_mode_atomic_ioctl()
                                    drm_atomic_get_crtc_state()
                                      drm_modeset_lock(&crtc->mutex, ...)
        drm_mode_config_reset()
          atomic_destroy_state()
            kfree(crtc->state)
                                      atomic_duplicate_state(crtc)
                                        __drm_atomic_helper_crtc_duplicate_state()
                                          <-- use-after-free / NULL deref

In addition, between fl2000_pre_reset() and .post_reset,
drm_mode_config_helper_suspend() leaves the CRTC disabled with all modeset
locks released, so concurrent page flips from the running compositor fail
with -EINVAL.

[Severity: Medium]
Also, if fl2000_drm_resume() returns an error during .post_reset, does that
cause the USB core to unbind the driver and fail to rebind it?

fl2000_pre_reset() ignores the return value of fl2000_suspend() because
usb_reset_device() treats a nonzero return as a request to unbind and
rebind the interface, but .post_reset is wired directly to fl2000_resume().
usb_reset_device() calls .post_reset in reverse interface order (2, 1, 0):
on interface 2, fl2000_resume() clears devs->suspended and returns any
error from fl2000_drm_resume(), setting needs_binding = 1 on interface 2,
and then returns 0 on interfaces 1 and 0.
usb_unbind_and_rebind_marked_interfaces() then unbinds interface 2 (running
fl2000_disconnect() and releasing all interfaces) and only attempts to
rebind interface 2, which does not match fl2000_id_table.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_interrupt.c b/drivers/gpu/drm/fl2000/fl2000_interrupt.c
> new file mode 100644
> index 0000000000000..8f8f9fac1ea35
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_interrupt.c

[ ... ]

> +static int fl2000_intr_read_status(struct fl2000_intr *intr, u32 *status)
> +{
> +	int ret;
> +
> +	ret = fl2000_read_status(intr->regmap, status);
> +	if (ret)
> +		return ret;
> +
> +	atomic_or(*status & FL2000_SINK_EVENTS, &intr->events);
> +
> +	return 0;
> +}
> +
> +/**
> + * fl2000_intr_monitor_connected() - does the FL2000 sense a monitor on its monitor input
> + * @intr: interrupt context
> + *
> + * The chip powers the DAC and the PLL only while this is the case. On the boards with both
> + * outputs the input follows whichever monitor is plugged in, so it does not tell VGA from HDMI.
> + * While HDMI is being driven the sense is forced on and tells nothing: that counts as not sensing,
> + * or an HDMI monitor going away would look like a VGA one showing up, until the CRTC is disabled.
> + *
> + * Return: 1 if a monitor is sensed, 0 if not, a negative error code if the status cannot be read
> + */
> +int fl2000_intr_monitor_connected(struct fl2000_intr *intr)
> +{
> +	u32 status;
> +	int ret;
> +
> +	ret = fl2000_intr_read_status(intr, &status);
> +	if (ret)
> +		return ret;

[Severity: Medium]
When fl2000_intr_monitor_connected() reads FL2000_VGA_STATUS_HDMI_EVENT
during VGA connector detection, can that strand the event in intr->events
without acknowledging the IT66121 interrupt?

Reading FL2000_VGA_STATUS_REG clears the pending interrupt and event bits
in the FL2000 hardware.  When fl2000_vga_detect_ctx() ->
fl2000_intr_monitor_connected() reads the status register (for example
during initial probing in fl2000_drm_create() before fl2000_intr_start(),
or during periodic polling before the USB interrupt endpoint fires),
fl2000_intr_read_status() saves FL2000_SINK_EVENTS into intr->events, but
nothing queues intr->work to run fl2000_drm_hotplug() and call
it66121_ack_interrupts().  The IT66121 interrupt line then remains asserted
and cannot signal further HDMI hotplug events.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-fl2000-upstream-v2-0-201f87708cb0@gmail.com?part=4