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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Sleeping functions called from atomic encoder callbacks, violating display subsystem rules.
- [High] TOCTOU race condition causing concurrent DSI retriggering during IRQ re-enable.
- [High] Integer underflow when computing `start_delay` with adversarial or edge-case DRM display modes.
- [Medium] Resource leak in PWM driver: channel clock gate left permanently enabled on disable.
- [Low] The 'allwinner,sun50i-a133-tcon-top' compatible string breaks device tree binding schema validation.

Pre-existing issues:
- [High] Use-After-Free (TOCTOU) in TCON TOP register access due to lack of synchronization with device unbind.
- [High] Division by zero if the hardware DCLK divider register reads as zero.
--

commit 5601bade2c59d4b26ab64fee73a2b3036bf1fb38
Author: leow149 <leow149@lucidmail.xyz>

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

This commit adds board support for the TrimUI Smart Pro handheld. It
introduces new DTS configurations, a panel driver for the OTM1289A/ER68576,
a PWM driver, and modifies TCON/DSI code to support the A133 SoC's display
and PWM pipelines.

> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml b/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml
> --- a/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml
> +++ b/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml
> @@ -43,6 +43,7 @@ properties:
>        - allwinner,sun8i-r40-tcon-top
>        - allwinner,sun20i-d1-tcon-top
>        - allwinner,sun50i-h6-tcon-top
> +      - allwinner,sun50i-a133-tcon-top

[Severity: Low]
Does adding the A133 compatible here break dtbs_check validation for valid
device trees?

The A133 hardware lacks an HDMI mux and only uses port@0 and port@1. However,
the base schema unconditionally requires port@4 and port@5 across all
compatibles. This would cause a correct A133 device tree to be rejected
as invalid.

> 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
> @@ -0,0 +1,424 @@
[ ... ]
> +static int otm1289a_prepare(struct drm_panel *panel)
> +{
> +	struct otm1289a *ctx = panel_to_otm1289a(panel);
> +	int ret;
> +
> +	ret = regulator_enable(ctx->supply);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to enable supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	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]
Can these msleep() calls execute in an atomic context?

If otm1289a_prepare() is invoked from an encoder's atomic enable callback
path (such as sun6i_dsi_encoder_enable), explicit sleeps are forbidden
and can cause deadlocks or kernel warnings.

> 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
> @@ -395,6 +608,18 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
>  	regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
>  	tcon_div &= GENMASK(6, 0);
>  	block_space = mode->htotal * bpp / (tcon_div * lanes);

[Severity: High]
This is a pre-existing issue, but can tcon_div evaluate to 0 and cause a
division-by-zero panic here?

If the clock framework fails to configure the rate in clk_set_rate() earlier
in the function, SUN4I_TCON0_DCLK_REG might retain its reset value of 0.
The subsequent calculation would then divide by zero.

[ ... ]
> +	if (tcon->quirks->dsi_cpu_needs_retrigger) {
> +		start_delay = (mode->crtc_vtotal - mode->crtc_vdisplay - 9);
> +		start_delay = start_delay * mode->crtc_htotal * 300;

[Severity: High]
Is it possible for this subtraction to underflow if a custom DRM mode is
supplied?

If userspace supplies an edge-case DRM mode where mode->crtc_vtotal is less
than mode->crtc_vdisplay + 9, the subtraction evaluates to a negative number.
When assigned to the unsigned start_delay variable, this wraps around to a
massive positive value, which would corrupt SUN4I_TCON0_CPU_TRI2_REG and
break display timings.

[ ... ]
> +static enum hrtimer_restart sun4i_tcon_dsi_retrigger_timer_fn(struct hrtimer *timer)
> +{
> +	struct sun4i_tcon *tcon = container_of(timer, struct sun4i_tcon,
> +						dsi_retrigger_timer);
> +	unsigned int cpu_if_before;
> +	unsigned int tick = tcon->dsi_retrigger_ticks;
> +	u32 dsi_int;
> +	bool busy;
> +
> +	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;
> +	}
> +
> +	dsi_int = sun6i_dsi_read_int_status(tcon->dsi);
> +	if (dsi_int & GENMASK(31, 16))
> +		sun6i_dsi_clear_int_status(tcon->dsi);
> +
> +	if (tick == 0 && tcon->irq && !tcon->irq_enabled) {
> +		enable_irq(tcon->irq);
> +		tcon->irq_enabled = true;
> +	}
> +
> +	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]
Can a race condition occur here if the interrupt line is unmasked?

The busy state is cached before calling enable_irq(tcon->irq). Because
SUN4I_TCON_GINT0_REG retains pending interrupts, enable_irq() can unmask the
line and cause the hardirq to fire immediately, preempting this softirq.
The hardirq could kick the DSI transfer and set TRI_START.

When the softirq resumes, it uses the cached !busy value to kick the transfer
a second time, writing 1 to TRI_START when it is already 1. Does this
violate the hardware state machine?

> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -720,7 +836,6 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	struct mipi_dsi_device *device = dsi->device;
>  	union phy_configure_opts opts = { };
>  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;

[Severity: High]
Does executing this function in an atomic context violate DRM subsystem rules?

The DRM subsystem defines encoder atomic enable and disable callbacks as
atomic contexts where sleeping is forbidden. Functions like
clk_prepare_enable() are used inside this path, which can sleep and lead to
kernel warnings or potential deadlocks.

[ ... ]
> +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);

[Severity: High]
Does calling sun4i_tcon_dsi_stop_retrigger() here trigger a sleep in an
atomic context?

This path eventually calls disable_irq(), which is a sleeping function. Since
encoder disable callbacks run in atomic context during non-blocking commits,
this could cause system instability or deadlocks.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> @@ -143,12 +143,115 @@ int sun8i_tcon_top_de_config(struct device *dev, int mixer, int tcon)
>  }
>  EXPORT_SYMBOL(sun8i_tcon_top_de_config);
>  
> +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 is a pre-existing issue, but does this pattern leave us vulnerable to a
Use-After-Free?

An atomic commit worker can retrieve tcon_top via dev_get_drvdata(dev).
Concurrently, a sysfs unbind could finish its remove callback and free the
memory backing tcon_top.

Since the !tcon_top check occurs before the lock is acquired, the memory
could be freed immediately after the check but before spin_lock_irqsave(),
leading to a dereference of freed memory.

[ ... ]
> +int sun8i_tcon_top_set_de0_port(struct device *dev, int port)
> +{
> +	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 is a pre-existing issue, but is this unprotected access pattern also
susceptible to the same Use-After-Free race mentioned above?

> 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
> @@ -0,0 +1,322 @@
[ ... ]
> +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);
> +		val = pwm_a133_readl(pc, PWM_A133_PER_REG);
> +		val &= ~PWM_A133_PER_EN(ch);
> +		pwm_a133_writel(pc, PWM_A133_PER_REG, val);
> +		mutex_unlock(&pc->lock);
> +		return 0;
> +	}

[Severity: Medium]
Does returning early here leak the channel clock gate?

By returning 0 immediately, the code skips the PWM_A133_PCGR_GATING clear at
the end of the function, leaving the channel clock gate permanently enabled
after the PWM channel is disabled.

[ ... ]
> +	mutex_lock(&pc->lock);

[Severity: High]
Does acquiring this mutex violate atomic context rules when called from the
backlight driver?

If the backlight update is triggered during the panel prepare phase from an
encoder's atomic enable callback, calling mutex_lock() could sleep and lead
to system instability.

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