[PATCH v2 0/3] fbdev: pxafb: fix teardown and early-interrupt lifetimes

[PATCH v2 0/3] fbdev: pxafb: fix teardown and early-interrupt lifetimes

[Matthias Goergens]

Three lifetime fixes for problems Sashiko, the AI review bot, pointed
out while reviewing a pxafb cleanup [1].  Removal leaves the cpufreq
notifier registered on freed memory and the smart-panel thread running,
and the interrupt handler can signal completions before probe has
initialised them.

v2: v1 went out before it was ready, as separate patches without my
Signed-off-by or Fixes tags, and the completion patch depended on the
others without saying so; sorry about that.  This version adds the tags,
sends the patches as a series, and says in each commit message what goes
wrong and how it is reached.  The notifier fix is now a patch of its
own, and the smart-panel patch stops the thread after the controller is
disabled and no longer changes the thread's freezer loop.  Patch 3 is
unchanged.

I have dropped "fbdev: pxafb: preserve IRQ probe errors".  On the
in-tree PXA platforms the interrupt controller is registered from
init_IRQ(), before any driver probes, so I could not find a way for
platform_get_irq() to return -EPROBE_DEFER here, and the patch changed
nothing users would see.

[1] https://lore.kernel.org/all/20260722081427.413EF1F000E9@smtp.kernel.org/

v1 of patches 1 and 2 ("stop smart-panel thread and notifier"):
https://lore.kernel.org/all/20261031033248.2491098-1-matthias.goergens@gmail.com/
v1 of patch 3:
https://lore.kernel.org/all/20261031003424.1796478-1-matthias.goergens@gmail.com/
v1 of the dropped patch:
https://lore.kernel.org/all/20261031033236.2490968-1-matthias.goergens@gmail.com/

Thanks,
Matthias

Matthias Goergens (3):
  fbdev: pxafb: unregister cpufreq notifier on removal
  fbdev: pxafb: stop the smart-panel thread
  fbdev: pxafb: initialize IRQ completions before requesting IRQ

 drivers/video/fbdev/pxafb.c | 46 +++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 10 deletions(-)


base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.56.0

[PATCH v2 1/3] fbdev: pxafb: unregister cpufreq notifier on removal

[Matthias Goergens]

pxafb_probe() registers a cpufreq transition notifier, but
pxafb_remove() never unregisters it.  The notifier block is part of
struct pxafb_info, which is devm-allocated and freed after removal, so
once the driver is unbound or the module unloaded, the next CPU
frequency change calls pxafb_freq_transition() through freed memory.

Unregister the notifier in pxafb_remove().

Sashiko, the AI review bot, pointed this out while reviewing a pxafb
cleanup.  I have no PXA hardware; this is build-tested only.

Link: https://lore.kernel.org/all/20260722081427.413EF1F000E9@smtp.kernel.org/
Fixes: 9f17f2874834 ("[ARM] 5118/1: pxafb: add exit and remove handlers")
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 drivers/video/fbdev/pxafb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 1682345fb9b0..4f5c408eae56 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2384,6 +2384,10 @@ static void pxafb_remove(struct platform_device *dev)
 
 	info = &fbi->fb;
 
+#ifdef CONFIG_CPU_FREQ
+	cpufreq_unregister_notifier(&fbi->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+#endif
 	pxafb_overlay_exit(fbi);
 	cancel_work_sync(&fbi->task);
 	unregister_framebuffer(info);
-- 
2.56.0

[PATCH v2 2/3] fbdev: pxafb: stop the smart-panel thread

[Matthias Goergens]

pxafb_smart_init() starts the smart-panel refresh thread, but nothing
ever stops it: neither pxafb_remove() nor the probe error paths that
run after it.  The thread then keeps running on the devm-allocated
struct pxafb_info after it has been freed.

Stop the thread on removal, after pxafb_disable_controller(), which
waits for a refresh that only the thread signals, and on those probe
error paths.

A thread that has already exited must not be passed to kthread_stop()
unless the caller holds a reference to its task_struct (see the comment
above kthread_stop()).  This thread returns at once when the board
provides no smart_update callback, so check for the callback in
pxafb_smart_init() before starting the thread.  Probe now fails with
-EINVAL in that case, instead of succeeding with a smart panel that is
never refreshed.

This needs CONFIG_FB_PXA_SMARTPANEL and a board with a smart panel; I
found no in-tree board that provides smart_update.  Sashiko, the AI
review bot, pointed this out while reviewing a pxafb cleanup.  I have
no PXA hardware; this is build-tested only.

Link: https://lore.kernel.org/all/20260722081427.413EF1F000E9@smtp.kernel.org/
Fixes: 3c42a449107b ("pxafb: preliminary smart panel interface support")
Fixes: 9f17f2874834 ("[ARM] 5118/1: pxafb: add exit and remove handlers")
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 drivers/video/fbdev/pxafb.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 4f5c408eae56..b889a17e7304 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -1248,12 +1248,6 @@ static int pxafb_smart_thread(void *arg)
 	struct pxafb_info *fbi = arg;
 	struct pxafb_mach_info *inf = fbi->inf;
 
-	if (!inf->smart_update) {
-		pr_err("%s: not properly initialized, thread terminated\n",
-				__func__);
-		return -EINVAL;
-	}
-
 	pr_debug("%s(): task starting\n", __func__);
 
 	set_freezable();
@@ -1290,6 +1284,9 @@ static int pxafb_smart_init(struct pxafb_info *fbi)
 	init_completion(&fbi->command_done);
 	init_completion(&fbi->refresh_done);
 
+	if (!fbi->inf->smart_update)
+		return -EINVAL;
+
 	fbi->smart_thread = kthread_run(pxafb_smart_thread, fbi,
 					"lcd_refresh");
 	if (IS_ERR(fbi->smart_thread)) {
@@ -1299,8 +1296,15 @@ static int pxafb_smart_init(struct pxafb_info *fbi)
 
 	return 0;
 }
+
+static void pxafb_smart_exit(struct pxafb_info *fbi)
+{
+	if (!IS_ERR_OR_NULL(fbi->smart_thread))
+		kthread_stop(fbi->smart_thread);
+}
 #else
 static inline int pxafb_smart_init(struct pxafb_info *fbi) { return 0; }
+static inline void pxafb_smart_exit(struct pxafb_info *fbi) { }
 #endif /* CONFIG_FB_PXA_SMARTPANEL */
 
 static void setup_parallel_timing(struct pxafb_info *fbi,
@@ -2366,6 +2370,7 @@ static int pxafb_probe(struct platform_device *dev)
 	if (fbi->fb.cmap.len)
 		fb_dealloc_cmap(&fbi->fb.cmap);
 failed_free_mem:
+	pxafb_smart_exit(fbi);
 	free_pages_exact(fbi->video_mem, fbi->video_mem_size);
 failed_free_dma:
 	dma_free_coherent(&dev->dev, fbi->dma_buff_size,
@@ -2393,6 +2398,7 @@ static void pxafb_remove(struct platform_device *dev)
 	unregister_framebuffer(info);
 
 	pxafb_disable_controller(fbi);
+	pxafb_smart_exit(fbi);
 
 	if (fbi->fb.cmap.len)
 		fb_dealloc_cmap(&fbi->fb.cmap);
-- 
2.56.0

[PATCH v2 3/3] fbdev: pxafb: initialize IRQ completions before requesting IRQ

[Matthias Goergens]

pxafb_handle_irq() completes command_done when LCSR has LCSR_CMD_INT
set, and an overlay's branch_done when LCSR1 has its branch status bit
set.  Probe requests the IRQ before it initialises either: command_done
in pxafb_smart_init(), where commit 07df1c4fea14 ("[ARM] pxafb: small
cleanup of the smart panel code") moved it, and branch_done in the
overlay setup, since the overlays were added.  If one of those status
bits is already set when the handler is installed, for example because
the bootloader left a smart panel or an overlay running, complete()
runs on an uninitialised completion.

Initialise all completions in a helper called from pxafb_init_fbinfo(),
before the IRQ is requested, and drop the later initialisation.

With the completions initialised early, such an interrupt instead
leaves a completion count with no waiter, which would let a later wait
return before its transaction has finished.  So reinitialise
command_done and branch_done just before starting the transactions they
wait for.

I have no PXA hardware.  In a test build on ARM QEMU that raises the
LCD interrupt early through the real handler, the unpatched driver
crashes in complete() and the patched one does not.  After an early
command interrupt, a later smart-panel flush returned success at once
without the reinitialisation, and timed out with it, as it should when
no further interrupt arrives.

Sashiko, the AI review bot, pointed out the early interrupt while
reviewing a pxafb cleanup.

Link: https://lore.kernel.org/all/20260722081427.413EF1F000E9@smtp.kernel.org/
Fixes: 07df1c4fea14 ("[ARM] pxafb: small cleanup of the smart panel code")
Fixes: 198fc108ee4c ("[ARM] pxafb: add support for overlay1 and overlay2 as framebuffer devices")
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 drivers/video/fbdev/pxafb.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index b889a17e7304..afdcb14f4b42 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -642,6 +642,7 @@ static void overlay1fb_disable(struct pxafb_layer *ofb)
 
 	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
 	lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(1));
+	reinit_completion(&ofb->branch_done);
 	lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
 
 	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
@@ -705,6 +706,7 @@ static void overlay2fb_disable(struct pxafb_layer *ofb)
 
 	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));
 	lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(2));
+	reinit_completion(&ofb->branch_done);
 	lcd_writel(ofb->fbi, FBR2, ofb->fbi->fdadr[DMA_OV2_Y]  | 0x3);
 	lcd_writel(ofb->fbi, FBR3, ofb->fbi->fdadr[DMA_OV2_Cb] | 0x3);
 	lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
@@ -894,7 +896,6 @@ static void init_pxafb_overlay(struct pxafb_info *fbi, struct pxafb_layer *ofb,
 	ofb->ops = &ofb_ops[id];
 	ofb->usage = 0;
 	ofb->fbi = fbi;
-	init_completion(&ofb->branch_done);
 }
 
 static inline int pxafb_overlay_supported(void)
@@ -1170,6 +1171,7 @@ int pxafb_smart_flush(struct fb_info *info)
 	lcd_writel(fbi, FDADR6, fbi->fdadr[6]);
 
 	/* begin sending */
+	reinit_completion(&fbi->command_done);
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 | LCCR0_ENB);
 
 	if (wait_for_completion_timeout(&fbi->command_done, HZ/2) == 0) {
@@ -1281,9 +1283,6 @@ static int pxafb_smart_init(struct pxafb_info *fbi)
 	fbi->smart_cmds = (uint16_t *) fbi->dma_buff->cmd_buff;
 	fbi->n_smart_cmds = 0;
 
-	init_completion(&fbi->command_done);
-	init_completion(&fbi->refresh_done);
-
 	if (!fbi->inf->smart_update)
 		return -EINVAL;
 
@@ -1779,6 +1778,23 @@ static void pxafb_decode_mach_info(struct pxafb_info *fbi,
 		fbi->video_mem_size = video_mem_size;
 }
 
+static void pxafb_init_completions(struct pxafb_info *fbi)
+{
+#ifdef CONFIG_FB_PXA_OVERLAY
+	int i;
+#endif
+
+	init_completion(&fbi->disable_done);
+#ifdef CONFIG_FB_PXA_SMARTPANEL
+	init_completion(&fbi->command_done);
+	init_completion(&fbi->refresh_done);
+#endif
+#ifdef CONFIG_FB_PXA_OVERLAY
+	for (i = 0; i < 2; i++)
+		init_completion(&fbi->overlay[i].branch_done);
+#endif
+}
+
 static struct pxafb_info *pxafb_init_fbinfo(struct device *dev,
 					    struct pxafb_mach_info *inf)
 {
@@ -1835,7 +1851,7 @@ static struct pxafb_info *pxafb_init_fbinfo(struct device *dev,
 	init_waitqueue_head(&fbi->ctrlr_wait);
 	INIT_WORK(&fbi->task, pxafb_task);
 	mutex_init(&fbi->ctrlr_lock);
-	init_completion(&fbi->disable_done);
+	pxafb_init_completions(fbi);
 
 	return fbi;
 }
-- 
2.56.0