Re: [RFC PATCH v8] arm64: allwinner: add TrimUI Smart Pro (A133) board support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Sleeping in Atomic Context Violation during DRM encoder callbacks.
- [High] Unbalanced DSI clock gate lifecycle during DPMS ON/OFF cycle permanently disables the display.
- [High] Denial of Service (CPU Hog) from defeating unhandled IRQ storm protection.

Pre-existing issues:
- [High] TOCTOU race condition causing lost interrupts due to RMW on a Write-0-to-Clear (W0C) register.
- [High] TOCTOU Use-After-Free race condition in TCON TOP driver data access.
--

commit 739730d576ddf8a50358b7451672c4a7b9d896ed
Author: leow149 <leow149@lucidmail.xyz>

arm64: allwinner: add TrimUI Smart Pro (A133) board support

Board bring-up for the TrimUI Smart Pro handheld (Allwinner A133 Plus). This 
adds the new DTS, an OTM1289A DSI panel driver, a new PWM driver, and DRM 
display pipeline configurations.

> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index bf405a2aa3249..9d916efa8566b 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -226,20 +226,40 @@ void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
>  
>  	sun4i_tcon_channel_set_status(tcon, channel, enabled);
>  
> +	/*
> +	 * On disable, stop the DSI/CPU-mode retrigger timer/IRQ. This is a
> +	 * backstop for boards where sun6i_dsi_encoder_disable() itself
> +	 * already did this (see sun4i_tcon_dsi_stop_retrigger()'s comment
> +	 * for why that earlier call site is the one that actually matters)
> +	 * -- harmless and idempotent to also do it here.
> +	 */
> +	if (!enabled) {
> +		sun4i_tcon_dsi_stop_retrigger(tcon);
> +		/*
> +		 * sun4i_tcon0_mode_set() only ever calls
> +		 * sun4i_tcon0_set_dsi_gate(tcon, true) -- there was no
> +		 * matching false call anywhere, leaking this gate (and, via
> +		 * CLK_IGNORE_UNUSED, its CLK_MIPI_DSI parent) permanently on
> +		 * past this TCON's own disable. Ungate here instead; this
> +		 * call is a safe no-op on boards without a matching TCON TOP
> +		 * remote node (see its own comment), so it's fine to call
> +		 * unconditionally on every disable, not just DSI ones.
> +		 */
> +		sun4i_tcon0_set_dsi_gate(tcon, false);
> +	} else if (!tcon->dsi_cpu_mode && tcon->irq && !tcon->irq_enabled) {
> +		/*
> +		 * Non-DSI boards: tcon->irq was requested IRQF_NO_AUTOEN for
> +		 * every board using this driver (needed for the DSI/CPU-mode
> +		 * path's delayed enable -- see the comment on
> +		 * sun4i_tcon_init_irq()), so it needs an explicit enable_irq()
> +		 * somewhere for boards that never take that path at all. This
> +		 * runs on every CRTC enable -- both a real modeset (paired
> +		 * with sun4i_tcon_mode_set() in the same commit) and a plain
> +		 * DPMS off/on cycle, which does NOT call mode_set_nofb/
> +		 * sun4i_tcon_mode_set() again.
> +		 */
> +		enable_irq(tcon->irq);
> +		tcon->irq_enabled = true;
> +	}
>  }

[Severity: High]
What re-enables the DSI gate when the display wakes up from DPMS OFF?

While sun4i_tcon0_set_dsi_gate(tcon, false) is called when enabled is false,
the true branch here lacks a corresponding sun4i_tcon0_set_dsi_gate(tcon, true).
Since the gate is only enabled in sun4i_tcon_mode_set(), a simple DPMS ON
that doesn't trigger a full modeset might leave the clock gate permanently off.

[ ... ]

> @@ -772,12 +772,16 @@ void sun4i_tcon_dsi_stop_retrigger(struct sun4i_tcon *tcon)
>  {
>  	hrtimer_cancel(&tcon->dsi_retrigger_timer);
>  	/*
>  	 * disable_irq() (which might_sleep()s internally) is safe here:
>  	 * both call sites run in normal process/workqueue context, not a
>  	 * genuinely atomic section -- see the comment on
>  	 * sun4i_tcon_set_status()'s disable_irq() call for why (this
>  	 * function is called from there too, unchanged reasoning).
>  	 */
>  	if (tcon->irq_enabled) {
>  		disable_irq(tcon->irq);
>  		tcon->irq_enabled = false;
>  	}
>  }

[Severity: High]
Is it entirely safe to invoke disable_irq() here?

Because disable_irq() can sleep, and this path can be reached from 
sun6i_dsi_encoder_disable() during DRM non-blocking atomic commits, executing 
it in an atomic context would lead to a scheduling-while-atomic violation.

[ ... ]

> @@ -1151,12 +1151,33 @@ static irqreturn_t sun4i_tcon_handler(int irq, void *private)
>  {
>  	struct sun4i_tcon *tcon = private;
>  	struct drm_device *drm = tcon->drm;
>  	struct sun4i_crtc *scrtc = tcon->crtc;
>  	struct sunxi_engine *engine = scrtc->engine;
>  	unsigned int status;
> +	bool handled;
>  
>  	regmap_read(tcon->regs, SUN4I_TCON_GINT0_REG, &status);
>  
> +	handled = tcon->dsi_cpu_mode ||
> +		  (status & (SUN4I_TCON_GINT0_VBLANK_INT(0) |
> +			     SUN4I_TCON_GINT0_VBLANK_INT(1) |
> +			     SUN4I_TCON_GINT0_TCON0_TRI_FINISH_INT |
> +			     SUN4I_TCON_GINT0_TCON0_FSYNC_INT));
> +
> +	if (!handled)
> +		return IRQ_NONE;

[Severity: High]
Could this unconditional handled = true assignment in DSI/CPU mode defeat 
the kernel's unhandled IRQ storm protection?

If a hardware glitch causes the line to remain asserted, or if another device 
sharing this IRQ asserts, the kernel will never disable the line, potentially 
causing an infinite interrupt loop and a complete system lockup.

>  	/* Acknowledge the interrupt */
>  	regmap_update_bits(tcon->regs, SUN4I_TCON_GINT0_REG,
>  			   SUN4I_TCON_GINT0_VBLANK_INT(0) |
>  			   SUN4I_TCON_GINT0_VBLANK_INT(1) |
> -			   SUN4I_TCON_GINT0_TCON0_TRI_FINISH_INT,
> +			   SUN4I_TCON_GINT0_TCON0_TRI_FINISH_INT |
> +			   SUN4I_TCON_GINT0_TCON0_TRI_COUNTER_INT |
> +			   SUN4I_TCON_GINT0_TCON0_FSYNC_INT,
>  			   0);

[Severity: High]
This is a pre-existing issue, but using regmap_update_bits() on a 
Write-0-to-Clear (W0C) register like SUN4I_TCON_GINT0_REG seems to introduce 
a TOCTOU race condition.

Because it performs a read-modify-write, if a new interrupt bit asserts between 
the read and the write, writing 0 will silently clear it in hardware without 
the handler ever processing it.

> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index d504ae5832949..fae312e2ebf55 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c

[ ... ]

> @@ -910,21 +910,34 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  
>  	sun6i_dsi_start(dsi, DSI_START_HSC);
>  
>  	udelay(1000);
>  
> +	if (dsi->panel)
> +		drm_panel_prepare(dsi->panel);
> +
> +	if (dsi->panel)
> +		drm_panel_enable(dsi->panel);
> +
>  	sun6i_dsi_start(dsi, DSI_START_HSD);

[Severity: High]
Since DRM encoder enable callbacks can execute in atomic context during 
non-blocking commits, is it safe to invoke drm_panel_prepare() here? 

drm_panel_prepare() internally uses sleeping functions like msleep() which 
could lead to scheduling-while-atomic violations.

[ ... ]

> @@ -962,6 +962,21 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>  {
>  	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>  
>  	DRM_DEBUG_DRIVER("Disabling DSI output\n");
>  
> +	if (dsi->tcon)
> +		sun4i_tcon_dsi_stop_retrigger(dsi->tcon);
> +
>  	if (dsi->panel) {
>  		drm_panel_disable(dsi->panel);
>  		drm_panel_unprepare(dsi->panel);
>  	}

[Severity: High]
Does executing drm_panel_disable() here conflict with atomic context 
requirements? 

Because this is invoked from the encoder disable path, which may run in atomic 
context, it will eventually acquire mutexes in the PWM driver, causing the 
process to sleep.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> index 9cbd655518b20..3f519df7d97dc 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> @@ -157,6 +157,63 @@ int sun8i_tcon_top_set_dsi_gate(struct device *dev, bool enable)
>  {
>  	struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
>  	unsigned long flags;
>  	u32 val;
>  
>  	if (!sun8i_tcon_top_node_is_tcon_top(dev->of_node)) {
>  		dev_err(dev, "Device is not TCON TOP!\n");
>  		return -EINVAL;
>  	}
>  
> +	if (!tcon_top)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&tcon_top->reg_lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but dereferencing tcon_top here 
contains a TOCTOU Use-After-Free race condition.

If the TCON TOP driver is unbound concurrently, tcon_top might be freed 
before the spin_lock_irqsave(&tcon_top->reg_lock, flags) call executes.

> diff --git a/drivers/pwm/pwm-sun50i-a133.c b/drivers/pwm/pwm-sun50i-a133.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i-a133.c
> @@ -140,6 +140,94 @@ static int pwm_a133_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			  const struct pwm_state *state)
>  {
>  	struct pwm_a133 *pc = to_pwm_a133(chip);
>  	unsigned int ch = pwm->hwpwm;
>  	u32 entire_cycle, act_cycle, prescale_k, val;
>  	u32 pccr_reg = (ch < 2) ? PWM_A133_PCCR01_REG : PWM_A133_PCCR23_REG;
>  	int ret;
>  
>  	if (!state->enabled) {
>  		mutex_lock(&pc->lock);

[Severity: High]
Can acquiring a mutex here be safely performed?

This function is invoked from panel disable/enable paths. Since those can 
run during DRM non-blocking commits, acquiring a sleeping lock could cause 
system deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912233238.110960-1-leow149@lucidmail.xyz?part=1