From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH v4 REPOST] Revert "serial/amba-pl011: Leave the TX IRQ alone when the UART is not open" Date: Mon, 11 May 2015 17:09:30 +0100 Message-ID: <5550D43A.4090200@arm.com> References: <1431090470-1394-1-git-send-email-Dave.Martin@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1431090470-1394-1-git-send-email-Dave.Martin@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Greg Kroah-Hartman Cc: Russell King , =?UTF-8?B?SmFrdWIgS2ljacWEc2tp?= , Andrew Jackson , Graeme Gregory , "linux-serial@vger.kernel.org" , popcorn mix , Jorge Ramirez-Ortiz , Dave P Martin , "linux-arm-kernel@lists.infradead.org" List-Id: linux-serial@vger.kernel.org SGkgR3JlZywKCk9uIDA1LzA4LzIwMTUgMDI6MDcgUE0sIERhdmUgUCBNYXJ0aW4gd3JvdGU6Cj4g VGhpcyByZXZlcnRzIGNvbW1pdCBmMmVlNmRmYTBlODU5N2VlYThiOThkMjQwYjAwMzM5OTRlMjBk MjE1Lgo+IAo+IEpha3ViIEtpY2nFhHNraSBvYnNlcnZlZCB0aGF0IHRoaXMgcGF0Y2ggY2FuIGNh dXNlIHRoZSBwbDAxMQo+IGRyaXZlciB0byBoYW5nIGlmIGlmIHRoZSBvbmx5IHByb2Nlc3Mgd2l0 aCBhIHBsMDExIHBvcnQgb3BlbiBpcwo+IGtpbGxlZCBieSBhIHNpZ25hbCwgcGwwMTFfc2h1dGRv d24oKSBjYW4gZ2V0IGNhbGxlZCB3aXRoIGFuCj4gYXJiaXRyYXJ5IGFtb3VudCBvZiBkYXRhIHN0 aWxsIGluIHRoZSBGSUZPLgo+IAo+IENhbGxpbmcgX3NodXRkb3duKCkgd2l0aCB0aGUgVFggRklG TyBub24tZW1wdHkgaXMgcXVlc3Rpb25hYmxlCj4gYmVoYXZpb3VyIGFuZCBteSBpdHNlbGYgYmUg YSBidWcuCj4gCj4gU2luY2UgdGhlIGFmZmVjdGVkIHBhdGNoIHdhcyBzcGVjdWxhdGl2ZSBhbnl3 YXksIGFuZCBicmluZ3MgbGltaXRlZAo+IGJlbmVmaXQsIHRoZSBzaW1wbGVzdCBjb3Vyc2UgaXMg dG8gcmVtb3ZlIHRoZSBhc3N1bXB0aW9uIHRoYXQgVFhJUwo+IHdpbGwgYWx3YXlzIGJlIGxlZnQg YXNzZXJ0ZWQgYWZ0ZXIgdGhlIHBvcnQgaXMgc2h1dCBkb3duLgoKSSBzZWUgdGhhdCB5b3UgaGF2 ZSBxdWV1ZWQgaXQgZm9yIExpbnVzIGFscmVhZHksIHNvIEkgZ3Vlc3MgaXQgd2lsbCBoaXQKNC4x LXJjNCBhbnl3YXksIGJ1dCBqdXN0IHRvIGdpdmUgbW9yZSByYXRpb25hbGU6ClRoaXMgcmV2ZXJ0 IGZpeGVzIGEgcmVib290IGlzc3VlIHdpdGggUEwwMTEgYW5kIHN5c3RlbWQgb24gdGhlIEFNRApT ZWF0dGxlIGJvYXJkLiBSZWJvb3QgKG9yIHNodXRkb3duKSB3YXMgZmluZSB3aXRoIDQuMCwgYnV0 IHN0YXJ0ZWQgdG8KaGFuZyBhZnRlciB0aGUgZmlyc3QgY2hhcmFjdGVycyBmb2xsb3dpbmcgdGhl IHJlYm9vdCBvciBzaHV0ZG93biBjb21tYW5kCndpdGggNC4xLXJjMS4gU28gdGhpcyB2ZXJ5IHBh dGNoIGFjdHVhbGx5IGZpeGVzIGEgNC4xIHJlZ3Jlc3Npb24gYW5kIGhhcwp0byBiZSBpbiB0aGUg cmVsZWFzZS4KU28gYWN0dWFsbHkgbm90aGluZyB0byBkbyBmb3IgeW91IChnaXZlbiB0aGF0IHR0 eS1saW51cyB3aWxsIGJlIHRoZQpicmFuY2ggZm9yIHlvdXIgbmV4dCA0LjEtcmMgcHVsbCByZXF1 ZXN0KSwganVzdCB0byBtYWtlIHN1cmUgdGhhdCB5b3UKZG9uJ3Qgc3RpbGwgZHJvcCB0aGUgcGF0 Y2guCgpDaGVlcnMsCkFuZHJlLgoKPiAKPiBTaWduZWQtb2ZmLWJ5OiBEYXZlIE1hcnRpbiA8RGF2 ZS5NYXJ0aW5AYXJtLmNvbT4KPiBTaWduZWQtb2ZmLWJ5OiBHcmVnIEtyb2FoLUhhcnRtYW4gPGdy ZWdraEBsaW51eGZvdW5kYXRpb24ub3JnPgo+IC0tLQo+IAo+ICBkcml2ZXJzL3R0eS9zZXJpYWwv YW1iYS1wbDAxMS5jIHwgICAgNSArKysrLQo+ICAxIGZpbGUgY2hhbmdlZCwgNCBpbnNlcnRpb25z KCspLCAxIGRlbGV0aW9uKC0pCj4gCj4gVGhlIG9yaWdpbmFsIHBhdGNoIHdhcyBhIG1pc2d1aWRl ZCBvcHRpbWlzYXRpb24gYW5kIGludHJvZHVjZXMgYSBidWcKPiB0aGF0IGNhbiBsZWFkIHRvIGRy aXZlciBkZWFkbG9jay4gIFNvbWUgcGVvcGxlIGFyZSBoaXR0aW5nIHRoaXMgaW4KPiB2NC4wLXJj MS8yLgo+IAo+IFRoZSByZXZlcnQgaXMgc3RyYWlnaHRmb3dhcmQ6IHBsZWFzZSBjb25zaWRlciBh cHBseWluZyBpdCBmb3IgdjQuMS4KPiAKPiBUaGlzIHJldmVydCBoYXMgYWxyZWFkeSBiZWVuIHBv c3RlZCB0byBsaW51eC1zZXJpYWwgYW5kIGFwcGxpZWQgaW4KPiB0dHktbmV4dCwgYnV0IGlzIG5v dCB5ZXQgc2NoZWR1bGVkIGZvciB2NC4xLgo+IAo+IFRoYW5rcwo+IC0tLURhdmUKPiAKPiBkaWZm IC0tZ2l0IGEvZHJpdmVycy90dHkvc2VyaWFsL2FtYmEtcGwwMTEuYyBiL2RyaXZlcnMvdHR5L3Nl cmlhbC9hbWJhLXBsMDExLmMKPiBpbmRleCA1YTRlOWQ1Li42ZjVhMDcyIDEwMDY0NAo+IC0tLSBh L2RyaXZlcnMvdHR5L3NlcmlhbC9hbWJhLXBsMDExLmMKPiArKysgYi9kcml2ZXJzL3R0eS9zZXJp YWwvYW1iYS1wbDAxMS5jCj4gQEAgLTE2MzksNiArMTYzOSw5IEBAIHN0YXRpYyBpbnQgcGwwMTFf c3RhcnR1cChzdHJ1Y3QgdWFydF9wb3J0ICpwb3J0KQo+ICAKPiAgCXdyaXRldyh1YXAtPnZlbmRv ci0+aWZscywgdWFwLT5wb3J0Lm1lbWJhc2UgKyBVQVJUMDExX0lGTFMpOwo+ICAKPiArCS8qIEFz c3VtZSB0aGF0IFRYIElSUSBkb2Vzbid0IHdvcmsgdW50aWwgd2Ugc2VlIG9uZTogKi8KPiArCXVh cC0+dHhfaXJxX3NlZW4gPSAwOwo+ICsKPiAgCXNwaW5fbG9ja19pcnEoJnVhcC0+cG9ydC5sb2Nr KTsKPiAgCj4gIAkvKiByZXN0b3JlIFJUUyBhbmQgRFRSICovCj4gQEAgLTE3MDIsNyArMTcwNSw3 IEBAIHN0YXRpYyB2b2lkIHBsMDExX3NodXRkb3duKHN0cnVjdCB1YXJ0X3BvcnQgKnBvcnQpCj4g IAlzcGluX2xvY2tfaXJxKCZ1YXAtPnBvcnQubG9jayk7Cj4gIAl1YXAtPmltID0gMDsKPiAgCXdy aXRldyh1YXAtPmltLCB1YXAtPnBvcnQubWVtYmFzZSArIFVBUlQwMTFfSU1TQyk7Cj4gLQl3cml0 ZXcoMHhmZmZmICYgflVBUlQwMTFfVFhJUywgdWFwLT5wb3J0Lm1lbWJhc2UgKyBVQVJUMDExX0lD Uik7Cj4gKwl3cml0ZXcoMHhmZmZmLCB1YXAtPnBvcnQubWVtYmFzZSArIFVBUlQwMTFfSUNSKTsK PiAgCXNwaW5fdW5sb2NrX2lycSgmdWFwLT5wb3J0LmxvY2spOwo+ICAKPiAgCXBsMDExX2RtYV9z aHV0ZG93bih1YXApOwo+IAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBs aXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlz dGluZm8vbGludXgtYXJtLWtlcm5lbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: andre.przywara@arm.com (Andre Przywara) Date: Mon, 11 May 2015 17:09:30 +0100 Subject: [PATCH v4 REPOST] Revert "serial/amba-pl011: Leave the TX IRQ alone when the UART is not open" In-Reply-To: <1431090470-1394-1-git-send-email-Dave.Martin@arm.com> References: <1431090470-1394-1-git-send-email-Dave.Martin@arm.com> Message-ID: <5550D43A.4090200@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Greg, On 05/08/2015 02:07 PM, Dave P Martin wrote: > This reverts commit f2ee6dfa0e8597eea8b98d240b0033994e20d215. > > Jakub Kici?ski observed that this patch can cause the pl011 > driver to hang if if the only process with a pl011 port open is > killed by a signal, pl011_shutdown() can get called with an > arbitrary amount of data still in the FIFO. > > Calling _shutdown() with the TX FIFO non-empty is questionable > behaviour and my itself be a bug. > > Since the affected patch was speculative anyway, and brings limited > benefit, the simplest course is to remove the assumption that TXIS > will always be left asserted after the port is shut down. I see that you have queued it for Linus already, so I guess it will hit 4.1-rc4 anyway, but just to give more rationale: This revert fixes a reboot issue with PL011 and systemd on the AMD Seattle board. Reboot (or shutdown) was fine with 4.0, but started to hang after the first characters following the reboot or shutdown command with 4.1-rc1. So this very patch actually fixes a 4.1 regression and has to be in the release. So actually nothing to do for you (given that tty-linus will be the branch for your next 4.1-rc pull request), just to make sure that you don't still drop the patch. Cheers, Andre. > > Signed-off-by: Dave Martin > Signed-off-by: Greg Kroah-Hartman > --- > > drivers/tty/serial/amba-pl011.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > The original patch was a misguided optimisation and introduces a bug > that can lead to driver deadlock. Some people are hitting this in > v4.0-rc1/2. > > The revert is straightfoward: please consider applying it for v4.1. > > This revert has already been posted to linux-serial and applied in > tty-next, but is not yet scheduled for v4.1. > > Thanks > ---Dave > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 5a4e9d5..6f5a072 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -1639,6 +1639,9 @@ 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; > + > spin_lock_irq(&uap->port.lock); > > /* restore RTS and DTR */ > @@ -1702,7 +1705,7 @@ static void pl011_shutdown(struct uart_port *port) > spin_lock_irq(&uap->port.lock); > uap->im = 0; > writew(uap->im, uap->port.membase + UART011_IMSC); > - writew(0xffff & ~UART011_TXIS, uap->port.membase + UART011_ICR); > + writew(0xffff, uap->port.membase + UART011_ICR); > spin_unlock_irq(&uap->port.lock); > > pl011_dma_shutdown(uap); >