Re: [PATCH 2/7] staging: fbtft: Remove redundant dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver relies on static global variables (`irq_te` and `panel_te`) to manage device-specific IRQ and synchronization state, which breaks multi-device configurations and can cause Use-After-Free bugs.
- [Medium] In the display update path `write_vmem()`, the tearing effect (TE) interrupt is enabled before the completion structure is reinitialized, creating a race window that can lead to missed TE events and unnecessary timeouts.
- [Medium] The driver prematurely frees the `te` GPIO descriptor via `gpiod_put(te)` immediately after converting it to an IRQ, allowing the pin to be reconfigured or powered down.
--

--- Patch [2]: [PATCH 2/7] staging: fbtft: Remove redundant dev_err_probe() ---
commit ac4d01370b4ee0a3ff08280bd824b5b8e0c0efe6
Author: Pan Chuang <panchuang@vivo.com>

staging: fbtft: Remove redundant dev_err_probe()
    
Since commit
55b48e23f5c4 ("genirq/devres: Add error handling in devm_request_*_irq()"),
devm_request_irq() automatically logs detailed error messages on
failure. Remove the now-redundant driver-specific dev_err_probe() calls.

> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 861a154144e66..ec46ad3f6a918 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -119,7 +119,7 @@ static int init_tearing_effect_line(struct fbtft_par *par)
>  	rc = devm_request_irq(dev, irq_te, panel_te_handler,
>  			      IRQF_TRIGGER_RISING, "TE_GPIO", par);

[Severity: High]
This isn't a bug introduced by this patch, but does using the global
variables irq_te and panel_te break multi-device configurations?

If multiple displays are probed, the global irq_te will be overwritten.
Subsequent display updates from any panel will then manipulate the irq of
the most recently probed panel.

If one panel is unbound, devres frees its irq, but irq_te retains the freed
irq number, potentially leading to an invalid enable_irq() call on a
released irq.

>  	if (rc)
> -		return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
> +		return rc;
>  
>  	disable_irq_nosync(irq_te);
>  

[Severity: Medium]
This is a pre-existing issue, but does the driver prematurely free the te
gpio descriptor in init_tearing_effect_line() just before this?

drivers/staging/fbtft/fb_st7789v.c:init_tearing_effect_line() {
...
	irq = gpiod_to_irq(te);

	/* GPIO is locked as an IRQ, we may drop the reference */
	gpiod_put(te);

	if (irq < 0)
		return irq;
...
}

Calling gpiod_put() completely frees the descriptor and marks it as
unrequested in the gpiolib core. Doesn't this allow other drivers or
userspace tools to request the gpio and reconfigure its direction, which
would silently break the te interrupt line?

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a race condition in
write_vmem() when enabling the tearing effect interrupt?

drivers/staging/fbtft/fb_st7789v.c:write_vmem() {
...
	if (irq_te) {
		enable_irq(irq_te);
		reinit_completion(&panel_te);
...
}

By enabling the irq before resetting the completion, could an interrupt
arrive in this tiny window?

If it does, the handler completes the event, but the main thread
immediately resets the completion state and waits, missing the event.
Could this regression lead to unnecessary timeouts and framerate drops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720134324.239391-1-panchuang@vivo.com?part=2