Re: [PATCH v3 07/14] drm/solomon: ssd16xx: Add power management support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/solomon: ssd16xx: missing error rollback in ssd16xx_pm_suspend()
- [Medium] drm/solomon: ssd16xx: missing state restore in ssd16xx_pm_runtime_resume()
- [Medium] drm/solomon: ssd16xx: premature runtime suspend in ssd16xx_probe()
- [Medium] drm/solomon: ssd16xx: incomplete PM error cleanup in ssd16xx_probe()
--

commit 7bb59d973ba345233d1093837dd50809ed285f26
Author: Devarsh Thakkar <devarsht@ti.com>

drm/solomon: ssd16xx: Add power management support

This patch adds runtime PM with a 35-second autosuspend delay and system
suspend/resume support to the SSD16xx e-paper display driver.

> diff --git a/drivers/gpu/drm/solomon/ssd16xx.c b/drivers/gpu/drm/solomon/ssd16xx.c
> index ff0e459082dea..99f11b7ec03e9 100644
> --- a/drivers/gpu/drm/solomon/ssd16xx.c
> +++ b/drivers/gpu/drm/solomon/ssd16xx.c

[ ... ]

> @@ -2011,9 +2059,26 @@ static int ssd16xx_probe(struct spi_device *spi)
>  
>  	drm_mode_config_reset(drm);
>  
> +	/*
> +	 * Mark the device active before enabling runtime PM.  The SPI device
> +	 * persists across module reload cycles; pm_runtime_enable() alone
> +	 * does not clear a stale dev->power.runtime_error from a previous
> +	 * module load, which would cause pm_runtime_resume_and_get() to
> +	 * return -EINVAL on every subsequent call.
> +	 */
> +	/* Use level1 (RAM retained) for runtime PM idle. */
> +	device->deep_sleep_mode = device->controller_cfg->deep_sleep_mode_level1;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, SSD16XX_PM_AUTOSUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);

[Severity: Medium]
Does calling pm_runtime_set_autosuspend_delay() after pm_runtime_enable()
here cause an immediate synchronous runtime suspend during ssd16xx_probe()?

At the point pm_runtime_set_autosuspend_delay() runs, the device is enabled
with usage_count == 0, dev->power.use_autosuspend is still false, and
dev->power.last_busy has not been marked. Inside the runtime PM core:

  pm_runtime_set_autosuspend_delay()
    -> update_autosuspend()
      -> rpm_idle(dev, RPM_AUTO)
        -> rpm_suspend(dev, RPM_AUTO)
          -> pm_runtime_autosuspend_expiration()

Because use_autosuspend is false (and last_busy is 0),
pm_runtime_autosuspend_expiration() returns 0, so rpm_suspend() invokes
ssd16xx_pm_runtime_suspend() synchronously and puts the panel into deep
sleep right after ssd16xx_hw_reset(), only for drm_client_setup() to
immediately wake and hardware-reset the panel a second time.

Should pm_runtime_set_autosuspend_delay(), pm_runtime_use_autosuspend(), and
pm_runtime_mark_last_busy() be called before pm_runtime_enable()?

> +
>  	ret = drm_dev_register(drm, 0);
> -	if (ret)
> +	if (ret) {
> +		pm_runtime_disable(dev);
>  		return ret;
> +	}

[Severity: Medium]
If drm_dev_register() fails in ssd16xx_probe(), is pm_runtime_disable(dev)
alone sufficient to clean up the runtime PM state?

Because pm_runtime_reinit() on unbind does not clear
dev->power.use_autosuspend or run the runtime_suspend callback, only calling
pm_runtime_disable(dev) leaves use_autosuspend enabled on the persistent
spi_device and leaves the panel powered if it is still active. Should this
error path call pm_runtime_dont_use_autosuspend(dev) and
pm_runtime_force_suspend(dev) to match ssd16xx_remove() and
ssd16xx_shutdown()?

>  
>  	drm_dbg(drm, "SSD16xx e-paper display initialized (%dx%d, %d° rotation)\n",
>  		device->width, device->height, device->orientation);
> @@ -2029,6 +2094,12 @@ static void ssd16xx_remove(struct spi_device *spi)
>  
>  	drm_dev_unplug(&device->drm);
>  	drm_atomic_helper_shutdown(&device->drm);
> +	pm_runtime_dont_use_autosuspend(&spi->dev);
> +	/*
> +	 * Put the panel to sleep regardless of clear_on_disable, which can make
> +	 * atomic_disable skip touching the hardware.
> +	 */
> +	pm_runtime_force_suspend(&spi->dev);
>  }
>  
>  static void ssd16xx_shutdown(struct spi_device *spi)
> @@ -2036,8 +2107,139 @@ static void ssd16xx_shutdown(struct spi_device *spi)
>  	struct ssd16xx_device *device = spi_get_drvdata(spi);
>  
>  	drm_atomic_helper_shutdown(&device->drm);
> +	pm_runtime_dont_use_autosuspend(&spi->dev);
> +	pm_runtime_force_suspend(&spi->dev);
>  }
>  
> +static int ssd16xx_pm_suspend(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	struct drm_device *drm = &device->drm;
> +	int ret;
> +
> +	/* Use level2 deep sleep for system suspend; SSD1683 loses RAM but saves more power. */
> +	device->deep_sleep_mode = device->controller_cfg->deep_sleep_mode_level2;
> +	dev_dbg(dev, "system suspend: entering deep sleep mode 0x%02x\n",
> +		device->deep_sleep_mode);
> +
> +	ret = drm_mode_config_helper_suspend(drm);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If the autosuspend timer already fired the device is RPM_SUSPENDED
> +	 * at level1.  pm_runtime_force_suspend() would skip its callback, so
> +	 * level2 would never reach the hardware.
> +	 *
> +	 * SPI is unresponsive in deep sleep; HWRESET is the only exit.
> +	 * After the reset, level2 can be sent directly without a full hw_init.
> +	 *
> +	 * Skip this for controllers where level1 == level2 (e.g. SSD1677 which
> +	 * has only one deep sleep mode): the device is already in the deepest
> +	 * available state and no upgrade is needed.
> +	 */
> +	if (pm_runtime_status_suspended(dev)) {
> +		int err = 0;
> +
> +		if (device->controller_cfg->deep_sleep_mode_level1 !=
> +		    device->controller_cfg->deep_sleep_mode_level2) {
> +			dev_dbg(dev, "system suspend: upgrading level1->level2 via HWRESET\n");
> +			ssd16xx_hw_reset(device);
> +			ssd16xx_wait_for_device(device, &err);
> +			ssd16xx_send_cmd(device, SSD16XX_CMD_DEEP_SLEEP_MODE, &err);
> +			ssd16xx_send_data(device,
> +					  device->controller_cfg->deep_sleep_mode_level2,
> +					  &err);
> +		} else {
> +			dev_dbg(dev, "system suspend: already at deepest sleep mode\n");
> +		}
> +		device->initialized = false;
> +		device->pm_force_suspended = false;  /* Skip force_resume on resume */
> +		return err;
> +	}
> +
> +	/* Force runtime-suspended state for clean resume. */
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret) {
> +		drm_mode_config_helper_resume(drm);
> +		return ret;
> +	}
> +	device->pm_force_suspended = true;  /* Resume must call force_resume */
> +
> +	return 0;
> +}

[Severity: High]
What happens in ssd16xx_pm_suspend() if an error occurs during the
pm_runtime_status_suspended(dev) upgrade path, or if
drm_mode_config_helper_suspend() or pm_runtime_force_suspend() fails?

In the pm_runtime_status_suspended(dev) branch, if ssd16xx_wait_for_device()
times out or ssd16xx_send_cmd()/ssd16xx_send_data() fails,
ssd16xx_pm_suspend() returns err directly without calling
drm_mode_config_helper_resume(drm). Because the suspend callback returns an
error, the PM core aborts system suspend and will not call
ssd16xx_pm_resume(), which leaves the DRM modeset pipeline and in-kernel
clients suspended, leaks drm->mode_config.suspend_state, and leaves the
panel awake after ssd16xx_hw_reset() while runtime PM status remains
RPM_SUSPENDED.

In addition, device->deep_sleep_mode is overwritten with
deep_sleep_mode_level2 at the start of ssd16xx_pm_suspend() and is never
restored to deep_sleep_mode_level1 on any of the three failure paths
(drm_mode_config_helper_suspend() failure, the level1->level2 upgrade
failure, or pm_runtime_force_suspend() failure). Wouldn't that cause all
subsequent runtime PM idle suspends to enter level-2 sleep (0x03) and lose
display RAM?

[ ... ]

> +static int ssd16xx_pm_runtime_resume(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	int ret;
> +
> +	dev_dbg(dev, "runtime resume: initialized=%d%s\n",
> +		device->initialized,
> +		!device->initialized ? " (running hw_init)" : "");
> +
> +	/*
> +	 * pm_runtime_suspend clears initialized after sending the deep sleep
> +	 * command.  If a new app opens before the suspend fires (the put was
> +	 * cancelled), initialized is still true and hw_init is unnecessary.
> +	 */
> +	if (!device->initialized) {
> +		ret = ssd16xx_hw_init(device);
> +		if (ret)
> +			return ret;
> +
> +		device->initialized = true;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
When ssd16xx_pm_runtime_resume() runs ssd16xx_hw_init() while the CRTC is
already enabled, does it leave the border waveform register and refresh
state out of sync with the driver state?

ssd16xx_hw_init() issues both ssd16xx_hw_reset() and SSD16XX_CMD_SW_RESET,
which resets SSD16XX_CMD_BORDER_WAVEFORM_CONTROL (0x3C) to its power-on
default without reprogramming it. In ssd16xx_fb_dirty(), register 0x3C is
only written when border_refresh_on_every_update, init_refresh_pending, or
border_waveform_pending is set:

	if (device->border_refresh_on_every_update || device->init_refresh_pending ||
	    device->border_waveform_pending) {
		u8 idx = device->border_waveform_update_idx;
		u8 border = device->controller_cfg->border_waveform_table[idx];
		...
		ssd16xx_send_cmd(device, SSD16XX_CMD_BORDER_WAVEFORM_CONTROL, &err);
		ssd16xx_send_data(device, border, &err);
		device->border_waveform_pending = false;
	}

When waking from runtime suspend in ssd16xx_plane_atomic_update(),
ssd16xx_crtc_atomic_enable() does not run, so if
border_refresh_on_every_update is false and the pending flags were already
consumed before autosuspend, register 0x3C is never restored to
border_waveform_update_idx.

Also, on controllers where deep_sleep_mode_level1 is 0x03 (such as SSD1677),
controller RAM is lost during runtime suspend. Without re-arming
init_refresh_pending (or forcing a full refresh), wouldn't the next update
in SSD16XX_REFRESH_PARTIAL mode compare the new BW RAM against lost RED RAM?
Finally, the commit message mentions that level-1 sleep allows fast resume
via SPI wakeup, whereas ssd16xx_pm_runtime_resume() always calls
ssd16xx_hw_init() which performs a full GPIO hardware reset.

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019213726.4145532-1-devarsht@ti.com?part=7