linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] More console rebind changes for 8250_omap serial driver
@ 2023-05-08  8:20 Tony Lindgren
  2023-05-08  8:20 ` [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register Tony Lindgren
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08  8:20 UTC (permalink / raw)
  Cc: Andy Shevchenko, Dhruva Gole, Ilpo Järvinen, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-omap

Hi all,

Here are more serial console rebind improvments for 8250_omap. Turns out
there have been some life cycle issues in the driver for quite a while.
The issues rebinding the driver have certainly made debugging serial core
runtime PM changes a bit harder..

Probably only the first patch is needed as a fix for the stable kernels.

Not sure if folks have been hitting the other issues so far. These might
issues only if device tree overlays are being used for serial ports, and
I'm not aware of such cases so far with the mainline kernel.

Regards,

Tony


Tony Lindgren (4):
  serial: 8250: omap: Fix freeing of resources on failed register
  serial: 8250: omap: Fix imprecise external abort for omap_8250_pm()
  serial: 8250: omap: Fix life cycle issues for interrupt handlers
  serial: 8250: omap: Shut down on remove for console uart

 drivers/tty/serial/8250/8250_omap.c | 141 +++++++++++++++-------------
 1 file changed, 76 insertions(+), 65 deletions(-)

-- 
2.40.1

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

* [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register
  2023-05-08  8:20 [PATCH 0/4] More console rebind changes for 8250_omap serial driver Tony Lindgren
@ 2023-05-08  8:20 ` Tony Lindgren
  2023-05-08  9:55   ` Ilpo Järvinen
  2023-05-08  8:20 ` [PATCH 2/4] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08  8:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Peter Hurley, Sebastian Andrzej Siewior
  Cc: Andy Shevchenko, Dhruva Gole, Ilpo Järvinen, Johan Hovold,
	Vignesh Raghavendra, linux-omap, linux-serial, linux-kernel

If serial8250_register_8250_port() fails, the SoC can hang as the
deferred PMQoS work will still run as is not flushed and removed.

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 | 2 ++
 1 file changed, 2 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
@@ -1532,7 +1532,9 @@ static int omap8250_probe(struct platform_device *pdev)
 err:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
+	flush_work(&priv->qos_work);
 	pm_runtime_disable(&pdev->dev);
+	cpu_latency_qos_remove_request(&priv->pm_qos_request);
 	return ret;
 }
 
-- 
2.40.1

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

* [PATCH 2/4] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm()
  2023-05-08  8:20 [PATCH 0/4] More console rebind changes for 8250_omap serial driver Tony Lindgren
  2023-05-08  8:20 ` [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register Tony Lindgren
@ 2023-05-08  8:20 ` Tony Lindgren
  2023-05-08  8:20 ` [PATCH 3/4] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
  2023-05-08  8:20 ` [PATCH 4/4] serial: 8250: omap: Shut down on remove for console uart Tony Lindgren
  3 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08  8:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Dhruva Gole, Ilpo Järvinen, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-omap,
	linux-serial, linux-kernel

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 needs to have runtime PM functional before the
driver probe calls serial8250_register_8250_port(). And it needs
runtime PM 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 that 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 a custom 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 instead, and use it for runtime PM related
functions to remove the dependency to port->membase for early and
late register access.

Note that during use, we need to check for a valid port in the runtime PM
related functions. This is needed for the optional wakeup configuration.
We now need to set the drvdata a bit earlier so it's available for the
runtime PM functions.

With the port checks in runtime PM functions, the old checks for priv in
omap8250_runtime_suspend() and omap8250_runtime_resume() functions 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)
@@ -115,6 +116,7 @@
 #define UART_OMAP_RX_LVL		0x19
 
 struct omap8250_priv {
+	void __iomem *membase;
 	int line;
 	u8 habit;
 	u8 mdr1;
@@ -159,9 +161,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));
 }
 
 /*
@@ -548,7 +555,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;
@@ -1394,7 +1401,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;
@@ -1457,6 +1464,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);
@@ -1464,6 +1473,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);
@@ -1525,7 +1536,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;
@@ -1547,11 +1557,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;
@@ -1627,7 +1638,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;
@@ -1641,20 +1651,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) {
@@ -1668,13 +1678,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;
+	struct uart_8250_port *up = NULL;
 
-	/* In case runtime-pm tries this before we are setup */
-	if (!priv)
-		return 0;
-
-	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,
@@ -1682,7 +1689,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;
 	}
 
@@ -1693,13 +1700,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;
@@ -1711,18 +1720,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;
+	struct uart_8250_port *up = NULL;
 
-	/* In case runtime-pm tries this before we are setup */
-	if (!priv)
-		return 0;
+	if (priv->line >= 0)
+		up = serial8250_get_port(priv->line);
 
-	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.40.1

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

* [PATCH 3/4] serial: 8250: omap: Fix life cycle issues for interrupt handlers
  2023-05-08  8:20 [PATCH 0/4] More console rebind changes for 8250_omap serial driver Tony Lindgren
  2023-05-08  8:20 ` [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register Tony Lindgren
  2023-05-08  8:20 ` [PATCH 2/4] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
@ 2023-05-08  8:20 ` Tony Lindgren
  2023-05-08  8:20 ` [PATCH 4/4] serial: 8250: omap: Shut down on remove for console uart Tony Lindgren
  3 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08  8:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Dhruva Gole, Ilpo Järvinen, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-omap,
	linux-serial, linux-kernel

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 | 33 ++++++++++++++---------------
 1 file changed, 16 insertions(+), 17 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
@@ -623,9 +623,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;
 
@@ -709,11 +709,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);
 
@@ -730,14 +725,11 @@ static int omap_8250_startup(struct uart_port *port)
 	if (up->dma && !(priv->habit & UART_HAS_EFR2))
 		up->dma->rx_dma(up);
 
+	enable_irq(up->port.irq);
+
 	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)
@@ -757,6 +749,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 
 	up->ier = 0;
 	serial_out(up, UART_IER, 0);
+	disable_irq_nosync(up->port.irq);
+	dev_pm_clear_wake_irq(port->dev);
 
 	if (up->dma)
 		serial8250_release_dma(up);
@@ -770,8 +764,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)
@@ -1451,8 +1443,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;
@@ -1530,6 +1520,15 @@ 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);
+
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to register 8250 port\n");
-- 
2.40.1

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

* [PATCH 4/4] serial: 8250: omap: Shut down on remove for console uart
  2023-05-08  8:20 [PATCH 0/4] More console rebind changes for 8250_omap serial driver Tony Lindgren
                   ` (2 preceding siblings ...)
  2023-05-08  8:20 ` [PATCH 3/4] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
@ 2023-05-08  8:20 ` Tony Lindgren
  3 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08  8:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Dhruva Gole, Ilpo Järvinen, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-omap,
	linux-serial, linux-kernel

When unbinding the console uart, we want to power down the uart hardware
on remove. For the console uart, the normal shutdown path will never get
called as the cons_filp stays open. Let's rearrange the dma related
functions a bit so we can call driver shutdown also on console uart rebind.

Currently we set up->dma on probe, but that causes issues calling
omap_8250_shutdown() on remove. The dma startup will not get called on
the next rebind as we still have up->dma set from probe.

Note that serial8250_release_dma() already checks for dma so we can
remove the check for it in 8205_omap driver.

With these changes we also avoid hogging dma virtual channels for the
unused uarts that may be limited on some devices.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 36 ++++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 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
@@ -679,6 +679,7 @@ static int omap_8250_startup(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct omap8250_priv *priv = port->private_data;
+	struct uart_8250_dma *dma = &priv->omap8250_dma;
 	int ret;
 
 	if (priv->wakeirq) {
@@ -697,16 +698,16 @@ static int omap_8250_startup(struct uart_port *port)
 	up->msr_saved_flags = 0;
 
 	/* Disable DMA for console UART */
-	if (uart_console(port))
-		up->dma = NULL;
-
-	if (up->dma) {
+	if (dma->fn && !uart_console(port)) {
+		up->dma = &priv->omap8250_dma;
 		ret = serial8250_request_dma(up);
 		if (ret) {
 			dev_warn_ratelimited(port->dev,
 					     "failed to request DMA\n");
 			up->dma = NULL;
 		}
+	} else {
+		up->dma = NULL;
 	}
 
 	up->ier = UART_IER_RLSI | UART_IER_RDI;
@@ -752,8 +753,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 	disable_irq_nosync(up->port.irq);
 	dev_pm_clear_wake_irq(port->dev);
 
-	if (up->dma)
-		serial8250_release_dma(up);
+	serial8250_release_dma(up);
+	up->dma = NULL;
 
 	/*
 	 * Disable break condition and FIFOs
@@ -1499,24 +1500,24 @@ static int omap8250_probe(struct platform_device *pdev)
 	ret = of_property_count_strings(np, "dma-names");
 	if (ret == 2) {
 		struct omap8250_dma_params *dma_params = NULL;
+		struct uart_8250_dma *dma = &priv->omap8250_dma;
 
-		up.dma = &priv->omap8250_dma;
-		up.dma->fn = the_no_dma_filter_fn;
-		up.dma->tx_dma = omap_8250_tx_dma;
-		up.dma->rx_dma = omap_8250_rx_dma;
+		dma->fn = the_no_dma_filter_fn;
+		dma->tx_dma = omap_8250_tx_dma;
+		dma->rx_dma = omap_8250_rx_dma;
 		if (pdata)
 			dma_params = pdata->dma_params;
 
 		if (dma_params) {
-			up.dma->rx_size = dma_params->rx_size;
-			up.dma->rxconf.src_maxburst = dma_params->rx_trigger;
-			up.dma->txconf.dst_maxburst = dma_params->tx_trigger;
+			dma->rx_size = dma_params->rx_size;
+			dma->rxconf.src_maxburst = dma_params->rx_trigger;
+			dma->txconf.dst_maxburst = dma_params->tx_trigger;
 			priv->rx_trigger = dma_params->rx_trigger;
 			priv->tx_trigger = dma_params->tx_trigger;
 		} else {
-			up.dma->rx_size = RX_TRIGGER;
-			up.dma->rxconf.src_maxburst = RX_TRIGGER;
-			up.dma->txconf.dst_maxburst = TX_TRIGGER;
+			dma->rx_size = RX_TRIGGER;
+			dma->rxconf.src_maxburst = RX_TRIGGER;
+			dma->txconf.dst_maxburst = TX_TRIGGER;
 		}
 	}
 #endif
@@ -1550,12 +1551,15 @@ static int omap8250_probe(struct platform_device *pdev)
 static int omap8250_remove(struct platform_device *pdev)
 {
 	struct omap8250_priv *priv = platform_get_drvdata(pdev);
+	struct uart_8250_port *up;
 	int err;
 
 	err = pm_runtime_resume_and_get(&pdev->dev);
 	if (err)
 		return err;
 
+	up = serial8250_get_port(priv->line);
+	omap_8250_shutdown(&up->port);
 	serial8250_unregister_port(priv->line);
 	priv->line = -ENODEV;
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
-- 
2.40.1

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

* Re: [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register
  2023-05-08  8:20 ` [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register Tony Lindgren
@ 2023-05-08  9:55   ` Ilpo Järvinen
  2023-05-08 11:08     ` Tony Lindgren
  0 siblings, 1 reply; 7+ messages in thread
From: Ilpo Järvinen @ 2023-05-08  9:55 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Greg Kroah-Hartman, Jiri Slaby, Peter Hurley,
	Sebastian Andrzej Siewior, Andy Shevchenko, Dhruva Gole,
	Johan Hovold, Vignesh Raghavendra, linux-omap, linux-serial,
	LKML

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Mon, 8 May 2023, Tony Lindgren wrote:

> If serial8250_register_8250_port() fails, the SoC can hang as the
> deferred PMQoS work will still run as is not flushed and removed.
> 
> 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 | 2 ++
>  1 file changed, 2 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
> @@ -1532,7 +1532,9 @@ static int omap8250_probe(struct platform_device *pdev)
>  err:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_sync(&pdev->dev);
> +	flush_work(&priv->qos_work);
>  	pm_runtime_disable(&pdev->dev);
> +	cpu_latency_qos_remove_request(&priv->pm_qos_request);
>  	return ret;

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

An unrelated comment to the patch itself, there seems to be somewhat 
handwavy and possibly wrong calculation for the pm qos latency. First of 
all, I think it would want something based on port->frame_time, and I'm 
far from convinced that 64 is right as it matches FIFO size which doesn't 
feel correct for a wakeup related time.

-- 
 i.

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

* Re: [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register
  2023-05-08  9:55   ` Ilpo Järvinen
@ 2023-05-08 11:08     ` Tony Lindgren
  0 siblings, 0 replies; 7+ messages in thread
From: Tony Lindgren @ 2023-05-08 11:08 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Jiri Slaby, Peter Hurley,
	Sebastian Andrzej Siewior, Andy Shevchenko, Dhruva Gole,
	Johan Hovold, Vignesh Raghavendra, linux-omap, linux-serial,
	LKML

* Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [230508 09:55]:
> An unrelated comment to the patch itself, there seems to be somewhat 
> handwavy and possibly wrong calculation for the pm qos latency. First of 
> all, I think it would want something based on port->frame_time, and I'm 
> far from convinced that 64 is right as it matches FIFO size which doesn't 
> feel correct for a wakeup related time.

Thanks for spotting it, good point. I'll take a look at that in a separate
patch.

Regards,

Tony

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

end of thread, other threads:[~2023-05-08 11:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-08  8:20 [PATCH 0/4] More console rebind changes for 8250_omap serial driver Tony Lindgren
2023-05-08  8:20 ` [PATCH 1/4] serial: 8250: omap: Fix freeing of resources on failed register Tony Lindgren
2023-05-08  9:55   ` Ilpo Järvinen
2023-05-08 11:08     ` Tony Lindgren
2023-05-08  8:20 ` [PATCH 2/4] serial: 8250: omap: Fix imprecise external abort for omap_8250_pm() Tony Lindgren
2023-05-08  8:20 ` [PATCH 3/4] serial: 8250: omap: Fix life cycle issues for interrupt handlers Tony Lindgren
2023-05-08  8:20 ` [PATCH 4/4] serial: 8250: omap: Shut down on remove for console uart Tony Lindgren

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).