Re: [RFC PATCH v6] 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 functions called from atomic context in DRM encoder callbacks.
- [High] State Confusion / Overlapping Transfers due to uncleared pending interrupts on IRQ re-enable.
- [High] Incorrect Horizontal Display Timings (Front and Back Porch Swap).
- [Medium] CPU Hog: The DSI retrigger timer will never stop.

Pre-existing issues:
- [High] TOCTOU Race Condition on Write-0-to-Clear Interrupt Acknowledge.
--

commit 2a1faaf3f9e395a5ec4a2a530f311f244c0a15e3
Author: leow149 <leow149@lucidmail.xyz>
Subject: arm64: allwinner: add TrimUI Smart Pro (A133) board support

This patch brings up board support for the TrimUI Smart Pro handheld.
It introduces new DTS configuration, panel timings for OTM1289A, a
new PWM driver, and modifies TCON/DSI files for CPU/8080 interface
bring-up.

> diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c b/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c
[ ... ]
> +static const struct drm_display_mode otm1289a_mode = {
> +	.clock = 69000,
> +	.hdisplay = OTM1289A_HDISPLAY,
> +	.hsync_start = OTM1289A_HDISPLAY + 88,
> +	.hsync_end = OTM1289A_HDISPLAY + 88 + 4,

[Severity: High]
Is it possible these horizontal porch values are swapped?

The DRM front porch is calculated as htotal - hdisplay - hbp 
(880 - 720 - 88 = 72). However, in otm1289a_mode, hsync_start is set to 
OTM1289A_HDISPLAY + 88, which uses the vendor's back porch value instead of 
the computed front porch offset of 72.

Could this cause incorrect display output shifting when the hardware is 
configured?

[ ... ]
> +static int otm1289a_prepare(struct drm_panel *panel)
> +{
[ ... ]
> +	if (ctx->reset_gpio) {
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		msleep(20);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +		msleep(100);
> +	}

[Severity: High]
Is it safe to call sleeping functions from this atomic context?

The DRM subsystem architectural guidelines specify that encoder atomic enable 
and disable callbacks execute in an atomic context where sleeping is strictly 
forbidden.

The panel's prepare and disable hooks (which are invoked from the encoder 
enable/disable paths) use msleep().

Could this lead to kernel warnings, system instability, or deadlocks during 
display modesets?

[ ... ]
> diff --git a/drivers/pwm/pwm-sun50i-a133.c b/drivers/pwm/pwm-sun50i-a133.c
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i-a133.c
[ ... ]
> +static int pwm_a133_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
[ ... ]
> +	/*
> +	 * PWM_A133_PCCR01_REG/PCCR23_REG (shared by a channel pair) and
> +	 * PWM_A133_PCGR_REG/PWM_A133_PER_REG (shared by all 16 channels) are
> +	 * each read-modify-written below; the PWM core doesn't serialize
[ ... ]
> +	mutex_lock(&pc->lock);

[Severity: High]
Does taking this mutex pose a sleep-in-atomic hazard?

This PWM apply hook is reachable from the panel enable hook via a backlight 
update. Since the panel enable hook runs from the encoder atomic enable 
context where sleeping is prohibited, acquiring a sleeping mutex here violates 
subsystem constraints.

[ ... ]
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
[ ... ]
> +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]
Does this disable_irq() call violate the DRM atomic context rules?

The function is called directly by sun6i_dsi_encoder_disable(), which is an 
encoder atomic callback. Although the comment states it runs in process 
context, the DRM guidelines classify encoder atomic callbacks as atomic 
contexts where sleeping functions are forbidden.

[ ... ]
> @@ -1117,14 +1117,76 @@ static irqreturn_t sun4i_tcon_handler(int irq, void *private)
[ ... ]
> +	/*
> +	 * Acknowledge the interrupt. TRI_COUNTER_INT is included even though
> +	 * nothing here acts on it: in DSI/CPU mode every firing is treated as
> +	 * handled unconditionally (see the comment above), so if this bit
> +	 * were ever the one actually driving the level line and went
> +	 * unacknowledged, the GIC would keep re-presenting it forever with
> +	 * nothing in this function able to break out -- a genuine hard
> +	 * lockup, not just wasted CPU time. Never observed asserting in any
> +	 * testing so far, but clearing it here is free and closes the gap.
> +	 */
> +	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_COUNTER_INT |
> +			   SUN4I_TCON_GINT0_TCON0_FSYNC_INT,
> +			   0);

[Severity: High]
This is a pre-existing issue, but could this write-0-to-clear sequence 
drop hardware interrupts?

SUN4I_TCON_GINT0_REG is a write-0-to-clear register. The regmap_update_bits() 
call reads the register and unconditionally writes 0 to the specified mask.

If an interrupt such as TRI_FINISH_INT asserts after the initial regmap_read() 
but before the regmap_update_bits() write, the unconditional 0-write clears 
it. Since it was not in the original status snapshot, the handler will not 
process it, resulting in a lost interrupt.

With the high frequency of FSYNC_INT events described in the commit, could 
this TOCTOU race window lead to a permanent display freeze when TRI_FINISH_INT 
is lost?

[ ... ]
> +static enum hrtimer_restart sun4i_tcon_dsi_retrigger_timer_fn(struct hrtimer *timer)
> +{
[ ... ]
> +	regmap_read(tcon->regs, SUN4I_TCON0_CPU_IF_REG, &cpu_if_before);
> +	busy = cpu_if_before & SUN4I_TCON0_CPU_IF_TRI_START;
> +
> +	if (tick && !busy) {
> +		/* TRIGGER_START self-cleared; the IRQ path takes over from here. */
> +		return HRTIMER_NORESTART;
> +	}

[Severity: Medium]
Will this retrigger timer ever actually stop?

The timer polls every 2ms and expects to stop when 
SUN4I_TCON0_CPU_IF_TRI_START is cleared (!busy).

However, when the hardware clears TRI_START at the end of a frame, it 
immediately asserts TRI_FINISH_INT. The hardirq handler runs and immediately 
re-asserts TRI_START to 1. 

Because the timer runs in a softirq context, it executes after the hardirq. 
Thus, the timer will almost always sample TRI_START as 1, miss the window 
where it was 0, and continue running continuously as a permanent CPU hog.

[ ... ]
> +	if (tick == 0 && tcon->irq && !tcon->irq_enabled) {
> +		enable_irq(tcon->irq);
> +		tcon->irq_enabled = true;
> +	}
> +
> +	/*
> +	 * The vendor per-frame retrigger is LINE-edge-triggered and
> +	 * busy-gated, never blindly re-asserting TRIGGER_START -- matching
> +	 * the documented precondition on this bit ("software must write 1
> +	 * only when this flag is 0"). Only the initial kick is fired from
> +	 * software here; TCON0's AUTO bit (set in mode_set_cpu()) is
> +	 * expected to handle Vsync-driven retriggering afterward, the same
> +	 * way mainline's other DSI/CPU-interface boards need no per-frame
> +	 * software retrigger at all.
> +	 */
> +	if (tick == 0 && !busy) {
> +		sun6i_dsi_tri_start(tcon->dsi);
> +		regmap_update_bits(tcon->regs, SUN4I_TCON0_CPU_IF_REG,
> +				   SUN4I_TCON0_CPU_IF_TRI_START,
> +				   SUN4I_TCON0_CPU_IF_TRI_START);
> +	}

[Severity: High]
Could enabling the IRQ here without first clearing pending device-level 
interrupts cause overlapping transfers?

When the display is re-enabled during a DPMS unblank cycle, this function 
calls enable_irq() at tick == 0. If TRI_FINISH_INT or FSYNC_INT is pending 
from a previous session, the hardirq handler will run immediately, preempting 
the timer (or running concurrently), and issue a transfer via 
sun6i_dsi_tri_start().

When enable_irq() returns, the timer proceeds, observes busy == false, and 
also issues a transfer. Could this result in two concurrent transfer commands 
while a transfer is already in flight, confusing the DSI engine state?

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