linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/5] 8250_omap fixes for testing
@ 2022-09-28  7:29 Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 1/5] serial: 8250: omap: Fix missing PM runtime calls for omap8250_set_mctrl() Tony Lindgren
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Ivaylo Dimitrov, Merlijn Wajer,
	Romain Naour

Hi all,

Here are some 8250_omap fixes for testing. I'm not sure if the first patch
fixes the issue reported. I'm not sure if I've seen that one, so please test.

The other patches are for all kind of issues I started running into after
testing rebinding the driver.

These are tagged RFC as we're close to the merge window, I'll repost the
series probably after -rc1 after folks have tested this a bit. It seems
that all these issues have been around for quite a long time.

Regards,

Tony


Tony Lindgren (5):
  serial: 8250: omap: Fix missing PM runtime calls for
    omap8250_set_mctrl()
  serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in
    omap8250_remove()
  serial: 8250: omap: Flush PM QOS work on remove
  serial: 8250: omap: Fix imprecise external abort for omap_8250_pm()
  serial: 8250: omap: Fix life cycle issues for interrupt handlers

 drivers/tty/serial/8250/8250_omap.c | 140 +++++++++++++++++-----------
 1 file changed, 84 insertions(+), 56 deletions(-)

-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 1/5] serial: 8250: omap: Fix missing PM runtime calls for omap8250_set_mctrl()
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
@ 2022-09-28  7:29 ` Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 2/5] serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in omap8250_remove() Tony Lindgren
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Merlijn Wajer, Romain Naour,
	Ivaylo Dimitrov

There are cases where omap8250_set_mctrl() may get called after the
UART has already autoidled causing an asynchronous external abort.

This can happen on ttyport_open():

mem_serial_in from omap8250_set_mctrl+0x38/0xa0
omap8250_set_mctrl from uart_update_mctrl+0x4c/0x58
uart_update_mctrl from uart_dtr_rts+0x60/0xa8
uart_dtr_rts from tty_port_block_til_ready+0xd0/0x2a8
tty_port_block_til_ready from uart_open+0x14/0x1c
uart_open from ttyport_open+0x64/0x148

And on ttyport_close():

omap8250_set_mctrl from uart_update_mctrl+0x3c/0x48
uart_update_mctrl from uart_dtr_rts+0x54/0x9c
uart_dtr_rts from tty_port_shutdown+0x78/0x9c
tty_port_shutdown from tty_port_close+0x3c/0x74
tty_port_close from ttyport_close+0x40/0x58

It can also happen on disassociate_ctty() calling uart_shutdown()
that ends up calling omap8250_set_mctrl().

Let's fix the issue by adding missing PM runtime calls to
omap8250_set_mctrl(). To do this, we need to add __omap8250_set_mctrl()
that can be called from both omap8250_set_mctrl(), and from runtime PM
resume path when restoring the registers.

Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
Depends-on: dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
Reported-by: Merlijn Wajer <merlijn@wizzup.org>
Reported-by: Romain Naour <romain.naour@smile.fr>
Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -157,7 +157,11 @@ static u32 uart_read(struct uart_8250_port *up, u32 reg)
 	return readl(up->port.membase + (reg << up->port.regshift));
 }
 
-static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+/*
+ * Called on runtime PM resume path from omap8250_restore_regs(), and
+ * omap8250_set_mctrl().
+ */
+static void __omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct omap8250_priv *priv = up->port.private_data;
@@ -181,6 +185,20 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	}
 }
 
+static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+	int err;
+
+	err = pm_runtime_resume_and_get(port->dev);
+	if (err)
+		return;
+
+	__omap8250_set_mctrl(port, mctrl);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+}
+
 /*
  * Work Around for Errata i202 (2430, 3430, 3630, 4430 and 4460)
  * The access to uart register after MDR1 Access
@@ -341,7 +359,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 
 	omap8250_update_mdr1(up, priv);
 
-	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
+	__omap8250_set_mctrl(&up->port, up->port.mctrl);
 }
 
 /*
-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 2/5] serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in omap8250_remove()
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 1/5] serial: 8250: omap: Fix missing PM runtime calls for omap8250_set_mctrl() Tony Lindgren
@ 2022-09-28  7:29 ` Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 3/5] serial: 8250: omap: Flush PM QOS work on remove Tony Lindgren
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Ivaylo Dimitrov, Merlijn Wajer,
	Romain Naour

On remove, we get an error for "Runtime PM usage count underflow!". I guess
this driver is mostly built-in, and this issue has gone unnoticed for a
while. Somehow I did not catch this issue with my earlier fix done with
commit 4e0f5cc65098 ("serial: 8250_omap: Fix probe and remove for PM
runtime").

Fixes: 4e0f5cc65098 ("serial: 8250_omap: Fix probe and remove for PM runtime")
Depends-on: dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1475,6 +1475,11 @@ static int omap8250_probe(struct platform_device *pdev)
 static int omap8250_remove(struct platform_device *pdev)
 {
 	struct omap8250_priv *priv = platform_get_drvdata(pdev);
+	int err;
+
+	err = pm_runtime_resume_and_get(&pdev->dev);
+	if (err)
+		return err;
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 3/5] serial: 8250: omap: Flush PM QOS work on remove
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 1/5] serial: 8250: omap: Fix missing PM runtime calls for omap8250_set_mctrl() Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 2/5] serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in omap8250_remove() Tony Lindgren
@ 2022-09-28  7:29 ` Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 4/5] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Ivaylo Dimitrov, Merlijn Wajer,
	Romain Naour

Rebinding 8250_omap in a loop will at some point produce a warning for
kernel/power/qos.c:296 cpu_latency_qos_update_request() with error
"cpu_latency_qos_update_request called for unknown object". Let's flush
the possibly pending PM QOS work scheduled from omap8250_runtime_suspend()
before we disable runtime PM.

Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1483,6 +1483,7 @@ static int omap8250_remove(struct platform_device *pdev)
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
+	flush_work(&priv->qos_work);
 	pm_runtime_disable(&pdev->dev);
 	serial8250_unregister_port(priv->line);
 	cpu_latency_qos_remove_request(&priv->pm_qos_request);
-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 4/5] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm()
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
                   ` (2 preceding siblings ...)
  2022-09-28  7:29 ` [RFC PATCH 3/5] serial: 8250: omap: Flush PM QOS work on remove Tony Lindgren
@ 2022-09-28  7:29 ` Tony Lindgren
  2022-09-28  7:29 ` [RFC PATCH 5/5] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
  2022-09-28 15:33 ` [RFC PATCH 0/5] 8250_omap fixes for testing Ivaylo Dimitrov
  5 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Ivaylo Dimitrov, Merlijn Wajer,
	Romain Naour

We must idle the uart only after serial8250_unregister_port(). Otherwise
unbinding the uart via sysfs while doing cat on the port produces an
imprecise external abort:

mem_serial_in from omap_8250_pm+0x44/0xf4
omap_8250_pm from uart_hangup+0xe0/0x194
uart_hangup from __tty_hangup.part.0+0x37c/0x3a8
__tty_hangup.part.0 from uart_remove_one_port+0x9c/0x22c
uart_remove_one_port from serial8250_unregister_port+0x60/0xe8
serial8250_unregister_port from omap8250_remove+0x6c/0xd0
omap8250_remove from platform_remove+0x28/0x54

Turns out the driver really needs to have runtime PM functional before
the driver probe calls serial8250_register_8250_port(), and after driver
remove calls serial8250_unregister_port().

On probe, we need to read registers before registering the port in
omap_serial_fill_features_erratas(). We do with custom uart_read()
already.

On remove, after serial8250_unregister_port(), we need to write to the
uart registers to idle the device. Let's add uart_write() for that.

Currently the uart register access depends on port->membase to be
initialized, which won't work after serial8250_unregister_port().
Let's use priv->membase, and use that for runtime PM related functions
to remove the need for port->membase.

We need to check for a valid port for the runtime PM related functions
for the optional wakeup configuration that is not needed for probe
or remove. And we now need to set the drvdata earlier so it's available
for the runtime PM functions.

Note that the checks for missing priv for both omap8250_runtime_suspend()
and omap8250_runtime_resume() are no longer needed and are removed.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 70 ++++++++++++++++-------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -32,6 +32,7 @@
 #include "8250.h"
 
 #define DEFAULT_CLK_SPEED	48000000
+#define OMAP_UART_REGSHIFT	2
 
 #define UART_ERRATA_i202_MDR1_ACCESS	(1 << 0)
 #define OMAP_UART_WER_HAS_TX_WAKEUP	(1 << 1)
@@ -109,6 +110,7 @@
 #define UART_OMAP_RX_LVL		0x19
 
 struct omap8250_priv {
+	void __iomem *membase;
 	int line;
 	u8 habit;
 	u8 mdr1;
@@ -152,9 +154,14 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p);
 static inline void omap_8250_rx_dma_flush(struct uart_8250_port *p) { }
 #endif
 
-static u32 uart_read(struct uart_8250_port *up, u32 reg)
+static u32 uart_read(struct omap8250_priv *priv, u32 reg)
 {
-	return readl(up->port.membase + (reg << up->port.regshift));
+	return readl(priv->membase + (reg << OMAP_UART_REGSHIFT));
+}
+
+static void uart_write(struct omap8250_priv *priv, u32 reg, u32 val)
+{
+	writel(val, priv->membase + (reg << OMAP_UART_REGSHIFT));
 }
 
 /*
@@ -550,7 +557,7 @@ static void omap_serial_fill_features_erratas(struct uart_8250_port *up,
 	u32 mvr, scheme;
 	u16 revision, major, minor;
 
-	mvr = uart_read(up, UART_OMAP_MVER);
+	mvr = uart_read(priv, UART_OMAP_MVER);
 
 	/* Check revision register scheme */
 	scheme = mvr >> OMAP_UART_MVR_SCHEME_SHIFT;
@@ -1331,7 +1338,7 @@ static int omap8250_probe(struct platform_device *pdev)
 		UPF_HARD_FLOW;
 	up.port.private_data = priv;
 
-	up.port.regshift = 2;
+	up.port.regshift = OMAP_UART_REGSHIFT;
 	up.port.fifosize = 64;
 	up.tx_loadsz = 64;
 	up.capabilities = UART_CAP_FIFO;
@@ -1393,6 +1400,8 @@ static int omap8250_probe(struct platform_device *pdev)
 			 DEFAULT_CLK_SPEED);
 	}
 
+	priv->membase = membase;
+	priv->line = -ENODEV;
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	priv->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	cpu_latency_qos_add_request(&priv->pm_qos_request, priv->latency);
@@ -1400,6 +1409,8 @@ static int omap8250_probe(struct platform_device *pdev)
 
 	spin_lock_init(&priv->rx_dma_lock);
 
+	platform_set_drvdata(pdev, priv);
+
 	device_init_wakeup(&pdev->dev, true);
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_use_autosuspend(&pdev->dev);
@@ -1461,7 +1472,6 @@ static int omap8250_probe(struct platform_device *pdev)
 		goto err;
 	}
 	priv->line = ret;
-	platform_set_drvdata(pdev, priv);
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 	return 0;
@@ -1481,11 +1491,12 @@ static int omap8250_remove(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	serial8250_unregister_port(priv->line);
+	priv->line = -ENODEV;
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	flush_work(&priv->qos_work);
 	pm_runtime_disable(&pdev->dev);
-	serial8250_unregister_port(priv->line);
 	cpu_latency_qos_remove_request(&priv->pm_qos_request);
 	device_init_wakeup(&pdev->dev, false);
 	return 0;
@@ -1561,7 +1572,6 @@ static int omap8250_lost_context(struct uart_8250_port *up)
 static int omap8250_soft_reset(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
-	struct uart_8250_port *up = serial8250_get_port(priv->line);
 	int timeout = 100;
 	int sysc;
 	int syss;
@@ -1575,20 +1585,20 @@ static int omap8250_soft_reset(struct device *dev)
 	 * needing omap8250_soft_reset() quirk. Do it in two writes as
 	 * recommended in the comment for omap8250_update_scr().
 	 */
-	serial_out(up, UART_OMAP_SCR, OMAP_UART_SCR_DMAMODE_1);
-	serial_out(up, UART_OMAP_SCR,
+	uart_write(priv, UART_OMAP_SCR, OMAP_UART_SCR_DMAMODE_1);
+	uart_write(priv, UART_OMAP_SCR,
 		   OMAP_UART_SCR_DMAMODE_1 | OMAP_UART_SCR_DMAMODE_CTL);
 
-	sysc = serial_in(up, UART_OMAP_SYSC);
+	sysc = uart_read(priv, UART_OMAP_SYSC);
 
 	/* softreset the UART */
 	sysc |= OMAP_UART_SYSC_SOFTRESET;
-	serial_out(up, UART_OMAP_SYSC, sysc);
+	uart_write(priv, UART_OMAP_SYSC, sysc);
 
 	/* By experiments, 1us enough for reset complete on AM335x */
 	do {
 		udelay(1);
-		syss = serial_in(up, UART_OMAP_SYSS);
+		syss = uart_read(priv, UART_OMAP_SYSS);
 	} while (--timeout && !(syss & OMAP_UART_SYSS_RESETDONE));
 
 	if (!timeout) {
@@ -1602,13 +1612,10 @@ static int omap8250_soft_reset(struct device *dev)
 static int omap8250_runtime_suspend(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
-	struct uart_8250_port *up;
-
-	/* In case runtime-pm tries this before we are setup */
-	if (!priv)
-		return 0;
+	struct uart_8250_port *up = NULL;
 
-	up = serial8250_get_port(priv->line);
+	if (priv->line >= 0)
+		up = serial8250_get_port(priv->line);
 	/*
 	 * When using 'no_console_suspend', the console UART must not be
 	 * suspended. Since driver suspend is managed by runtime suspend,
@@ -1616,7 +1623,7 @@ static int omap8250_runtime_suspend(struct device *dev)
 	 * active during suspend.
 	 */
 	if (priv->is_suspending && !console_suspend_enabled) {
-		if (uart_console(&up->port))
+		if (up && uart_console(&up->port))
 			return -EBUSY;
 	}
 
@@ -1627,13 +1634,15 @@ static int omap8250_runtime_suspend(struct device *dev)
 		if (ret)
 			return ret;
 
-		/* Restore to UART mode after reset (for wakeup) */
-		omap8250_update_mdr1(up, priv);
-		/* Restore wakeup enable register */
-		serial_out(up, UART_OMAP_WER, priv->wer);
+		if (up) {
+			/* Restore to UART mode after reset (for wakeup) */
+			omap8250_update_mdr1(up, priv);
+			/* Restore wakeup enable register */
+			serial_out(up, UART_OMAP_WER, priv->wer);
+		}
 	}
 
-	if (up->dma && up->dma->rxchan)
+	if (up && up->dma && up->dma->rxchan)
 		omap_8250_rx_dma_flush(up);
 
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
@@ -1645,18 +1654,15 @@ static int omap8250_runtime_suspend(struct device *dev)
 static int omap8250_runtime_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
-	struct uart_8250_port *up;
-
-	/* In case runtime-pm tries this before we are setup */
-	if (!priv)
-		return 0;
+	struct uart_8250_port *up = NULL;
 
-	up = serial8250_get_port(priv->line);
+	if (priv->line >= 0)
+		up = serial8250_get_port(priv->line);
 
-	if (omap8250_lost_context(up))
+	if (up && omap8250_lost_context(up))
 		omap8250_restore_regs(up);
 
-	if (up->dma && up->dma->rxchan && !(priv->habit & UART_HAS_EFR2))
+	if (up && up->dma && up->dma->rxchan && !(priv->habit & UART_HAS_EFR2))
 		omap_8250_rx_dma(up);
 
 	priv->latency = priv->calc_latency;
-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 5/5] serial: 8250: omap: Fix life cycle issues for interrupt handlers
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
                   ` (3 preceding siblings ...)
  2022-09-28  7:29 ` [RFC PATCH 4/5] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
@ 2022-09-28  7:29 ` Tony Lindgren
  2022-09-28 15:33 ` [RFC PATCH 0/5] 8250_omap fixes for testing Ivaylo Dimitrov
  5 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2022-09-28  7:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Ivaylo Dimitrov, Merlijn Wajer,
	Romain Naour

We have the struct uart_8250_port instance recycled on device rebind while
the struct omap8250_priv instance is not. For the console uart,
__tty_hangup() does not call tty->ops->hangup() as cons_filp stays open,
and uart shutdown won't get called. This means we have a stale
priv->wakeirq handler around after unbind, and port->irq is not freed on
unbind.

There's no need to claim the interrupts on startup. We can fix this and
simplify the driver a bit by claiming the interrupts in probe, and clearing
them on remove. For the device interrupt, we can use devm_request_irq().

To do this, we change omap8250_irq() to use struct omap8250_priv data
directly so we don't have to wait for the assigned port from
serial8250_register_8250_port().

We must also drop IRQF_SHARED to set IRQ_NOAUTOEN to avoid spurious
interrupts until the port has been registered. There's no need for
IRQF_SHARED for 8250_omap, the serial port interrupt lines are dedicated
lines.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 44 ++++++++++++++---------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -625,9 +625,9 @@ static int omap_8250_dma_handle_irq(struct uart_port *port);
 
 static irqreturn_t omap8250_irq(int irq, void *dev_id)
 {
-	struct uart_port *port = dev_id;
-	struct omap8250_priv *priv = port->private_data;
-	struct uart_8250_port *up = up_to_u8250p(port);
+	struct omap8250_priv *priv = dev_id;
+	struct uart_8250_port *up = serial8250_get_port(priv->line);
+	struct uart_port *port = &up->port;
 	unsigned int iir, lsr;
 	int ret;
 
@@ -683,12 +683,6 @@ static int omap_8250_startup(struct uart_port *port)
 	struct omap8250_priv *priv = port->private_data;
 	int ret;
 
-	if (priv->wakeirq) {
-		ret = dev_pm_set_dedicated_wake_irq(port->dev, priv->wakeirq);
-		if (ret)
-			return ret;
-	}
-
 	pm_runtime_get_sync(port->dev);
 
 	up->mcr = 0;
@@ -712,11 +706,6 @@ static int omap_8250_startup(struct uart_port *port)
 		}
 	}
 
-	ret = request_irq(port->irq, omap8250_irq, IRQF_SHARED,
-			  dev_name(port->dev), port);
-	if (ret < 0)
-		goto err;
-
 	up->ier = UART_IER_RLSI | UART_IER_RDI;
 	serial_out(up, UART_IER, up->ier);
 
@@ -736,11 +725,6 @@ static int omap_8250_startup(struct uart_port *port)
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	return 0;
-err:
-	pm_runtime_mark_last_busy(port->dev);
-	pm_runtime_put_autosuspend(port->dev);
-	dev_pm_clear_wake_irq(port->dev);
-	return ret;
 }
 
 static void omap_8250_shutdown(struct uart_port *port)
@@ -773,8 +757,6 @@ static void omap_8250_shutdown(struct uart_port *port)
 
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
-	free_irq(port->irq, port);
-	dev_pm_clear_wake_irq(port->dev);
 }
 
 static void omap_8250_throttle(struct uart_port *port)
@@ -1387,8 +1369,6 @@ static int omap8250_probe(struct platform_device *pdev)
 				 &up.overrun_backoff_time_ms) != 0)
 		up.overrun_backoff_time_ms = 0;
 
-	priv->wakeirq = irq_of_parse_and_map(np, 1);
-
 	pdata = of_device_get_match_data(&pdev->dev);
 	if (pdata)
 		priv->habit |= pdata->habit;
@@ -1466,16 +1446,33 @@ static int omap8250_probe(struct platform_device *pdev)
 		}
 	}
 #endif
+
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(&pdev->dev, irq, omap8250_irq, 0,
+			       dev_name(&pdev->dev), priv);
+	if (ret < 0)
+		return ret;
+
+	priv->wakeirq = irq_of_parse_and_map(np, 1);
+	if (priv->wakeirq) {
+		ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, priv->wakeirq);
+		if (ret)
+			return ret;
+	}
+
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to register 8250 port\n");
 		goto err;
 	}
 	priv->line = ret;
+	platform_set_drvdata(pdev, priv);
+	enable_irq(irq);
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 	return 0;
 err:
+	dev_pm_clear_wake_irq(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -1493,6 +1490,7 @@ static int omap8250_remove(struct platform_device *pdev)
 
 	serial8250_unregister_port(priv->line);
 	priv->line = -ENODEV;
+	dev_pm_clear_wake_irq(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	flush_work(&priv->qos_work);
-- 
2.37.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH 0/5] 8250_omap fixes for testing
  2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
                   ` (4 preceding siblings ...)
  2022-09-28  7:29 ` [RFC PATCH 5/5] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
@ 2022-09-28 15:33 ` Ivaylo Dimitrov
  5 siblings, 0 replies; 7+ messages in thread
From: Ivaylo Dimitrov @ 2022-09-28 15:33 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Johan Hovold, Sebastian Andrzej Siewior, Vignesh Raghavendra,
	linux-serial, linux-omap, Merlijn Wajer, Romain Naour,
	Greg Kroah-Hartman

Hi Tony,

On 28.09.22 г. 10:29 ч., Tony Lindgren wrote:
> Hi all,
> 
> Here are some 8250_omap fixes for testing. I'm not sure if the first patch
> fixes the issue reported. I'm not sure if I've seen that one, so please test.
> 
> The other patches are for all kind of issues I started running into after
> testing rebinding the driver.

It seems the external abort I was seeing on reboot/poweroff on my d4 is 
no longer here after applying the series, feel free to add:

Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

> 
> These are tagged RFC as we're close to the merge window, I'll repost the
> series probably after -rc1 after folks have tested this a bit. It seems
> that all these issues have been around for quite a long time.
> 
> Regards,
> 
> Tony
> 
> 
> Tony Lindgren (5):
>    serial: 8250: omap: Fix missing PM runtime calls for
>      omap8250_set_mctrl()
>    serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in
>      omap8250_remove()
>    serial: 8250: omap: Flush PM QOS work on remove
>    serial: 8250: omap: Fix imprecise external abort for omap_8250_pm()
>    serial: 8250: omap: Fix life cycle issues for interrupt handlers
> 
>   drivers/tty/serial/8250/8250_omap.c | 140 +++++++++++++++++-----------
>   1 file changed, 84 insertions(+), 56 deletions(-)
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-28 15:33 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-28  7:29 [RFC PATCH 0/5] 8250_omap fixes for testing Tony Lindgren
2022-09-28  7:29 ` [RFC PATCH 1/5] serial: 8250: omap: Fix missing PM runtime calls for omap8250_set_mctrl() Tony Lindgren
2022-09-28  7:29 ` [RFC PATCH 2/5] serial: 8250: omap: Fix unpaired pm_runtime_put_sync() in omap8250_remove() Tony Lindgren
2022-09-28  7:29 ` [RFC PATCH 3/5] serial: 8250: omap: Flush PM QOS work on remove Tony Lindgren
2022-09-28  7:29 ` [RFC PATCH 4/5] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
2022-09-28  7:29 ` [RFC PATCH 5/5] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
2022-09-28 15:33 ` [RFC PATCH 0/5] 8250_omap fixes for testing Ivaylo Dimitrov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).