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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Sleeping functions used in CRTC and encoder atomic callbacks (Subsystem Guide Violation)
- [High] Vblank interrupts are permanently disabled for non-DSI boards after DPMS cycle
- [High] hrtimer callback attempts to acquire sleepable spinlock on PREEMPT_RT
- [High] TCON interrupt storm causes severe CPU hog by unconditionally returning IRQ_HANDLED
- [Medium] CLK_IS_CRITICAL on tcon-top-dsi gate causes power management regression
- [Low] Missing EXPORT_SYMBOL for sun6i_mipi_dsi functions causes module linkage failure
--

commit 5d94a8dca17c72188e51f01e2e6fb28930a6a4da
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 includes a new board DTS, a new OTM1289A/ER68576 DSI panel driver,
a new PWM driver for the A133, and display pipeline support.

> 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 int otm1289a_init_sequence_send(struct otm1289a *ctx)
> +{
[ ... ]
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0)
> +		return ret;
> +	msleep(50);

[Severity: High]
Does this msleep() introduce a sleep in atomic context? The panel
initialization is invoked from the encoder atomic enable callback, which is an
atomic context where sleeping is forbidden. This can lead to kernel panics and
deadlocks during display atomic commits.

> 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
[ ... ]
> @@ -221,6 +221,34 @@ void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
[ ... ]
> +		if (tcon->irq_enabled) {
> +			disable_irq(tcon->irq);

[Severity: High]
Could this disable_irq() call cause issues by sleeping in atomic context? This
is executed within the CRTC atomic disable path, which shouldn't contain
sleepable functions.

[ ... ]
> @@ -713,11 +948,31 @@ void sun4i_tcon_mode_set(struct sun4i_tcon *tcon,
[ ... ]
> +	if (!tcon->dsi_cpu_mode && tcon->irq && !tcon->irq_enabled) {
> +		enable_irq(tcon->irq);
> +		tcon->irq_enabled = true;
> +	}
>  }

[Severity: High]
Will vblank interrupts stay permanently disabled for non-DSI boards after a
DPMS off/on cycle? Because standard DPMS unblanking doesn't invoke the
mode_set_nofb hook, placing the IRQ enable logic here means it is skipped
during DPMS cycles, leaving the IRQ disabled.

[ ... ]
> @@ -759,30 +1030,173 @@ static irqreturn_t sun4i_tcon_handler(int irq, void *private)
[ ... ]
> +	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]
Does unconditionally returning IRQ_HANDLED for DSI/CPU mode leave the system
vulnerable to an unmitigated interrupt storm? By defeating genirq's
protection, the FSYNC_INT storm could cause a severe CPU hog. The DSI block's
own interrupt (SPI 100) might be the safer hardware approach here.

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

[Severity: High]
Can this regmap_read() cause a kernel panic on PREEMPT_RT kernels? It attempts
to acquire a sleepable spinlock on an MMIO regmap, but the hrtimer is
initialized with HRTIMER_MODE_REL (hardirq context).

[ ... ]
> @@ -1136,6 +1562,9 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
[ ... ]
> +	hrtimer_setup(&tcon->dsi_retrigger_timer, sun4i_tcon_dsi_retrigger_timer_fn,
> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL);

[Severity: High]
Should this timer be configured as HRTIMER_MODE_REL_SOFT instead, so that its
callback can safely execute sleepable functions in softirq context on
PREEMPT_RT?

> 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
[ ... ]
> +void sun6i_dsi_tri_start(struct sun6i_dsi *dsi)
> +{
> +	sun6i_dsi_start(dsi, DSI_START_TRI);
> +}

[Severity: Low]
Is this function missing an EXPORT_SYMBOL macro? Without it, modular builds of
sun4i-tcon.ko depending on this function will fail at the modpost stage.

[ ... ]
> +u32 sun6i_dsi_read_int_status(struct sun6i_dsi *dsi)
> +{
> +	unsigned int val = 0;
> +
> +	regmap_read(dsi->regs, SUN6I_DSI_INT_REG, &val);
> +
> +	return val;
> +}

[Severity: Low]
Does this function also need an EXPORT_SYMBOL macro to prevent module linkage
failures?

[ ... ]
> +void sun6i_dsi_clear_int_status(struct sun6i_dsi *dsi)
> +{
> +	unsigned int val = 0;
> +
> +	regmap_read(dsi->regs, SUN6I_DSI_INT_REG, &val);
> +	regmap_write_bits(dsi->regs, SUN6I_DSI_INT_REG,
> +			  GENMASK(31, 16), val & GENMASK(31, 16));
> +}

[Severity: Low]
Is an EXPORT_SYMBOL macro required here as well for modular builds?

> 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
[ ... ]
> @@ -241,19 +315,33 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
[ ... ]
>  	if (quirks->has_dsi)
>  		clk_data->hws[CLK_TCON_TOP_DSI] =
>  			sun8i_tcon_top_register_gate(dev, "dsi", regs,
>  						     &tcon_top->reg_lock,
> -						     TCON_TOP_TCON_DSI_GATE, i++);
> +						     TCON_TOP_TCON_DSI_GATE, i++,
> +						     CLK_IS_CRITICAL);

[Severity: Medium]
Will using CLK_IS_CRITICAL on this gate cause a power management regression?
This forces the clock to remain permanently enabled, holding references to its
parent (CLK_MIPI_DSI) and preventing the PLL from ever powering down.
CLK_IGNORE_UNUSED might be more appropriate to prevent the clock core's
disable_unused sweep.

> 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)
> +{
[ ... ]
> +	mutex_lock(&pc->lock);

[Severity: High]
Does this use a sleeping mutex inside an atomic context? The PWM apply hook
is reached via backlight_enable() from the encoder atomic enable callback,
which is an atomic context where sleeping is forbidden. This can lead to
kernel panics and deadlocks.

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