diff mbox

[10/11] Revert "serial: omap: unlock the port lock"

Message ID 1395343807-21618-10-git-send-email-balbi@ti.com
State Accepted
Commit 6bf789672ee8387fda08af1deeffd12126e60659
Headers show

Commit Message

Felipe Balbi March 20, 2014, 7:30 p.m. UTC
This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.

That commit tried to fix a deadlock problem when using
hci_ldisc, but it turns out the bug was in hci_ldsic
all along where it was calling ->write() from within
->write_wakeup() callback.

The problem is that ->write_wakeup() was called with
port lock held and ->write() tried to grab the same
port lock.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Tony Lindgren March 25, 2014, 6:28 p.m. UTC | #1
* Felipe Balbi <balbi@ti.com> [140320 12:39]:
> This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
> 
> That commit tried to fix a deadlock problem when using
> hci_ldisc, but it turns out the bug was in hci_ldsic
> all along where it was calling ->write() from within
> ->write_wakeup() callback.
> 
> The problem is that ->write_wakeup() was called with
> port lock held and ->write() tried to grab the same
> port lock.

Should this and the next patch be earlier in the series
as a fix for the v3.15-rc cycle? Should they be cc: stable
as well?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Peter Hurley March 27, 2014, 12:39 a.m. UTC | #2
On 03/25/2014 02:28 PM, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [140320 12:39]:
>> This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
>>
>> That commit tried to fix a deadlock problem when using
>> hci_ldisc, but it turns out the bug was in hci_ldsic
>> all along where it was calling ->write() from within
>> ->write_wakeup() callback.
>>
>> The problem is that ->write_wakeup() was called with
>> port lock held and ->write() tried to grab the same
>> port lock.
>
> Should this and the next patch be earlier in the series
> as a fix for the v3.15-rc cycle? Should they be cc: stable
> as well?

Well, right now the other fix has had _zero_ testing
so not really a -stable candidate just yet.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Felipe Balbi March 27, 2014, 2:10 a.m. UTC | #3
Hi,

On Wed, Mar 26, 2014 at 08:39:11PM -0400, Peter Hurley wrote:
> On 03/25/2014 02:28 PM, Tony Lindgren wrote:
> >* Felipe Balbi <balbi@ti.com> [140320 12:39]:
> >>This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
> >>
> >>That commit tried to fix a deadlock problem when using
> >>hci_ldisc, but it turns out the bug was in hci_ldsic
> >>all along where it was calling ->write() from within
> >>->write_wakeup() callback.
> >>
> >>The problem is that ->write_wakeup() was called with
> >>port lock held and ->write() tried to grab the same
> >>port lock.
> >
> >Should this and the next patch be earlier in the series
> >as a fix for the v3.15-rc cycle? Should they be cc: stable
> >as well?
> 
> Well, right now the other fix has had _zero_ testing
> so not really a -stable candidate just yet.

how can you even say that ? Unless you work for some 3 letter acronym
organizations, you have no clue about the fact that this was tested on a
keystone 2 platform. How else would we have found the issue to start
with ?
Peter Hurley March 27, 2014, 2:27 a.m. UTC | #4
On 03/26/2014 10:10 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Mar 26, 2014 at 08:39:11PM -0400, Peter Hurley wrote:
>> On 03/25/2014 02:28 PM, Tony Lindgren wrote:
>>> * Felipe Balbi <balbi@ti.com> [140320 12:39]:
>>>> This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
>>>>
>>>> That commit tried to fix a deadlock problem when using
>>>> hci_ldisc, but it turns out the bug was in hci_ldsic
>>>> all along where it was calling ->write() from within
>>>> ->write_wakeup() callback.
>>>>
>>>> The problem is that ->write_wakeup() was called with
>>>> port lock held and ->write() tried to grab the same
>>>> port lock.
>>>
>>> Should this and the next patch be earlier in the series
>>> as a fix for the v3.15-rc cycle? Should they be cc: stable
>>> as well?
>>
>> Well, right now the other fix has had _zero_ testing
>> so not really a -stable candidate just yet.
>
> how can you even say that ?

I misunderstood when you wrote:

On 03/20/2014 02:11 PM, Felipe Balbi wrote:
 > here's a build-tested only patch which is waiting for testing from other
 > colleagues who've got a platform to reproduce the problem:

and then the version I reviewed had no Tested-by: tags.

> Unless you work for some 3 letter acronym
> organizations, you have no clue about the fact that this was tested on a
> keystone 2 platform.

Ok.

> How else would we have found the issue to start with ?

Bug report?

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi March 27, 2014, 3:37 a.m. UTC | #5
Hi,

On Wed, Mar 26, 2014 at 10:27:13PM -0400, Peter Hurley wrote:
> On 03/26/2014 10:10 PM, Felipe Balbi wrote:
> >Hi,
> >
> >On Wed, Mar 26, 2014 at 08:39:11PM -0400, Peter Hurley wrote:
> >>On 03/25/2014 02:28 PM, Tony Lindgren wrote:
> >>>* Felipe Balbi <balbi@ti.com> [140320 12:39]:
> >>>>This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
> >>>>
> >>>>That commit tried to fix a deadlock problem when using
> >>>>hci_ldisc, but it turns out the bug was in hci_ldsic
> >>>>all along where it was calling ->write() from within
> >>>>->write_wakeup() callback.
> >>>>
> >>>>The problem is that ->write_wakeup() was called with
> >>>>port lock held and ->write() tried to grab the same
> >>>>port lock.
> >>>
> >>>Should this and the next patch be earlier in the series
> >>>as a fix for the v3.15-rc cycle? Should they be cc: stable
> >>>as well?
> >>
> >>Well, right now the other fix has had _zero_ testing
> >>so not really a -stable candidate just yet.
> >
> >how can you even say that ?
> 
> I misunderstood when you wrote:
> 
> On 03/20/2014 02:11 PM, Felipe Balbi wrote:
> > here's a build-tested only patch which is waiting for testing from other
> > colleagues who've got a platform to reproduce the problem:
> 
> and then the version I reviewed had no Tested-by: tags.

I wouldn't add that tag myself, but Murali (in Cc) did help testing
together with other colleagues.

> >How else would we have found the issue to start with ?
> 
> Bug report?

touchè :-)
Murali Karicheri March 27, 2014, 2:54 p.m. UTC | #6
>-----Original Message-----
>From: Balbi, Felipe
>Sent: Wednesday, March 26, 2014 11:37 PM
>To: Peter Hurley
>Cc: Balbi, Felipe; Tony Lindgren; Greg KH; linux-serial@vger.kernel.org; linux-
>bluetooth@vger.kernel.org; Karicheri, Muralidharan; b32955@freescale.com; Linux OMAP
>Mailing List; Linux Kernel Mailing List
>Subject: Re: [PATCH 10/11] Revert "serial: omap: unlock the port lock"
>
>Hi,
>
>On Wed, Mar 26, 2014 at 10:27:13PM -0400, Peter Hurley wrote:
>> On 03/26/2014 10:10 PM, Felipe Balbi wrote:
>> >Hi,
>> >
>> >On Wed, Mar 26, 2014 at 08:39:11PM -0400, Peter Hurley wrote:
>> >>On 03/25/2014 02:28 PM, Tony Lindgren wrote:
>> >>>* Felipe Balbi <balbi@ti.com> [140320 12:39]:
>> >>>>This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
>> >>>>
>> >>>>That commit tried to fix a deadlock problem when using hci_ldisc,
>> >>>>but it turns out the bug was in hci_ldsic all along where it was
>> >>>>calling ->write() from within
>> >>>>->write_wakeup() callback.
>> >>>>
>> >>>>The problem is that ->write_wakeup() was called with port lock
>> >>>>held and ->write() tried to grab the same port lock.
>> >>>
>> >>>Should this and the next patch be earlier in the series as a fix
>> >>>for the v3.15-rc cycle? Should they be cc: stable as well?
>> >>
>> >>Well, right now the other fix has had _zero_ testing so not really a
>> >>-stable candidate just yet.
>> >
>> >how can you even say that ?
>>
>> I misunderstood when you wrote:
>>
>> On 03/20/2014 02:11 PM, Felipe Balbi wrote:
>> > here's a build-tested only patch which is waiting for testing from
>> > other colleagues who've got a platform to reproduce the problem:
>>
>> and then the version I reviewed had no Tested-by: tags.
>
>I wouldn't add that tag myself, but Murali (in Cc) did help testing together with other

Yes. One of our customer did the test as the problem was reported by the
customer. The deadlock fix patch from Balbi (hci_ldsic) fixed the issue 
and customer confirmed that the issue is fixed.

Murali

>colleagues.
>
>> >How else would we have found the issue to start with ?
>>
>> Bug report?
>
>touchè :-)
>
>--
>balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b092d3d..be10e7e 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -380,11 +380,8 @@  static void transmit_chars(struct uart_omap_port *up, unsigned int lsr)
 			break;
 	} while (--count > 0);
 
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&up->port.lock);
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&up->port);
-		spin_lock(&up->port.lock);
-	}
 
 	if (uart_circ_empty(xmit))
 		serial_omap_stop_tx(&up->port);