* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-09 10:57 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-09 10:57 UTC (permalink / raw) To: linux-serial, Greg Kroah-Hartman Cc: Russell King, Robin Murphy, linux-arm-kernel, Jakub Kiciński, Andrew Jackson, Graeme Gregory, Andre Przywara, Jorge Ramirez-Ortiz, popcorn mix, stable pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively Signed-off-by: Dave Martin <Dave.Martin@arm.com> Tested-by: Robin Murphy <robin.murphy@arm.com> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Cc: stable <stable@vger.kernel.org> # 4.1 --- drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-09 10:57 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-09 10:57 UTC (permalink / raw) To: linux-arm-kernel pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively Signed-off-by: Dave Martin <Dave.Martin@arm.com> Tested-by: Robin Murphy <robin.murphy@arm.com> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Cc: stable <stable@vger.kernel.org> # 4.1 --- drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-07-09 10:57 ` Dave Martin @ 2015-07-23 22:05 ` Greg Kroah-Hartman -1 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-07-23 22:05 UTC (permalink / raw) To: Dave Martin Cc: linux-serial, Russell King, Robin Murphy, linux-arm-kernel, Jakub Kiciński, Andrew Jackson, Graeme Gregory, Andre Przywara, Jorge Ramirez-Ortiz, popcorn mix, stable On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > --- > drivers/tty/serial/amba-pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This doesn't apply at all to 4.2-rc3, so I can't apply it :( ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-23 22:05 ` Greg Kroah-Hartman 0 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-07-23 22:05 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > --- > drivers/tty/serial/amba-pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This doesn't apply at all to 4.2-rc3, so I can't apply it :( ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-07-23 22:05 ` Greg Kroah-Hartman @ 2015-07-24 9:56 ` Dave Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-24 9:56 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, stable, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > --- > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( This is applicable to 4.1 stable only, since the code this patch fixes was replaced in 4.2-rc1. Apologies, I didn't make that clear in the text. Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-24 9:56 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-24 9:56 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > --- > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( This is applicable to 4.1 stable only, since the code this patch fixes was replaced in 4.2-rc1. Apologies, I didn't make that clear in the text. Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-07-24 9:56 ` Dave Martin @ 2015-07-24 15:15 ` Greg Kroah-Hartman -1 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-07-24 15:15 UTC (permalink / raw) To: Dave Martin Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, stable, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Fri, Jul 24, 2015 at 10:56:06AM +0100, Dave Martin wrote: > On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > > sufficient to inhibit pl011_int() from being triggered by a local > > > IRQ and trying to re-take the same lock. This can lead to > > > deadlocks. > > > > > > This patch uses the _irq() locking variants instead to ensure that > > > pl011_int() handling for a given port is deferred until any > > > pl011_tx_softirq() work for that port is complete. > > > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > --- > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( > > This is applicable to 4.1 stable only, since the code this patch fixes > was replaced in 4.2-rc1. > > Apologies, I didn't make that clear in the text. I can't do anything with a 4.1-only patch, please read Documentation/stable_kernel_rules.txt for how to get a patch into the stable kernel releases. thanks, greg k-h ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-24 15:15 ` Greg Kroah-Hartman 0 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-07-24 15:15 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 24, 2015 at 10:56:06AM +0100, Dave Martin wrote: > On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > > sufficient to inhibit pl011_int() from being triggered by a local > > > IRQ and trying to re-take the same lock. This can lead to > > > deadlocks. > > > > > > This patch uses the _irq() locking variants instead to ensure that > > > pl011_int() handling for a given port is deferred until any > > > pl011_tx_softirq() work for that port is complete. > > > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > --- > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( > > This is applicable to 4.1 stable only, since the code this patch fixes > was replaced in 4.2-rc1. > > Apologies, I didn't make that clear in the text. I can't do anything with a 4.1-only patch, please read Documentation/stable_kernel_rules.txt for how to get a patch into the stable kernel releases. thanks, greg k-h ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-07-24 15:15 ` Greg Kroah-Hartman @ 2015-07-24 16:46 ` Dave Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-24 16:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, stable, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Fri, Jul 24, 2015 at 08:15:59AM -0700, Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2015 at 10:56:06AM +0100, Dave Martin wrote: > > On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > > > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > > > sufficient to inhibit pl011_int() from being triggered by a local > > > > IRQ and trying to re-take the same lock. This can lead to > > > > deadlocks. > > > > > > > > This patch uses the _irq() locking variants instead to ensure that > > > > pl011_int() handling for a given port is deferred until any > > > > pl011_tx_softirq() work for that port is complete. > > > > > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > > --- > > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( > > > > This is applicable to 4.1 stable only, since the code this patch fixes > > was replaced in 4.2-rc1. > > > > Apologies, I didn't make that clear in the text. > > I can't do anything with a 4.1-only patch, please read > Documentation/stable_kernel_rules.txt for how to get a patch into the > stable kernel releases. OK, reposted[1] with a clear reference to the equivalent upstream commit, and explanation of why this patch is different, which I'm guessing is what was missing. Is that sufficient? Cheers ---Dave [1] http://marc.info/?l=linux-serial&m=143775597205041&w=2 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-07-24 16:46 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-07-24 16:46 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 24, 2015 at 08:15:59AM -0700, Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2015 at 10:56:06AM +0100, Dave Martin wrote: > > On Thu, Jul 23, 2015 at 03:05:50PM -0700, Greg Kroah-Hartman wrote: > > > On Thu, Jul 09, 2015 at 11:57:12AM +0100, Dave Martin wrote: > > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > > > sufficient to inhibit pl011_int() from being triggered by a local > > > > IRQ and trying to re-take the same lock. This can lead to > > > > deadlocks. > > > > > > > > This patch uses the _irq() locking variants instead to ensure that > > > > pl011_int() handling for a given port is deferred until any > > > > pl011_tx_softirq() work for that port is complete. > > > > > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > > --- > > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > This doesn't apply at all to 4.2-rc3, so I can't apply it :( > > > > This is applicable to 4.1 stable only, since the code this patch fixes > > was replaced in 4.2-rc1. > > > > Apologies, I didn't make that clear in the text. > > I can't do anything with a 4.1-only patch, please read > Documentation/stable_kernel_rules.txt for how to get a patch into the > stable kernel releases. OK, reposted[1] with a clear reference to the equivalent upstream commit, and explanation of why this patch is different, which I'm guessing is what was missing. Is that sufficient? Cheers ---Dave [1] http://marc.info/?l=linux-serial&m=143775597205041&w=2 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-18 13:54 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-18 13:54 UTC (permalink / raw) To: linux-serial, Greg Kroah-Hartman Cc: Russell King, Robin Murphy, linux-arm-kernel, Jakub Kiciński, Andrew Jackson, Graeme Gregory, Andre Przywara, Jorge Ramirez-Ortiz, popcorn mix, stable pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively Signed-off-by: Dave Martin <Dave.Martin@arm.com> Tested-by: Robin Murphy <robin.murphy@arm.com> Cc: stable <stable@vger.kernel.org> # 4.1 --- drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-18 13:54 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-18 13:54 UTC (permalink / raw) To: linux-arm-kernel pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively Signed-off-by: Dave Martin <Dave.Martin@arm.com> Tested-by: Robin Murphy <robin.murphy@arm.com> Cc: stable <stable@vger.kernel.org> # 4.1 --- drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-18 13:54 ` Dave Martin (?) @ 2015-06-20 9:09 ` Stefan Wahren -1 siblings, 0 replies; 33+ messages in thread From: Stefan Wahren @ 2015-06-20 9:09 UTC (permalink / raw) To: linux-serial, Greg Kroah-Hartman, Dave Martin Cc: Robin Murphy, popcorn mix, Jakub Kiciński, Andrew Jackson, Russell King, Graeme Gregory, Andre Przywara, linux-arm-kernel, Jorge Ramirez-Ortiz, stable > Dave Martin <Dave.Martin@arm.com> hat am 18. Juni 2015 um 15:54 geschrieben: > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > I also get info messages about inconsistent lock state on my mx28 board, after applying the patch the messages disappeared. Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Regards Stefan -- To unsubscribe from this list: send the line "unsubscribe stable" in ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-20 9:09 ` Stefan Wahren 0 siblings, 0 replies; 33+ messages in thread From: Stefan Wahren @ 2015-06-20 9:09 UTC (permalink / raw) To: linux-arm-kernel > Dave Martin <Dave.Martin@arm.com> hat am 18. Juni 2015 um 15:54 geschrieben: > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > I also get info messages about inconsistent lock state on my mx28 board, after applying the patch the messages disappeared. Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Regards Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-20 9:09 ` Stefan Wahren 0 siblings, 0 replies; 33+ messages in thread From: Stefan Wahren @ 2015-06-20 9:09 UTC (permalink / raw) To: linux-serial, Greg Kroah-Hartman, Dave Martin Cc: Robin Murphy, popcorn mix, Jakub Kiciński, Andrew Jackson, Russell King, Graeme Gregory, Andre Przywara, linux-arm-kernel, Jorge Ramirez-Ortiz, stable > Dave Martin <Dave.Martin@arm.com> hat am 18. Juni 2015 um 15:54 geschrieben: > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > I also get info messages about inconsistent lock state on my mx28 board, after applying the patch the messages disappeared. Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Regards Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-20 9:09 ` Stefan Wahren @ 2015-06-23 11:13 ` Dave P Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-23 11:13 UTC (permalink / raw) To: Stefan Wahren Cc: linux-serial, Greg Kroah-Hartman, Robin Murphy, popcorn mix, Jakub Kiciński, Andrew Jackson, Russell King, Graeme Gregory, Andre Przywara, linux-arm-kernel, Jorge Ramirez-Ortiz, stable On Sat, Jun 20, 2015 at 10:09:37AM +0100, Stefan Wahren wrote: > > > Dave Martin <Dave.Martin@arm.com> hat am 18. Juni 2015 um 15:54 geschrieben: > > > > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > > I also get info messages about inconsistent lock state on my mx28 board, > after applying the patch the messages disappeared. > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Thanks for the confirmation. Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-23 11:13 ` Dave P Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-23 11:13 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 20, 2015 at 10:09:37AM +0100, Stefan Wahren wrote: > > > Dave Martin <Dave.Martin@arm.com> hat am 18. Juni 2015 um 15:54 geschrieben: > > > > > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Fixes: 734745caeb9f serial/amba-pl011: Activate TX IRQ passively > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > > > I also get info messages about inconsistent lock state on my mx28 board, > after applying the patch the messages disappeared. > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> Thanks for the confirmation. Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-05 14:07 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-05 14:07 UTC (permalink / raw) To: linux-serial, Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, Graeme Gregory, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring. This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next. I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm. The patch is straightforward and using non _irq() locking in a workitem that expects the lock to protect against interrupt handlers on the same CPU is clearly wrong. Cheers ---Dave drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-05 14:07 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-05 14:07 UTC (permalink / raw) To: linux-arm-kernel pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks. This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring. This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next. I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm. The patch is straightforward and using non _irq() locking in a workitem that expects the lock to protect against interrupt handlers on the same CPU is clearly wrong. Cheers ---Dave drivers/tty/serial/amba-pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..0cc622a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) struct uart_amba_port *uap = container_of(dwork, struct uart_amba_port, tx_softirq_work); - spin_lock(&uap->port.lock); + spin_lock_irq(&uap->port.lock); while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); + spin_unlock_irq(&uap->port.lock); } static void pl011_tx_irq_seen(struct uart_amba_port *uap) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-05 14:07 ` Dave Martin @ 2015-06-05 18:03 ` Robin Murphy -1 siblings, 0 replies; 33+ messages in thread From: Robin Murphy @ 2015-06-05 18:03 UTC (permalink / raw) To: Dave P Martin, linux-serial, Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, Graeme Gregory, popcorn mix, Jorge Ramirez-Ortiz, linux-arm-kernel On 05/06/15 15:07, Dave P Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > was already in, but it went astray when I was refactoring. > > This patch conflicts with tty-next like the previous patch, since it > fixes code that is removed by tty-next. The correct resolution for > the resulting merge conflict is to keep the code from tty-next. > > > I am not 100% certain yet whether some rare deadlocks that Robin is > seeing are caused by this issue, or whether this patch fixes them -- > he's testing atm. FWIW, I've been running Juno in a startup/shutdown loop with a very noisy systemd all afternoon and haven't hit a problem yet with this patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep splats in about the same amount of time. I'll leave it going over the weekend just to make sure, though. Robin. > > The patch is straightforward and using non _irq() locking in a workitem > that expects the lock to protect against interrupt handlers on the same > CPU is clearly wrong. > > > Cheers > ---Dave > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 763eb20..0cc622a 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) > struct uart_amba_port *uap = > container_of(dwork, struct uart_amba_port, tx_softirq_work); > > - spin_lock(&uap->port.lock); > + spin_lock_irq(&uap->port.lock); > while (pl011_tx_chars(uap)) ; > - spin_unlock(&uap->port.lock); > + spin_unlock_irq(&uap->port.lock); > } > > static void pl011_tx_irq_seen(struct uart_amba_port *uap) > ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-05 18:03 ` Robin Murphy 0 siblings, 0 replies; 33+ messages in thread From: Robin Murphy @ 2015-06-05 18:03 UTC (permalink / raw) To: linux-arm-kernel On 05/06/15 15:07, Dave P Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > was already in, but it went astray when I was refactoring. > > This patch conflicts with tty-next like the previous patch, since it > fixes code that is removed by tty-next. The correct resolution for > the resulting merge conflict is to keep the code from tty-next. > > > I am not 100% certain yet whether some rare deadlocks that Robin is > seeing are caused by this issue, or whether this patch fixes them -- > he's testing atm. FWIW, I've been running Juno in a startup/shutdown loop with a very noisy systemd all afternoon and haven't hit a problem yet with this patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep splats in about the same amount of time. I'll leave it going over the weekend just to make sure, though. Robin. > > The patch is straightforward and using non _irq() locking in a workitem > that expects the lock to protect against interrupt handlers on the same > CPU is clearly wrong. > > > Cheers > ---Dave > > > drivers/tty/serial/amba-pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 763eb20..0cc622a 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) > struct uart_amba_port *uap = > container_of(dwork, struct uart_amba_port, tx_softirq_work); > > - spin_lock(&uap->port.lock); > + spin_lock_irq(&uap->port.lock); > while (pl011_tx_chars(uap)) ; > - spin_unlock(&uap->port.lock); > + spin_unlock_irq(&uap->port.lock); > } > > static void pl011_tx_irq_seen(struct uart_amba_port *uap) > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-05 18:03 ` Robin Murphy @ 2015-06-08 10:34 ` Robin Murphy -1 siblings, 0 replies; 33+ messages in thread From: Robin Murphy @ 2015-06-08 10:34 UTC (permalink / raw) To: Dave P Martin, linux-serial, Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, Graeme Gregory, popcorn mix, Jorge Ramirez-Ortiz, linux-arm-kernel On 05/06/15 19:03, Robin Murphy wrote: > On 05/06/15 15:07, Dave P Martin wrote: >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not >> sufficient to inhibit pl011_int() from being triggered by a local >> IRQ and trying to re-take the same lock. This can lead to >> deadlocks. >> >> This patch uses the _irq() locking variants instead to ensure that >> pl011_int() handling for a given port is deferred until any >> pl011_tx_softirq() work for that port is complete. >> >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> --- >> >> Another candidate for v4.1 if possible (sorry!) -- I thought this change >> was already in, but it went astray when I was refactoring. >> >> This patch conflicts with tty-next like the previous patch, since it >> fixes code that is removed by tty-next. The correct resolution for >> the resulting merge conflict is to keep the code from tty-next. >> >> >> I am not 100% certain yet whether some rare deadlocks that Robin is >> seeing are caused by this issue, or whether this patch fixes them -- >> he's testing atm. > > FWIW, I've been running Juno in a startup/shutdown loop with a very > noisy systemd all afternoon and haven't hit a problem yet with this > patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep > splats in about the same amount of time. I'll leave it going over the > weekend just to make sure, though. Having still seen nothing over several hundred more reboot cycles, I'm satisfied that the irq vs. softirq race explains the deadlock and that this patch fixes it, so; Tested-by: Robin Murphy <robin.murphy@arm.com> > > Robin. > >> >> The patch is straightforward and using non _irq() locking in a workitem >> that expects the lock to protect against interrupt handlers on the same >> CPU is clearly wrong. >> >> >> Cheers >> ---Dave >> >> >> drivers/tty/serial/amba-pl011.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c >> index 763eb20..0cc622a 100644 >> --- a/drivers/tty/serial/amba-pl011.c >> +++ b/drivers/tty/serial/amba-pl011.c >> @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) >> struct uart_amba_port *uap = >> container_of(dwork, struct uart_amba_port, tx_softirq_work); >> >> - spin_lock(&uap->port.lock); >> + spin_lock_irq(&uap->port.lock); >> while (pl011_tx_chars(uap)) ; >> - spin_unlock(&uap->port.lock); >> + spin_unlock_irq(&uap->port.lock); >> } >> >> static void pl011_tx_irq_seen(struct uart_amba_port *uap) >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-08 10:34 ` Robin Murphy 0 siblings, 0 replies; 33+ messages in thread From: Robin Murphy @ 2015-06-08 10:34 UTC (permalink / raw) To: linux-arm-kernel On 05/06/15 19:03, Robin Murphy wrote: > On 05/06/15 15:07, Dave P Martin wrote: >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not >> sufficient to inhibit pl011_int() from being triggered by a local >> IRQ and trying to re-take the same lock. This can lead to >> deadlocks. >> >> This patch uses the _irq() locking variants instead to ensure that >> pl011_int() handling for a given port is deferred until any >> pl011_tx_softirq() work for that port is complete. >> >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> --- >> >> Another candidate for v4.1 if possible (sorry!) -- I thought this change >> was already in, but it went astray when I was refactoring. >> >> This patch conflicts with tty-next like the previous patch, since it >> fixes code that is removed by tty-next. The correct resolution for >> the resulting merge conflict is to keep the code from tty-next. >> >> >> I am not 100% certain yet whether some rare deadlocks that Robin is >> seeing are caused by this issue, or whether this patch fixes them -- >> he's testing atm. > > FWIW, I've been running Juno in a startup/shutdown loop with a very > noisy systemd all afternoon and haven't hit a problem yet with this > patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep > splats in about the same amount of time. I'll leave it going over the > weekend just to make sure, though. Having still seen nothing over several hundred more reboot cycles, I'm satisfied that the irq vs. softirq race explains the deadlock and that this patch fixes it, so; Tested-by: Robin Murphy <robin.murphy@arm.com> > > Robin. > >> >> The patch is straightforward and using non _irq() locking in a workitem >> that expects the lock to protect against interrupt handlers on the same >> CPU is clearly wrong. >> >> >> Cheers >> ---Dave >> >> >> drivers/tty/serial/amba-pl011.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c >> index 763eb20..0cc622a 100644 >> --- a/drivers/tty/serial/amba-pl011.c >> +++ b/drivers/tty/serial/amba-pl011.c >> @@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work) >> struct uart_amba_port *uap = >> container_of(dwork, struct uart_amba_port, tx_softirq_work); >> >> - spin_lock(&uap->port.lock); >> + spin_lock_irq(&uap->port.lock); >> while (pl011_tx_chars(uap)) ; >> - spin_unlock(&uap->port.lock); >> + spin_unlock_irq(&uap->port.lock); >> } >> >> static void pl011_tx_irq_seen(struct uart_amba_port *uap) >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-08 10:34 ` Robin Murphy @ 2015-06-08 11:34 ` Dave P Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-08 11:34 UTC (permalink / raw) To: Robin Murphy Cc: Russell King, Jakub Kiciński, Greg Kroah-Hartman, Andrew Jackson, Graeme Gregory, linux-serial, Andre Przywara, popcorn mix, Jorge Ramirez-Ortiz, linux-arm-kernel On Mon, Jun 08, 2015 at 11:34:45AM +0100, Robin Murphy wrote: > On 05/06/15 19:03, Robin Murphy wrote: > > On 05/06/15 15:07, Dave P Martin wrote: > >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > >> sufficient to inhibit pl011_int() from being triggered by a local > >> IRQ and trying to re-take the same lock. This can lead to > >> deadlocks. > >> > >> This patch uses the _irq() locking variants instead to ensure that > >> pl011_int() handling for a given port is deferred until any > >> pl011_tx_softirq() work for that port is complete. > >> > >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> > >> --- > >> > >> Another candidate for v4.1 if possible (sorry!) -- I thought this change > >> was already in, but it went astray when I was refactoring. > >> > >> This patch conflicts with tty-next like the previous patch, since it > >> fixes code that is removed by tty-next. The correct resolution for > >> the resulting merge conflict is to keep the code from tty-next. > >> > >> > >> I am not 100% certain yet whether some rare deadlocks that Robin is > >> seeing are caused by this issue, or whether this patch fixes them -- > >> he's testing atm. > > > > FWIW, I've been running Juno in a startup/shutdown loop with a very > > noisy systemd all afternoon and haven't hit a problem yet with this > > patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep > > splats in about the same amount of time. I'll leave it going over the > > weekend just to make sure, though. > > Having still seen nothing over several hundred more reboot cycles, I'm > satisfied that the irq vs. softirq race explains the deadlock and that > this patch fixes it, so; > > Tested-by: Robin Murphy <robin.murphy@arm.com> Thanks for this Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-08 11:34 ` Dave P Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-08 11:34 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 08, 2015 at 11:34:45AM +0100, Robin Murphy wrote: > On 05/06/15 19:03, Robin Murphy wrote: > > On 05/06/15 15:07, Dave P Martin wrote: > >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > >> sufficient to inhibit pl011_int() from being triggered by a local > >> IRQ and trying to re-take the same lock. This can lead to > >> deadlocks. > >> > >> This patch uses the _irq() locking variants instead to ensure that > >> pl011_int() handling for a given port is deferred until any > >> pl011_tx_softirq() work for that port is complete. > >> > >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> > >> --- > >> > >> Another candidate for v4.1 if possible (sorry!) -- I thought this change > >> was already in, but it went astray when I was refactoring. > >> > >> This patch conflicts with tty-next like the previous patch, since it > >> fixes code that is removed by tty-next. The correct resolution for > >> the resulting merge conflict is to keep the code from tty-next. > >> > >> > >> I am not 100% certain yet whether some rare deadlocks that Robin is > >> seeing are caused by this issue, or whether this patch fixes them -- > >> he's testing atm. > > > > FWIW, I've been running Juno in a startup/shutdown loop with a very > > noisy systemd all afternoon and haven't hit a problem yet with this > > patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep > > splats in about the same amount of time. I'll leave it going over the > > weekend just to make sure, though. > > Having still seen nothing over several hundred more reboot cycles, I'm > satisfied that the irq vs. softirq race explains the deadlock and that > this patch fixes it, so; > > Tested-by: Robin Murphy <robin.murphy@arm.com> Thanks for this Cheers ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-05 14:07 ` Dave Martin @ 2015-06-13 0:39 ` Greg Kroah-Hartman -1 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-06-13 0:39 UTC (permalink / raw) To: Dave Martin Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > --- > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > was already in, but it went astray when I was refactoring. Too late for 4.1, sorry. This doesn't apply to my tty-next branch, otherwise I would have queued it up there. Care to redo it and mark it for -stable, like I did above, and resend it? thanks, greg k-h ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-13 0:39 ` Greg Kroah-Hartman 0 siblings, 0 replies; 33+ messages in thread From: Greg Kroah-Hartman @ 2015-06-13 0:39 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > sufficient to inhibit pl011_int() from being triggered by a local > IRQ and trying to re-take the same lock. This can lead to > deadlocks. > > This patch uses the _irq() locking variants instead to ensure that > pl011_int() handling for a given port is deferred until any > pl011_tx_softirq() work for that port is complete. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Robin Murphy <robin.murphy@arm.com> > Cc: stable <stable@vger.kernel.org> # 4.1 > --- > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > was already in, but it went astray when I was refactoring. Too late for 4.1, sorry. This doesn't apply to my tty-next branch, otherwise I would have queued it up there. Care to redo it and mark it for -stable, like I did above, and resend it? thanks, greg k-h ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-13 0:39 ` Greg Kroah-Hartman @ 2015-06-15 10:53 ` Dave P Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-15 10:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Russell King, Jakub Kiciński, Andre Przywara, Andrew Jackson, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Sat, Jun 13, 2015 at 01:39:45AM +0100, Greg Kroah-Hartman wrote: > On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > --- > > > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > > was already in, but it went astray when I was refactoring. > > Too late for 4.1, sorry. This doesn't apply to my tty-next branch, No worries -- I was aware it was getting very late. The code this fixes is obsoleted by -next, so it is relevant only for stable now. > otherwise I would have queued it up there. Care to redo it and mark it > for -stable, like I did above, and resend it? Will do, thanks. ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-15 10:53 ` Dave P Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave P Martin @ 2015-06-15 10:53 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 13, 2015 at 01:39:45AM +0100, Greg Kroah-Hartman wrote: > On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: > > pl011_tx_softirq() currently uses spin_{,un}lock(), which are not > > sufficient to inhibit pl011_int() from being triggered by a local > > IRQ and trying to re-take the same lock. This can lead to > > deadlocks. > > > > This patch uses the _irq() locking variants instead to ensure that > > pl011_int() handling for a given port is deferred until any > > pl011_tx_softirq() work for that port is complete. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Tested-by: Robin Murphy <robin.murphy@arm.com> > > Cc: stable <stable@vger.kernel.org> # 4.1 > > --- > > > > Another candidate for v4.1 if possible (sorry!) -- I thought this change > > was already in, but it went astray when I was refactoring. > > Too late for 4.1, sorry. This doesn't apply to my tty-next branch, No worries -- I was aware it was getting very late. The code this fixes is obsoleted by -next, so it is relevant only for stable now. > otherwise I would have queued it up there. Care to redo it and mark it > for -stable, like I did above, and resend it? Will do, thanks. ---Dave ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-13 0:39 ` Greg Kroah-Hartman @ 2015-06-15 11:09 ` Andre Przywara -1 siblings, 0 replies; 33+ messages in thread From: Andre Przywara @ 2015-06-15 11:09 UTC (permalink / raw) To: Greg Kroah-Hartman, Dave P Martin Cc: Russell King, Jakub Kiciński, Andrew Jackson, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel Hi Greg, On 06/13/2015 01:39 AM, Greg Kroah-Hartman wrote: > On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not >> sufficient to inhibit pl011_int() from being triggered by a local >> IRQ and trying to re-take the same lock. This can lead to >> deadlocks. >> >> This patch uses the _irq() locking variants instead to ensure that >> pl011_int() handling for a given port is deferred until any >> pl011_tx_softirq() work for that port is complete. >> >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> Tested-by: Robin Murphy <robin.murphy@arm.com> >> Cc: stable <stable@vger.kernel.org> # 4.1 >> --- >> >> Another candidate for v4.1 if possible (sorry!) -- I thought this change >> was already in, but it went astray when I was refactoring. > > Too late for 4.1, sorry. So if I get this correctly, we go from "some characters lost from systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? Is that right? I understand that you are reluctant to take this patch this late in the game, but wouldn't it actually be better now to revert PL011 to the state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 release? We could still backport the solution ending up in 4.2-rc to 4.1.x. Cheers, Andre. > This doesn't apply to my tty-next branch, > otherwise I would have queued it up there. Care to redo it and mark it > for -stable, like I did above, and resend it? > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-15 11:09 ` Andre Przywara 0 siblings, 0 replies; 33+ messages in thread From: Andre Przywara @ 2015-06-15 11:09 UTC (permalink / raw) To: linux-arm-kernel Hi Greg, On 06/13/2015 01:39 AM, Greg Kroah-Hartman wrote: > On Fri, Jun 05, 2015 at 03:07:47PM +0100, Dave Martin wrote: >> pl011_tx_softirq() currently uses spin_{,un}lock(), which are not >> sufficient to inhibit pl011_int() from being triggered by a local >> IRQ and trying to re-take the same lock. This can lead to >> deadlocks. >> >> This patch uses the _irq() locking variants instead to ensure that >> pl011_int() handling for a given port is deferred until any >> pl011_tx_softirq() work for that port is complete. >> >> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> Tested-by: Robin Murphy <robin.murphy@arm.com> >> Cc: stable <stable@vger.kernel.org> # 4.1 >> --- >> >> Another candidate for v4.1 if possible (sorry!) -- I thought this change >> was already in, but it went astray when I was refactoring. > > Too late for 4.1, sorry. So if I get this correctly, we go from "some characters lost from systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? Is that right? I understand that you are reluctant to take this patch this late in the game, but wouldn't it actually be better now to revert PL011 to the state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 release? We could still backport the solution ending up in 4.2-rc to 4.1.x. Cheers, Andre. > This doesn't apply to my tty-next branch, > otherwise I would have queued it up there. Care to redo it and mark it > for -stable, like I did above, and resend it? > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] serial/amba-pl011: Disable interrupts around TX softirq 2015-06-15 11:09 ` Andre Przywara @ 2015-06-15 14:03 ` Dave Martin -1 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-15 14:03 UTC (permalink / raw) To: Andre Przywara Cc: Russell King, Jakub Kiciński, Greg Kroah-Hartman, Andrew Jackson, Graeme Gregory, linux-serial, popcorn mix, Jorge Ramirez-Ortiz, Robin Murphy, linux-arm-kernel On Mon, Jun 15, 2015 at 12:09:59PM +0100, Andre Przywara wrote: [...] > On 06/13/2015 01:39 AM, Greg Kroah-Hartman wrote: [...] > > Too late for 4.1, sorry. > > So if I get this correctly, we go from "some characters lost from > systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? > Is that right? So it seems. It's my bad that we hit this so late in the cycle. > I understand that you are reluctant to take this patch this late in the > game, but wouldn't it actually be better now to revert PL011 to the > state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 > release? > We could still backport the solution ending up in 4.2-rc to 4.1.x. If neither this patch, nor what is in -next, can be applied in 4.1 (and I understand where Greg was coming from in saying "no"), then this seems inevitable. The only other alternative that might be viable is to revert the relevant changes from 4.1 as you suggest, and optionally try to fix the systemd console corruption issue via stable. However, all the fixes currently available involve a lot of code -- I'm not sure whether they are good candidates for stable. There's also a risk of getting into a worse mess with that late revert if it interacts with any other patches. However, the revert [1] doesn't look complicated. There is a 1-line context clash with some of the dma changes that needs to be resolved, but that's it. Opinions? To avoid further confusion I'll hold off on sending any further patch for the moment -- I'd like to see some sort of consensus on the way forward if possible. Cheers ---Dave [1] For me, I can build and boot Juno with the v4.0 behaviour with the following reverts. These would clash with next again, but the resolution is the same as before: the code in -next overrides these changes in master. >From 89cd9714f223117389f24b191ff97f6969d81c3c Mon Sep 17 00:00:00 2001 From: Dave Martin <Dave.Martin@arm.com> Date: Mon, 15 Jun 2015 14:06:25 +0100 Subject: [REVERT 1/2] Revert "serial/amba-pl011: Unconditionally poll for FIFO space before each TX char" This reverts commit 43dd1f9a5b05d6db2cb258354a01ace63baa5c0b. --- drivers/tty/serial/amba-pl011.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..6f5a072 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1249,19 +1249,20 @@ __acquires(&uap->port.lock) /* * Transmit a character + * There must be at least one free entry in the TX FIFO to accept the char. * - * Returns true if the character was successfully queued to the FIFO. - * Returns false otherwise. + * Returns true if the FIFO might have space in it afterwards; + * returns false if the FIFO definitely became full. */ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c) { - if (readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF) - return false; /* unable to transmit character */ - writew(c, uap->port.membase + UART01x_DR); uap->port.icount.tx++; - return true; + if (likely(uap->tx_irq_seen > 1)) + return true; + + return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF); } static bool pl011_tx_chars(struct uart_amba_port *uap) @@ -1295,8 +1296,7 @@ static bool pl011_tx_chars(struct uart_amba_port *uap) return false; if (uap->port.x_char) { - if (!pl011_tx_char(uap, uap->port.x_char)) - goto done; + pl011_tx_char(uap, uap->port.x_char); uap->port.x_char = 0; --count; } -- 1.7.10.4 >From ece86a450686f37c3761a25c64af013bc556ee61 Mon Sep 17 00:00:00 2001 From: Dave Martin <Dave.Martin@arm.com> Date: Mon, 15 Jun 2015 14:07:39 +0100 Subject: [REVERT 2/2] Revert "serial/amba-pl011: Activate TX IRQ passively" This reverts commit 734745caeb9f155ab58918834a8c70e83fa6afd3. Conflicts: drivers/tty/serial/amba-pl011.c --- drivers/tty/serial/amba-pl011.c | 165 ++++++++++++--------------------------- 1 file changed, 50 insertions(+), 115 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 6f5a072..a9074c0 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -58,7 +58,6 @@ #include <linux/pinctrl/consumer.h> #include <linux/sizes.h> #include <linux/io.h> -#include <linux/workqueue.h> #define UART_NR 14 @@ -157,9 +156,7 @@ struct uart_amba_port { unsigned int lcrh_tx; /* vendor-specific */ unsigned int lcrh_rx; /* vendor-specific */ unsigned int old_cr; /* state during shutdown */ - struct delayed_work tx_softirq_work; bool autorts; - unsigned int tx_irq_seen; /* 0=none, 1=1, 2=2 or more */ char type[12]; #ifdef CONFIG_DMA_ENGINE /* DMA stuff */ @@ -403,9 +400,8 @@ static void pl011_dma_remove(struct uart_amba_port *uap) dma_release_channel(uap->dmarx.chan); } -/* Forward declare these for the refill routine */ +/* Forward declare this for the refill routine */ static int pl011_dma_tx_refill(struct uart_amba_port *uap); -static void pl011_start_tx_pio(struct uart_amba_port *uap); /* * The current DMA TX buffer has been sent. @@ -443,13 +439,14 @@ static void pl011_dma_tx_callback(void *data) return; } - if (pl011_dma_tx_refill(uap) <= 0) + if (pl011_dma_tx_refill(uap) <= 0) { /* * We didn't queue a DMA buffer for some reason, but we * have data pending to be sent. Re-enable the TX IRQ. */ - pl011_start_tx_pio(uap); - + uap->im |= UART011_TXIM; + writew(uap->im, uap->port.membase + UART011_IMSC); + } spin_unlock_irqrestore(&uap->port.lock, flags); } @@ -627,10 +624,12 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap) if (!uap->dmatx.queued) { if (pl011_dma_tx_refill(uap) > 0) { uap->im &= ~UART011_TXIM; - writew(uap->im, uap->port.membase + - UART011_IMSC); - } else + ret = true; + } else { + uap->im |= UART011_TXIM; ret = false; + } + writew(uap->im, uap->port.membase + UART011_IMSC); } else if (!(uap->dmacr & UART011_TXDMAE)) { uap->dmacr |= UART011_TXDMAE; writew(uap->dmacr, @@ -1172,24 +1171,15 @@ static void pl011_stop_tx(struct uart_port *port) pl011_dma_tx_stop(uap); } -static bool pl011_tx_chars(struct uart_amba_port *uap); - -/* Start TX with programmed I/O only (no DMA) */ -static void pl011_start_tx_pio(struct uart_amba_port *uap) -{ - uap->im |= UART011_TXIM; - writew(uap->im, uap->port.membase + UART011_IMSC); - if (!uap->tx_irq_seen) - pl011_tx_chars(uap); -} - static void pl011_start_tx(struct uart_port *port) { struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); - if (!pl011_dma_tx_start(uap)) - pl011_start_tx_pio(uap); + if (!pl011_dma_tx_start(uap)) { + uap->im |= UART011_TXIM; + writew(uap->im, uap->port.membase + UART011_IMSC); + } } static void pl011_stop_rx(struct uart_port *port) @@ -1247,87 +1237,40 @@ __acquires(&uap->port.lock) spin_lock(&uap->port.lock); } -/* - * Transmit a character - * There must be at least one free entry in the TX FIFO to accept the char. - * - * Returns true if the FIFO might have space in it afterwards; - * returns false if the FIFO definitely became full. - */ -static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c) -{ - writew(c, uap->port.membase + UART01x_DR); - uap->port.icount.tx++; - - if (likely(uap->tx_irq_seen > 1)) - return true; - - return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF); -} - -static bool pl011_tx_chars(struct uart_amba_port *uap) +static void pl011_tx_chars(struct uart_amba_port *uap) { struct circ_buf *xmit = &uap->port.state->xmit; int count; - if (unlikely(uap->tx_irq_seen < 2)) - /* - * Initial FIFO fill level unknown: we must check TXFF - * after each write, so just try to fill up the FIFO. - */ - count = uap->fifosize; - else /* tx_irq_seen >= 2 */ - /* - * FIFO initially at least half-empty, so we can simply - * write half the FIFO without polling TXFF. - - * Note: the *first* TX IRQ can still race with - * pl011_start_tx_pio(), which can result in the FIFO - * being fuller than expected in that case. - */ - count = uap->fifosize >> 1; - - /* - * If the FIFO is full we're guaranteed a TX IRQ at some later point, - * and can't transmit immediately in any case: - */ - if (unlikely(uap->tx_irq_seen < 2 && - readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF)) - return false; - if (uap->port.x_char) { - pl011_tx_char(uap, uap->port.x_char); + writew(uap->port.x_char, uap->port.membase + UART01x_DR); + uap->port.icount.tx++; uap->port.x_char = 0; - --count; + return; } if (uart_circ_empty(xmit) || uart_tx_stopped(&uap->port)) { pl011_stop_tx(&uap->port); - goto done; + return; } /* If we are using DMA mode, try to send some characters. */ if (pl011_dma_tx_irq(uap)) - goto done; + return; - while (count-- > 0 && pl011_tx_char(uap, xmit->buf[xmit->tail])) { + count = uap->fifosize >> 1; + do { + writew(xmit->buf[xmit->tail], uap->port.membase + UART01x_DR); xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); + uap->port.icount.tx++; if (uart_circ_empty(xmit)) break; - } + } while (--count > 0); if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) uart_write_wakeup(&uap->port); - if (uart_circ_empty(xmit)) { + if (uart_circ_empty(xmit)) pl011_stop_tx(&uap->port); - goto done; - } - - if (unlikely(!uap->tx_irq_seen)) - schedule_delayed_work(&uap->tx_softirq_work, uap->port.timeout); - -done: - return false; } static void pl011_modem_status(struct uart_amba_port *uap) @@ -1354,28 +1297,6 @@ static void pl011_modem_status(struct uart_amba_port *uap) wake_up_interruptible(&uap->port.state->port.delta_msr_wait); } -static void pl011_tx_softirq(struct work_struct *work) -{ - struct delayed_work *dwork = to_delayed_work(work); - struct uart_amba_port *uap = - container_of(dwork, struct uart_amba_port, tx_softirq_work); - - spin_lock(&uap->port.lock); - while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); -} - -static void pl011_tx_irq_seen(struct uart_amba_port *uap) -{ - if (likely(uap->tx_irq_seen > 1)) - return; - - uap->tx_irq_seen++; - if (uap->tx_irq_seen < 2) - /* first TX IRQ */ - cancel_delayed_work(&uap->tx_softirq_work); -} - static irqreturn_t pl011_int(int irq, void *dev_id) { struct uart_amba_port *uap = dev_id; @@ -1414,10 +1335,8 @@ static irqreturn_t pl011_int(int irq, void *dev_id) if (status & (UART011_DSRMIS|UART011_DCDMIS| UART011_CTSMIS|UART011_RIMIS)) pl011_modem_status(uap); - if (status & UART011_TXIS) { - pl011_tx_irq_seen(uap); + if (status & UART011_TXIS) pl011_tx_chars(uap); - } if (pass_counter-- == 0) break; @@ -1621,7 +1540,7 @@ static int pl011_startup(struct uart_port *port) { struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); - unsigned int cr; + unsigned int cr, lcr_h, fbrd, ibrd; int retval; retval = pl011_hwinit(port); @@ -1639,11 +1558,30 @@ static int pl011_startup(struct uart_port *port) writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS); - /* Assume that TX IRQ doesn't work until we see one: */ - uap->tx_irq_seen = 0; - + /* + * Provoke TX FIFO interrupt into asserting. Taking care to preserve + * baud rate and data format specified by FBRD, IBRD and LCRH as the + * UART may already be in use as a console. + */ spin_lock_irq(&uap->port.lock); + fbrd = readw(uap->port.membase + UART011_FBRD); + ibrd = readw(uap->port.membase + UART011_IBRD); + lcr_h = readw(uap->port.membase + uap->lcrh_rx); + + cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE; + writew(cr, uap->port.membase + UART011_CR); + writew(0, uap->port.membase + UART011_FBRD); + writew(1, uap->port.membase + UART011_IBRD); + pl011_write_lcr_h(uap, 0); + writew(0, uap->port.membase + UART01x_DR); + while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY) + barrier(); + + writew(fbrd, uap->port.membase + UART011_FBRD); + writew(ibrd, uap->port.membase + UART011_IBRD); + pl011_write_lcr_h(uap, lcr_h); + /* restore RTS and DTR */ cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR); cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE; @@ -1697,8 +1635,6 @@ static void pl011_shutdown(struct uart_port *port) container_of(port, struct uart_amba_port, port); unsigned int cr; - cancel_delayed_work_sync(&uap->tx_softirq_work); - /* * disable all interrupts */ @@ -2245,7 +2181,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->port.ops = &amba_pl011_pops; uap->port.flags = UPF_BOOT_AUTOCONF; uap->port.line = i; - INIT_DELAYED_WORK(&uap->tx_softirq_work, pl011_tx_softirq); /* Ensure interrupts from this UART are masked and cleared */ writew(0, uap->port.membase + UART011_IMSC); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH] serial/amba-pl011: Disable interrupts around TX softirq @ 2015-06-15 14:03 ` Dave Martin 0 siblings, 0 replies; 33+ messages in thread From: Dave Martin @ 2015-06-15 14:03 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 15, 2015 at 12:09:59PM +0100, Andre Przywara wrote: [...] > On 06/13/2015 01:39 AM, Greg Kroah-Hartman wrote: [...] > > Too late for 4.1, sorry. > > So if I get this correctly, we go from "some characters lost from > systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? > Is that right? So it seems. It's my bad that we hit this so late in the cycle. > I understand that you are reluctant to take this patch this late in the > game, but wouldn't it actually be better now to revert PL011 to the > state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 > release? > We could still backport the solution ending up in 4.2-rc to 4.1.x. If neither this patch, nor what is in -next, can be applied in 4.1 (and I understand where Greg was coming from in saying "no"), then this seems inevitable. The only other alternative that might be viable is to revert the relevant changes from 4.1 as you suggest, and optionally try to fix the systemd console corruption issue via stable. However, all the fixes currently available involve a lot of code -- I'm not sure whether they are good candidates for stable. There's also a risk of getting into a worse mess with that late revert if it interacts with any other patches. However, the revert [1] doesn't look complicated. There is a 1-line context clash with some of the dma changes that needs to be resolved, but that's it. Opinions? To avoid further confusion I'll hold off on sending any further patch for the moment -- I'd like to see some sort of consensus on the way forward if possible. Cheers ---Dave [1] For me, I can build and boot Juno with the v4.0 behaviour with the following reverts. These would clash with next again, but the resolution is the same as before: the code in -next overrides these changes in master. >From 89cd9714f223117389f24b191ff97f6969d81c3c Mon Sep 17 00:00:00 2001 From: Dave Martin <Dave.Martin@arm.com> Date: Mon, 15 Jun 2015 14:06:25 +0100 Subject: [REVERT 1/2] Revert "serial/amba-pl011: Unconditionally poll for FIFO space before each TX char" This reverts commit 43dd1f9a5b05d6db2cb258354a01ace63baa5c0b. --- drivers/tty/serial/amba-pl011.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 763eb20..6f5a072 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1249,19 +1249,20 @@ __acquires(&uap->port.lock) /* * Transmit a character + * There must be at least one free entry in the TX FIFO to accept the char. * - * Returns true if the character was successfully queued to the FIFO. - * Returns false otherwise. + * Returns true if the FIFO might have space in it afterwards; + * returns false if the FIFO definitely became full. */ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c) { - if (readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF) - return false; /* unable to transmit character */ - writew(c, uap->port.membase + UART01x_DR); uap->port.icount.tx++; - return true; + if (likely(uap->tx_irq_seen > 1)) + return true; + + return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF); } static bool pl011_tx_chars(struct uart_amba_port *uap) @@ -1295,8 +1296,7 @@ static bool pl011_tx_chars(struct uart_amba_port *uap) return false; if (uap->port.x_char) { - if (!pl011_tx_char(uap, uap->port.x_char)) - goto done; + pl011_tx_char(uap, uap->port.x_char); uap->port.x_char = 0; --count; } -- 1.7.10.4 >From ece86a450686f37c3761a25c64af013bc556ee61 Mon Sep 17 00:00:00 2001 From: Dave Martin <Dave.Martin@arm.com> Date: Mon, 15 Jun 2015 14:07:39 +0100 Subject: [REVERT 2/2] Revert "serial/amba-pl011: Activate TX IRQ passively" This reverts commit 734745caeb9f155ab58918834a8c70e83fa6afd3. Conflicts: drivers/tty/serial/amba-pl011.c --- drivers/tty/serial/amba-pl011.c | 165 ++++++++++++--------------------------- 1 file changed, 50 insertions(+), 115 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 6f5a072..a9074c0 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -58,7 +58,6 @@ #include <linux/pinctrl/consumer.h> #include <linux/sizes.h> #include <linux/io.h> -#include <linux/workqueue.h> #define UART_NR 14 @@ -157,9 +156,7 @@ struct uart_amba_port { unsigned int lcrh_tx; /* vendor-specific */ unsigned int lcrh_rx; /* vendor-specific */ unsigned int old_cr; /* state during shutdown */ - struct delayed_work tx_softirq_work; bool autorts; - unsigned int tx_irq_seen; /* 0=none, 1=1, 2=2 or more */ char type[12]; #ifdef CONFIG_DMA_ENGINE /* DMA stuff */ @@ -403,9 +400,8 @@ static void pl011_dma_remove(struct uart_amba_port *uap) dma_release_channel(uap->dmarx.chan); } -/* Forward declare these for the refill routine */ +/* Forward declare this for the refill routine */ static int pl011_dma_tx_refill(struct uart_amba_port *uap); -static void pl011_start_tx_pio(struct uart_amba_port *uap); /* * The current DMA TX buffer has been sent. @@ -443,13 +439,14 @@ static void pl011_dma_tx_callback(void *data) return; } - if (pl011_dma_tx_refill(uap) <= 0) + if (pl011_dma_tx_refill(uap) <= 0) { /* * We didn't queue a DMA buffer for some reason, but we * have data pending to be sent. Re-enable the TX IRQ. */ - pl011_start_tx_pio(uap); - + uap->im |= UART011_TXIM; + writew(uap->im, uap->port.membase + UART011_IMSC); + } spin_unlock_irqrestore(&uap->port.lock, flags); } @@ -627,10 +624,12 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap) if (!uap->dmatx.queued) { if (pl011_dma_tx_refill(uap) > 0) { uap->im &= ~UART011_TXIM; - writew(uap->im, uap->port.membase + - UART011_IMSC); - } else + ret = true; + } else { + uap->im |= UART011_TXIM; ret = false; + } + writew(uap->im, uap->port.membase + UART011_IMSC); } else if (!(uap->dmacr & UART011_TXDMAE)) { uap->dmacr |= UART011_TXDMAE; writew(uap->dmacr, @@ -1172,24 +1171,15 @@ static void pl011_stop_tx(struct uart_port *port) pl011_dma_tx_stop(uap); } -static bool pl011_tx_chars(struct uart_amba_port *uap); - -/* Start TX with programmed I/O only (no DMA) */ -static void pl011_start_tx_pio(struct uart_amba_port *uap) -{ - uap->im |= UART011_TXIM; - writew(uap->im, uap->port.membase + UART011_IMSC); - if (!uap->tx_irq_seen) - pl011_tx_chars(uap); -} - static void pl011_start_tx(struct uart_port *port) { struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); - if (!pl011_dma_tx_start(uap)) - pl011_start_tx_pio(uap); + if (!pl011_dma_tx_start(uap)) { + uap->im |= UART011_TXIM; + writew(uap->im, uap->port.membase + UART011_IMSC); + } } static void pl011_stop_rx(struct uart_port *port) @@ -1247,87 +1237,40 @@ __acquires(&uap->port.lock) spin_lock(&uap->port.lock); } -/* - * Transmit a character - * There must be at least one free entry in the TX FIFO to accept the char. - * - * Returns true if the FIFO might have space in it afterwards; - * returns false if the FIFO definitely became full. - */ -static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c) -{ - writew(c, uap->port.membase + UART01x_DR); - uap->port.icount.tx++; - - if (likely(uap->tx_irq_seen > 1)) - return true; - - return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF); -} - -static bool pl011_tx_chars(struct uart_amba_port *uap) +static void pl011_tx_chars(struct uart_amba_port *uap) { struct circ_buf *xmit = &uap->port.state->xmit; int count; - if (unlikely(uap->tx_irq_seen < 2)) - /* - * Initial FIFO fill level unknown: we must check TXFF - * after each write, so just try to fill up the FIFO. - */ - count = uap->fifosize; - else /* tx_irq_seen >= 2 */ - /* - * FIFO initially at least half-empty, so we can simply - * write half the FIFO without polling TXFF. - - * Note: the *first* TX IRQ can still race with - * pl011_start_tx_pio(), which can result in the FIFO - * being fuller than expected in that case. - */ - count = uap->fifosize >> 1; - - /* - * If the FIFO is full we're guaranteed a TX IRQ at some later point, - * and can't transmit immediately in any case: - */ - if (unlikely(uap->tx_irq_seen < 2 && - readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF)) - return false; - if (uap->port.x_char) { - pl011_tx_char(uap, uap->port.x_char); + writew(uap->port.x_char, uap->port.membase + UART01x_DR); + uap->port.icount.tx++; uap->port.x_char = 0; - --count; + return; } if (uart_circ_empty(xmit) || uart_tx_stopped(&uap->port)) { pl011_stop_tx(&uap->port); - goto done; + return; } /* If we are using DMA mode, try to send some characters. */ if (pl011_dma_tx_irq(uap)) - goto done; + return; - while (count-- > 0 && pl011_tx_char(uap, xmit->buf[xmit->tail])) { + count = uap->fifosize >> 1; + do { + writew(xmit->buf[xmit->tail], uap->port.membase + UART01x_DR); xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); + uap->port.icount.tx++; if (uart_circ_empty(xmit)) break; - } + } while (--count > 0); if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) uart_write_wakeup(&uap->port); - if (uart_circ_empty(xmit)) { + if (uart_circ_empty(xmit)) pl011_stop_tx(&uap->port); - goto done; - } - - if (unlikely(!uap->tx_irq_seen)) - schedule_delayed_work(&uap->tx_softirq_work, uap->port.timeout); - -done: - return false; } static void pl011_modem_status(struct uart_amba_port *uap) @@ -1354,28 +1297,6 @@ static void pl011_modem_status(struct uart_amba_port *uap) wake_up_interruptible(&uap->port.state->port.delta_msr_wait); } -static void pl011_tx_softirq(struct work_struct *work) -{ - struct delayed_work *dwork = to_delayed_work(work); - struct uart_amba_port *uap = - container_of(dwork, struct uart_amba_port, tx_softirq_work); - - spin_lock(&uap->port.lock); - while (pl011_tx_chars(uap)) ; - spin_unlock(&uap->port.lock); -} - -static void pl011_tx_irq_seen(struct uart_amba_port *uap) -{ - if (likely(uap->tx_irq_seen > 1)) - return; - - uap->tx_irq_seen++; - if (uap->tx_irq_seen < 2) - /* first TX IRQ */ - cancel_delayed_work(&uap->tx_softirq_work); -} - static irqreturn_t pl011_int(int irq, void *dev_id) { struct uart_amba_port *uap = dev_id; @@ -1414,10 +1335,8 @@ static irqreturn_t pl011_int(int irq, void *dev_id) if (status & (UART011_DSRMIS|UART011_DCDMIS| UART011_CTSMIS|UART011_RIMIS)) pl011_modem_status(uap); - if (status & UART011_TXIS) { - pl011_tx_irq_seen(uap); + if (status & UART011_TXIS) pl011_tx_chars(uap); - } if (pass_counter-- == 0) break; @@ -1621,7 +1540,7 @@ static int pl011_startup(struct uart_port *port) { struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); - unsigned int cr; + unsigned int cr, lcr_h, fbrd, ibrd; int retval; retval = pl011_hwinit(port); @@ -1639,11 +1558,30 @@ static int pl011_startup(struct uart_port *port) writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS); - /* Assume that TX IRQ doesn't work until we see one: */ - uap->tx_irq_seen = 0; - + /* + * Provoke TX FIFO interrupt into asserting. Taking care to preserve + * baud rate and data format specified by FBRD, IBRD and LCRH as the + * UART may already be in use as a console. + */ spin_lock_irq(&uap->port.lock); + fbrd = readw(uap->port.membase + UART011_FBRD); + ibrd = readw(uap->port.membase + UART011_IBRD); + lcr_h = readw(uap->port.membase + uap->lcrh_rx); + + cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE; + writew(cr, uap->port.membase + UART011_CR); + writew(0, uap->port.membase + UART011_FBRD); + writew(1, uap->port.membase + UART011_IBRD); + pl011_write_lcr_h(uap, 0); + writew(0, uap->port.membase + UART01x_DR); + while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY) + barrier(); + + writew(fbrd, uap->port.membase + UART011_FBRD); + writew(ibrd, uap->port.membase + UART011_IBRD); + pl011_write_lcr_h(uap, lcr_h); + /* restore RTS and DTR */ cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR); cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE; @@ -1697,8 +1635,6 @@ static void pl011_shutdown(struct uart_port *port) container_of(port, struct uart_amba_port, port); unsigned int cr; - cancel_delayed_work_sync(&uap->tx_softirq_work); - /* * disable all interrupts */ @@ -2245,7 +2181,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->port.ops = &amba_pl011_pops; uap->port.flags = UPF_BOOT_AUTOCONF; uap->port.line = i; - INIT_DELAYED_WORK(&uap->tx_softirq_work, pl011_tx_softirq); /* Ensure interrupts from this UART are masked and cleared */ writew(0, uap->port.membase + UART011_IMSC); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 33+ messages in thread
end of thread, other threads:[~2015-07-24 16:46 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-09 10:57 [PATCH] serial/amba-pl011: Disable interrupts around TX softirq Dave Martin 2015-07-09 10:57 ` Dave Martin 2015-07-23 22:05 ` Greg Kroah-Hartman 2015-07-23 22:05 ` Greg Kroah-Hartman 2015-07-24 9:56 ` Dave Martin 2015-07-24 9:56 ` Dave Martin 2015-07-24 15:15 ` Greg Kroah-Hartman 2015-07-24 15:15 ` Greg Kroah-Hartman 2015-07-24 16:46 ` Dave Martin 2015-07-24 16:46 ` Dave Martin -- strict thread matches above, loose matches on Subject: below -- 2015-06-18 13:54 Dave Martin 2015-06-18 13:54 ` Dave Martin 2015-06-20 9:09 ` Stefan Wahren 2015-06-20 9:09 ` Stefan Wahren 2015-06-20 9:09 ` Stefan Wahren 2015-06-23 11:13 ` Dave P Martin 2015-06-23 11:13 ` Dave P Martin 2015-06-05 14:07 Dave Martin 2015-06-05 14:07 ` Dave Martin 2015-06-05 18:03 ` Robin Murphy 2015-06-05 18:03 ` Robin Murphy 2015-06-08 10:34 ` Robin Murphy 2015-06-08 10:34 ` Robin Murphy 2015-06-08 11:34 ` Dave P Martin 2015-06-08 11:34 ` Dave P Martin 2015-06-13 0:39 ` Greg Kroah-Hartman 2015-06-13 0:39 ` Greg Kroah-Hartman 2015-06-15 10:53 ` Dave P Martin 2015-06-15 10:53 ` Dave P Martin 2015-06-15 11:09 ` Andre Przywara 2015-06-15 11:09 ` Andre Przywara 2015-06-15 14:03 ` Dave Martin 2015-06-15 14:03 ` Dave Martin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.