[PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[Ilpo Järvinen]

Invoking TIOCVHANGUP on 8250_mid port and then reopening the port
triggers these faults during serial8250_do_startup():

  DMAR: DRHD: handling fault status reg 3
  DMAR: [DMA Write NO_PASID] Request device [00:1a.0] fault addr 0x0 [fault reason 0x05] PTE Write access is not set

The cause is a DMA write to the address in MSI address register that
was zeroed during the hangup as the irq was freed. The writes are
triggered due signalling an interrupt during the THRE test that
temporarily toggles THRI in IER. The THRE test currently occurs before
UART's irq (and MSI address) is properly set up.

Refactor serial8250_do_startup() such that irq is set up before the
THRE test. The current irq setup code is intermixed with the timer
setup code. As THRE test must be performed prior to the timer setup,
extract it into own function and call it only after the THRE test.

Reported-by: Lennert Buytenhek <buytenh@arista.com>
Tested-by: Lennert Buytenhek <buytenh@arista.com>
Fixes: 40b36daad0ac ("[PATCH] 8250 UART backup timer")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v2:
- Remove unnecessary changes to comments & newlines
- Change Lennert's email & add Tested-by
- Improve description of the problem (thank to Lennert's explanation)

 drivers/tty/serial/8250/8250.h      |  2 ++
 drivers/tty/serial/8250/8250_core.c | 16 +++++++++++-----
 drivers/tty/serial/8250/8250_port.c |  8 +++++---
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 287153d32536..dbf4c1204bf3 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -403,3 +403,5 @@ static inline int serial_index(struct uart_port *port)
 {
 	return port->minor - 64;
 }
+
+void univ8250_setup_timer(struct uart_8250_port *up);
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 2e83e7367441..10d535640434 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -298,10 +298,9 @@ static void serial8250_backup_timeout(struct timer_list *t)
 		jiffies + uart_poll_timeout(&up->port) + HZ / 5);
 }
 
-static int univ8250_setup_irq(struct uart_8250_port *up)
+void univ8250_setup_timer(struct uart_8250_port *up)
 {
 	struct uart_port *port = &up->port;
-	int retval = 0;
 
 	/*
 	 * The above check will only give an accurate result the first time
@@ -322,10 +321,17 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
 	 */
 	if (!port->irq)
 		mod_timer(&up->timer, jiffies + uart_poll_timeout(port));
-	else
-		retval = serial_link_irq_chain(up);
+}
+EXPORT_SYMBOL_GPL(univ8250_setup_timer);
 
-	return retval;
+static int univ8250_setup_irq(struct uart_8250_port *up)
+{
+	struct uart_port *port = &up->port;
+
+	if (port->irq)
+		return serial_link_irq_chain(up);
+
+	return 0;
 }
 
 static void univ8250_release_irq(struct uart_8250_port *up)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 39b35a61958c..6e8e16227a3a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2294,6 +2294,10 @@ int serial8250_do_startup(struct uart_port *port)
 	if (port->irq && (up->port.flags & UPF_SHARE_IRQ))
 		up->port.irqflags |= IRQF_SHARED;
 
+	retval = up->ops->setup_irq(up);
+	if (retval)
+		goto out;
+
 	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
 		unsigned char iir1;
 
@@ -2336,9 +2340,7 @@ int serial8250_do_startup(struct uart_port *port)
 		}
 	}
 
-	retval = up->ops->setup_irq(up);
-	if (retval)
-		goto out;
+	univ8250_setup_timer(up);
 
 	/*
 	 * Now, initialize the UART
-- 
2.30.2

Re: [PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 04:38:04PM +0300, Ilpo Järvinen wrote:
> Invoking TIOCVHANGUP on 8250_mid port and then reopening the port
> triggers these faults during serial8250_do_startup():
> 
>   DMAR: DRHD: handling fault status reg 3
>   DMAR: [DMA Write NO_PASID] Request device [00:1a.0] fault addr 0x0 [fault reason 0x05] PTE Write access is not set
> 
> The cause is a DMA write to the address in MSI address register that
> was zeroed during the hangup as the irq was freed. The writes are
> triggered due signalling an interrupt during the THRE test that
> temporarily toggles THRI in IER. The THRE test currently occurs before
> UART's irq (and MSI address) is properly set up.
> 
> Refactor serial8250_do_startup() such that irq is set up before the
> THRE test. The current irq setup code is intermixed with the timer
> setup code. As THRE test must be performed prior to the timer setup,
> extract it into own function and call it only after the THRE test.

Not sure if it was a formal v1, but anyway, the result is good, since
I was following the thread from the beginning.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Reported-by: Lennert Buytenhek <buytenh@arista.com>
> Tested-by: Lennert Buytenhek <buytenh@arista.com>
> Fixes: 40b36daad0ac ("[PATCH] 8250 UART backup timer")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v2:
> - Remove unnecessary changes to comments & newlines
> - Change Lennert's email & add Tested-by
> - Improve description of the problem (thank to Lennert's explanation)
> 
>  drivers/tty/serial/8250/8250.h      |  2 ++
>  drivers/tty/serial/8250/8250_core.c | 16 +++++++++++-----
>  drivers/tty/serial/8250/8250_port.c |  8 +++++---
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 287153d32536..dbf4c1204bf3 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -403,3 +403,5 @@ static inline int serial_index(struct uart_port *port)
>  {
>  	return port->minor - 64;
>  }
> +
> +void univ8250_setup_timer(struct uart_8250_port *up);
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 2e83e7367441..10d535640434 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -298,10 +298,9 @@ static void serial8250_backup_timeout(struct timer_list *t)
>  		jiffies + uart_poll_timeout(&up->port) + HZ / 5);
>  }
>  
> -static int univ8250_setup_irq(struct uart_8250_port *up)
> +void univ8250_setup_timer(struct uart_8250_port *up)
>  {
>  	struct uart_port *port = &up->port;
> -	int retval = 0;
>  
>  	/*
>  	 * The above check will only give an accurate result the first time
> @@ -322,10 +321,17 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
>  	 */
>  	if (!port->irq)
>  		mod_timer(&up->timer, jiffies + uart_poll_timeout(port));
> -	else
> -		retval = serial_link_irq_chain(up);
> +}
> +EXPORT_SYMBOL_GPL(univ8250_setup_timer);
>  
> -	return retval;
> +static int univ8250_setup_irq(struct uart_8250_port *up)
> +{
> +	struct uart_port *port = &up->port;
> +
> +	if (port->irq)
> +		return serial_link_irq_chain(up);
> +
> +	return 0;
>  }
>  
>  static void univ8250_release_irq(struct uart_8250_port *up)
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 39b35a61958c..6e8e16227a3a 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2294,6 +2294,10 @@ int serial8250_do_startup(struct uart_port *port)
>  	if (port->irq && (up->port.flags & UPF_SHARE_IRQ))
>  		up->port.irqflags |= IRQF_SHARED;
>  
> +	retval = up->ops->setup_irq(up);
> +	if (retval)
> +		goto out;
> +
>  	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
>  		unsigned char iir1;
>  
> @@ -2336,9 +2340,7 @@ int serial8250_do_startup(struct uart_port *port)
>  		}
>  	}
>  
> -	retval = up->ops->setup_irq(up);
> -	if (retval)
> -		goto out;
> +	univ8250_setup_timer(up);
>  
>  	/*
>  	 * Now, initialize the UART
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 06:36:16PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 16, 2022 at 04:38:04PM +0300, Ilpo Järvinen wrote:

Side note:

$ git grep -n -w setup_irq -- drivers/tty/
drivers/tty/serial/8250/8250_core.c:382:        .setup_irq      = univ8250_setup_irq,
drivers/tty/serial/8250/8250_port.c:2341:       retval = up->ops->setup_irq(up);

which rises a question of whether we need the setup_irq member in the
respective structure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 06:44:49PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 16, 2022 at 06:36:16PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 16, 2022 at 04:38:04PM +0300, Ilpo Järvinen wrote:
> 
> Side note:
> 
> $ git grep -n -w setup_irq -- drivers/tty/
> drivers/tty/serial/8250/8250_core.c:382:        .setup_irq      = univ8250_setup_irq,
> drivers/tty/serial/8250/8250_port.c:2341:       retval = up->ops->setup_irq(up);
> 
> which rises a question of whether we need the setup_irq member in the
> respective structure.

And to be confident, the files that include linux/serial_8250.h and have
setup_irq word in them (recursive `git grep`) in entire source tree:

drivers/tty/serial/8250/8250_core.c
drivers/tty/serial/8250/8250_port.c
include/linux/serial_8250.h

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250: Turn IER bits on only after irq has been set up

[Ilpo Järvinen]

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

On Mon, 19 Sep 2022, Andy Shevchenko wrote:

> On Fri, Sep 16, 2022 at 06:44:49PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 16, 2022 at 06:36:16PM +0300, Andy Shevchenko wrote:
> > > On Fri, Sep 16, 2022 at 04:38:04PM +0300, Ilpo Järvinen wrote:
> > 
> > Side note:
> > 
> > $ git grep -n -w setup_irq -- drivers/tty/
> > drivers/tty/serial/8250/8250_core.c:382:        .setup_irq      = univ8250_setup_irq,
> > drivers/tty/serial/8250/8250_port.c:2341:       retval = up->ops->setup_irq(up);
> > 
> > which rises a question of whether we need the setup_irq member in the
> > respective structure.
> 
> And to be confident, the files that include linux/serial_8250.h and have
> setup_irq word in them (recursive `git grep`) in entire source tree:
> 
> drivers/tty/serial/8250/8250_core.c
> drivers/tty/serial/8250/8250_port.c
> include/linux/serial_8250.h

Thanks. I know about it already.

The whole struct uart_8250_ops seems useless indirection layer (and 
IIRC the results of my history dig, it was added w/o any justification). 
It's on my kill list already :-) but I'll wait a bit with the cleanups to 
avoid -linus vs -next conflicts.

-- 
 i.