per CodingStyle we should have those braces, no
functional changes.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 77f0351..73abba8 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1677,8 +1677,10 @@ static int serial_omap_probe(struct platform_device *pdev)
omap_up_info->DTR_present) {
up->DTR_gpio = omap_up_info->DTR_gpio;
up->DTR_inverted = omap_up_info->DTR_inverted;
- } else
+ } else {
up->DTR_gpio = -EINVAL;
+ }
+
up->DTR_active = 0;
up->dev = &pdev->dev;
@@ -1740,6 +1742,7 @@ static int serial_omap_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, up);
if (omap_up_info->autosuspend_timeout == 0)
omap_up_info->autosuspend_timeout = -1;
+
device_init_wakeup(up->dev, true);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
--
1.9.1.286.g5172cb3
>-----Original Message-----
>From: Balbi, Felipe
>Sent: Wednesday, March 26, 2014 11:37 PM
>To: Peter Hurley
>Cc: Balbi, Felipe; Tony Lindgren; Greg KH; [email protected]; linux-
>[email protected]; Karicheri, Muralidharan; [email protected]; 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 <[email protected]> [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
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 <[email protected]> [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? :-)
--
balbi
Hi,
On Wed, Mar 26, 2014 at 10:20:15PM -0400, Peter Hurley wrote:
> >>You may want to build on top of this patch split handling;
> >>I noticed some of the protocol drivers are calling
> >>hci_uart_tx_wakeup() from work functions already (so don't
> >>need to schedule another work...)
> >
> >I don't think that should be part of $subject, though.
>
> I don't understand what you mean here.
it seemed, at first, like you suggested to redo this patch modifying the
protocol drivers to avoid two workqueues. But now that I read it again
you _did_ write "on top of this patch".
--
balbi
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 <[email protected]> [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
On 03/26/2014 10:09 PM, Felipe Balbi wrote:
>> I just noticed this patch wasn't addressed to Marcel;
>> seems like this should go through the bluetooth tree (but not
>> through bluetooth-next because it fixes an oops).
>
> read the archives:
>
> http://marc.info/?l=linux-bluetooth&m=139534449409583&w=2
Sorry. I did actually get Marcel's reply but Thunderbird
didn't parent the reply properly in my inbox and I forgot about it.
>> Marcel,
>>
>> You may want to build on top of this patch split handling;
>> I noticed some of the protocol drivers are calling
>> hci_uart_tx_wakeup() from work functions already (so don't
>> need to schedule another work...)
>
> I don't think that should be part of $subject, though.
I don't understand what you mean here.
Regards,
Peter Hurley
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 <[email protected]> [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 ?
--
balbi
Hi,
On Wed, Mar 26, 2014 at 08:47:15PM -0400, Peter Hurley wrote:
> [ +to Marcel Holtmann ]
>
> On 03/20/2014 03:30 PM, Felipe Balbi wrote:
> >LDISCs shouldn't call tty->ops->write() from within
> >->write_wakeup().
> >
> >->write_wakeup() is called with port lock taken and
> >IRQs disabled, tty->ops->write() will try to acquire
> >the same port lock and we will deadlock.
> >
> >Reviewed-by: Peter Hurley <[email protected]>
> >Reported-by: Huang Shijie <[email protected]>
> >Signed-off-by: Felipe Balbi <[email protected]>
>
> I just noticed this patch wasn't addressed to Marcel;
> seems like this should go through the bluetooth tree (but not
> through bluetooth-next because it fixes an oops).
read the archives:
http://marc.info/?l=linux-bluetooth&m=139534449409583&w=2
> Marcel,
>
> You may want to build on top of this patch split handling;
> I noticed some of the protocol drivers are calling
> hci_uart_tx_wakeup() from work functions already (so don't
> need to schedule another work...)
I don't think that should be part of $subject, though.
--
balbi
[ +to Marcel Holtmann ]
On 03/20/2014 03:30 PM, Felipe Balbi wrote:
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
>
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
>
> Reviewed-by: Peter Hurley <[email protected]>
> Reported-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
I just noticed this patch wasn't addressed to Marcel;
seems like this should go through the bluetooth tree (but not
through bluetooth-next because it fixes an oops).
Marcel,
You may want to build on top of this patch split handling;
I noticed some of the protocol drivers are calling
hci_uart_tx_wakeup() from work functions already (so don't
need to schedule another work...)
Regards,
Peter Hurley
> ---
> drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> drivers/bluetooth/hci_uart.h | 1 +
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 6e06f6f..77af52f 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>
> int hci_uart_tx_wakeup(struct hci_uart *hu)
> {
> - struct tty_struct *tty = hu->tty;
> - struct hci_dev *hdev = hu->hdev;
> - struct sk_buff *skb;
> -
> if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
> set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> return 0;
> @@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>
> BT_DBG("");
>
> + schedule_work(&hu->write_work);
> +
> + return 0;
> +}
> +
> +static void hci_uart_write_work(struct work_struct *work)
> +{
> + struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
> + struct tty_struct *tty = hu->tty;
> + struct hci_dev *hdev = hu->hdev;
> + struct sk_buff *skb;
> +
> + /* REVISIT: should we cope with bad skbs or ->write() returning
> + * and error value ?
> + */
> +
> restart:
> clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
>
> @@ -153,7 +165,6 @@ restart:
> goto restart;
>
> clear_bit(HCI_UART_SENDING, &hu->tx_state);
> - return 0;
> }
>
> static void hci_uart_init_work(struct work_struct *work)
> @@ -281,6 +292,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
> tty->receive_room = 65536;
>
> INIT_WORK(&hu->init_ready, hci_uart_init_work);
> + INIT_WORK(&hu->write_work, hci_uart_write_work);
>
> spin_lock_init(&hu->rx_lock);
>
> @@ -318,6 +330,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> if (hdev)
> hci_uart_close(hdev);
>
> + cancel_work_sync(&hu->write_work);
> +
> if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> if (hdev) {
> if (test_bit(HCI_UART_REGISTERED, &hu->flags))
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index fffa61f..12df101 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -68,6 +68,7 @@ struct hci_uart {
> unsigned long hdev_flags;
>
> struct work_struct init_ready;
> + struct work_struct write_work;
>
> struct hci_uart_proto *proto;
> void *priv;
>
On 03/25/2014 02:28 PM, Tony Lindgren wrote:
> * Felipe Balbi <[email protected]> [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.
* Felipe Balbi <[email protected]> [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
On Thu, Mar 20, 2014 at 12:40:40PM -0700, Marcel Holtmann wrote:
> Hi Felipe,
>
> > LDISCs shouldn't call tty->ops->write() from within
> > ->write_wakeup().
> >
> > ->write_wakeup() is called with port lock taken and
> > IRQs disabled, tty->ops->write() will try to acquire
> > the same port lock and we will deadlock.
> >
> > Reviewed-by: Peter Hurley <[email protected]>
> > Reported-by: Huang Shijie <[email protected]>
> > Signed-off-by: Felipe Balbi <[email protected]>
> > ---
> > drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> > drivers/bluetooth/hci_uart.h | 1 +
> > 2 files changed, 20 insertions(+), 5 deletions(-)
>
> I hope these are not causing any conflicts with bluetooth-next /
> linux-next. If not, then I can let Greg take it through tty-next tree.
>
> Acked-by: Marcel Holtmann <[email protected]>
tty-next is already closed, i'll rebase (if necessary) once -rc1 is out
;-)
cheers
--
balbi
Hi Felipe,
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
>
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
>
> Reviewed-by: Peter Hurley <[email protected]>
> Reported-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> drivers/bluetooth/hci_uart.h | 1 +
> 2 files changed, 20 insertions(+), 5 deletions(-)
I hope these are not causing any conflicts with bluetooth-next / linux-next. If not, then I can let Greg take it through tty-next tree.
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
Hi,
On Thu, Mar 20, 2014 at 02:30:04PM -0500, Felipe Balbi wrote:
> From: Huang Shijie <[email protected]>
>
> The Documentation/serial/driver tells us:
> -----------------------------------------------
> start_tx(port)
> Start transmitting characters.
>
> Locking: port->lock taken.
> Interrupts: locally disabled.
> -----------------------------------------------
>
> So when the uart_write_wakeup() is called, the port->lock is taken by
> the upper. See the following callstack:
>
> |_ uart_write_wakeup
> |_ tty_wakeup
> |_ ld->ops->write_wakeup
>
> With the port->lock held, we call the @write_wakeup. Some implemetation of
> the @write_wakeup does not notice that the port->lock is held, and it still
> tries to send data with uart_write() which will try to grab the prot->lock.
> A dead lock occurs, see the following log caught in the Bluetooth by uart:
>
> --------------------------------------------------------------------
> BUG: spinlock lockup suspected on CPU#0, swapper/0/0
> lock: 0xdc3f4410, .magic: dead4ead, .owner: swapper/0/0, .owner_cpu: 0
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.10.17-16839-ge4a1bef #1320
> [<80014cbc>] (unwind_backtrace+0x0/0x138) from [<8001251c>] (show_stack+0x10/0x14)
> [<8001251c>] (show_stack+0x10/0x14) from [<802816ac>] (do_raw_spin_lock+0x108/0x184)
> [<802816ac>] (do_raw_spin_lock+0x108/0x184) from [<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60)
> [<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60) from [<802f5754>] (uart_write+0x38/0xe0)
> [<802f5754>] (uart_write+0x38/0xe0) from [<80455270>] (hci_uart_tx_wakeup+0xa4/0x168)
> [<80455270>] (hci_uart_tx_wakeup+0xa4/0x168) from [<802dab18>] (tty_wakeup+0x50/0x5c)
> [<802dab18>] (tty_wakeup+0x50/0x5c) from [<802f81a4>] (imx_rtsint+0x50/0x80)
> [<802f81a4>] (imx_rtsint+0x50/0x80) from [<802f88f4>] (imx_int+0x158/0x17c)
> [<802f88f4>] (imx_int+0x158/0x17c) from [<8007abe0>] (handle_irq_event_percpu+0x50/0x194)
> [<8007abe0>] (handle_irq_event_percpu+0x50/0x194) from [<8007ad60>] (handle_irq_event+0x3c/0x5c)
> --------------------------------------------------------------------
>
> This patch adds more limits to the @write_wakeup, the one who wants to
> implemet the @write_wakeup should follow the limits which avoid the deadlock.
>
> Signed-off-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
patch came without a subject, Peter already sent it anyway, so Greg can
ignore this one.
--
balbi
cleanup only, no functional changes.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index db5d90b..6831425 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1624,10 +1624,13 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
static int serial_omap_probe(struct platform_device *pdev)
{
- struct uart_omap_port *up;
- struct resource *mem, *irq;
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
- int ret, uartirq = 0, wakeirq = 0;
+ struct uart_omap_port *up;
+ struct resource *mem;
+ struct resource *irq;
+ int uartirq = 0;
+ int wakeirq = 0;
+ int ret;
/* The optional wakeirq may be specified in the board dts file */
if (pdev->dev.of_node) {
--
1.9.1.286.g5172cb3
this way we can remove one pointer declaration.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6831425..d041060 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1627,7 +1627,6 @@ static int serial_omap_probe(struct platform_device *pdev)
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
struct uart_omap_port *up;
struct resource *mem;
- struct resource *irq;
int uartirq = 0;
int wakeirq = 0;
int ret;
@@ -1641,12 +1640,9 @@ static int serial_omap_probe(struct platform_device *pdev)
omap_up_info = of_get_uart_port_info(&pdev->dev);
pdev->dev.platform_data = omap_up_info;
} else {
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
- }
- uartirq = irq->start;
+ uartirq = platform_get_irq(pdev, 0);
+ if (uartirq < 0)
+ return -EPROBE_DEFER;
}
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
--
1.9.1.286.g5172cb3
just using helper function to remove some duplicated
code a bit. While at that, also move allocation of
struct uart_omap_port higher in the code so that
we return much earlier in case of no memory.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 32 +++++++++-----------------------
1 file changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index d041060..d785327 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1627,6 +1627,7 @@ static int serial_omap_probe(struct platform_device *pdev)
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
struct uart_omap_port *up;
struct resource *mem;
+ void __iomem *base;
int uartirq = 0;
int wakeirq = 0;
int ret;
@@ -1645,17 +1646,14 @@ static int serial_omap_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!mem) {
- dev_err(&pdev->dev, "no mem resource?\n");
- return -ENODEV;
- }
+ up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
+ if (!up)
+ return -ENOMEM;
- if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
- pdev->dev.driver->name)) {
- dev_err(&pdev->dev, "memory region already claimed\n");
- return -EBUSY;
- }
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
@@ -1669,10 +1667,6 @@ static int serial_omap_probe(struct platform_device *pdev)
return ret;
}
- up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
- if (!up)
- return -ENOMEM;
-
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
up->DTR_gpio = omap_up_info->DTR_gpio;
@@ -1715,14 +1709,7 @@ static int serial_omap_probe(struct platform_device *pdev)
sprintf(up->name, "OMAP UART%d", up->port.line);
up->port.mapbase = mem->start;
- up->port.membase = devm_ioremap(&pdev->dev, mem->start,
- resource_size(mem));
- if (!up->port.membase) {
- dev_err(&pdev->dev, "can't ioremap UART\n");
- ret = -ENOMEM;
- goto err_ioremap;
- }
-
+ up->port.membase = base;
up->port.flags = omap_up_info->flags;
up->port.uartclk = omap_up_info->uartclk;
if (!up->port.uartclk) {
@@ -1769,7 +1756,6 @@ static int serial_omap_probe(struct platform_device *pdev)
err_add_port:
pm_runtime_put(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err_ioremap:
err_rs485:
err_port_line:
dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
--
1.9.1.286.g5172cb3
nobody passes a DTR_gpio to this driver, so
this code is not necessary.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
1 file changed, 39 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index d785327..c79d3e6 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -163,10 +163,6 @@ struct uart_omap_port {
u8 wakeups_enabled;
u32 features;
- int DTR_gpio;
- int DTR_inverted;
- int DTR_active;
-
struct serial_rs485 rs485;
int rts_gpio;
@@ -685,16 +681,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
serial_out(up, UART_MCR, up->mcr);
pm_runtime_mark_last_busy(up->dev);
pm_runtime_put_autosuspend(up->dev);
-
- if (gpio_is_valid(up->DTR_gpio) &&
- !!(mctrl & TIOCM_DTR) != up->DTR_active) {
- up->DTR_active = !up->DTR_active;
- if (gpio_cansleep(up->DTR_gpio))
- schedule_work(&up->qos_work);
- else
- gpio_set_value(up->DTR_gpio,
- up->DTR_active != up->DTR_inverted);
- }
}
static void serial_omap_break_ctl(struct uart_port *port, int break_state)
@@ -838,9 +824,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
qos_work);
pm_qos_update_request(&up->pm_qos_request, up->latency);
- if (gpio_is_valid(up->DTR_gpio))
- gpio_set_value_cansleep(up->DTR_gpio,
- up->DTR_active != up->DTR_inverted);
}
static void
@@ -1655,28 +1638,6 @@ static int serial_omap_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- if (gpio_is_valid(omap_up_info->DTR_gpio) &&
- omap_up_info->DTR_present) {
- ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
- "omap-serial");
- if (ret < 0)
- return ret;
- ret = gpio_direction_output(omap_up_info->DTR_gpio,
- omap_up_info->DTR_inverted);
- if (ret < 0)
- return ret;
- }
-
- if (gpio_is_valid(omap_up_info->DTR_gpio) &&
- omap_up_info->DTR_present) {
- up->DTR_gpio = omap_up_info->DTR_gpio;
- up->DTR_inverted = omap_up_info->DTR_inverted;
- } else {
- up->DTR_gpio = -EINVAL;
- }
-
- up->DTR_active = 0;
-
up->dev = &pdev->dev;
up->port.dev = &pdev->dev;
up->port.type = PORT_OMAP;
--
1.9.1.286.g5172cb3
this will make sure gpio gets freed automatically
when this device is destroyed.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 73abba8..db5d90b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1594,7 +1594,7 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
/* check for tx enable gpio */
up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);
if (gpio_is_valid(up->rts_gpio)) {
- ret = gpio_request(up->rts_gpio, "omap-serial");
+ ret = devm_gpio_request(up->dev, up->rts_gpio, "omap-serial");
if (ret < 0)
return ret;
ret = gpio_direction_output(up->rts_gpio,
@@ -1660,7 +1660,8 @@ static int serial_omap_probe(struct platform_device *pdev)
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
- ret = gpio_request(omap_up_info->DTR_gpio, "omap-serial");
+ ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
+ "omap-serial");
if (ret < 0)
return ret;
ret = gpio_direction_output(omap_up_info->DTR_gpio,
--
1.9.1.286.g5172cb3
From: Huang Shijie <[email protected]>
The Documentation/serial/driver tells us:
-----------------------------------------------
start_tx(port)
Start transmitting characters.
Locking: port->lock taken.
Interrupts: locally disabled.
-----------------------------------------------
So when the uart_write_wakeup() is called, the port->lock is taken by
the upper. See the following callstack:
|_ uart_write_wakeup
|_ tty_wakeup
|_ ld->ops->write_wakeup
With the port->lock held, we call the @write_wakeup. Some implemetation of
the @write_wakeup does not notice that the port->lock is held, and it still
tries to send data with uart_write() which will try to grab the prot->lock.
A dead lock occurs, see the following log caught in the Bluetooth by uart:
--------------------------------------------------------------------
BUG: spinlock lockup suspected on CPU#0, swapper/0/0
lock: 0xdc3f4410, .magic: dead4ead, .owner: swapper/0/0, .owner_cpu: 0
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.10.17-16839-ge4a1bef #1320
[<80014cbc>] (unwind_backtrace+0x0/0x138) from [<8001251c>] (show_stack+0x10/0x14)
[<8001251c>] (show_stack+0x10/0x14) from [<802816ac>] (do_raw_spin_lock+0x108/0x184)
[<802816ac>] (do_raw_spin_lock+0x108/0x184) from [<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60)
[<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60) from [<802f5754>] (uart_write+0x38/0xe0)
[<802f5754>] (uart_write+0x38/0xe0) from [<80455270>] (hci_uart_tx_wakeup+0xa4/0x168)
[<80455270>] (hci_uart_tx_wakeup+0xa4/0x168) from [<802dab18>] (tty_wakeup+0x50/0x5c)
[<802dab18>] (tty_wakeup+0x50/0x5c) from [<802f81a4>] (imx_rtsint+0x50/0x80)
[<802f81a4>] (imx_rtsint+0x50/0x80) from [<802f88f4>] (imx_int+0x158/0x17c)
[<802f88f4>] (imx_int+0x158/0x17c) from [<8007abe0>] (handle_irq_event_percpu+0x50/0x194)
[<8007abe0>] (handle_irq_event_percpu+0x50/0x194) from [<8007ad60>] (handle_irq_event+0x3c/0x5c)
--------------------------------------------------------------------
This patch adds more limits to the @write_wakeup, the one who wants to
implemet the @write_wakeup should follow the limits which avoid the deadlock.
Signed-off-by: Huang Shijie <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
---
include/linux/tty_ldisc.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index b8347c2..8946e31 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -92,7 +92,10 @@
* This function is called by the low-level tty driver to signal
* that line discpline should try to send more characters to the
* low-level driver for transmission. If the line discpline does
- * not have any more data to send, it can just return.
+ * not have any more data to send, it can just return. If the line
+ * discipline does have some data to send, please arise a tasklet
+ * or workqueue to do the real data transfer. Do not send data in
+ * this hook, it may leads to a deadlock.
*
* int (*hangup)(struct tty_struct *)
*
--
1.9.1.286.g5172cb3
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 <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
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);
--
1.9.1.286.g5172cb3
LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().
->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.
Reviewed-by: Peter Hurley <[email protected]>
Reported-by: Huang Shijie <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 6e06f6f..77af52f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- struct tty_struct *tty = hu->tty;
- struct hci_dev *hdev = hu->hdev;
- struct sk_buff *skb;
-
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
BT_DBG("");
+ schedule_work(&hu->write_work);
+
+ return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+ struct tty_struct *tty = hu->tty;
+ struct hci_dev *hdev = hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT: should we cope with bad skbs or ->write() returning
+ * and error value ?
+ */
+
restart:
clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
@@ -153,7 +165,6 @@ restart:
goto restart;
clear_bit(HCI_UART_SENDING, &hu->tx_state);
- return 0;
}
static void hci_uart_init_work(struct work_struct *work)
@@ -281,6 +292,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
tty->receive_room = 65536;
INIT_WORK(&hu->init_ready, hci_uart_init_work);
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
spin_lock_init(&hu->rx_lock);
@@ -318,6 +330,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (hdev)
hci_uart_close(hdev);
+ cancel_work_sync(&hu->write_work);
+
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
if (hdev) {
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index fffa61f..12df101 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -68,6 +68,7 @@ struct hci_uart {
unsigned long hdev_flags;
struct work_struct init_ready;
+ struct work_struct write_work;
struct hci_uart_proto *proto;
void *priv;
--
1.9.1.286.g5172cb3
it wasn't used by anything, just remove it.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index c79d3e6..b092d3d 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -180,8 +180,6 @@ static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
/* Forward declaration of functions */
static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
-static struct workqueue_struct *serial_omap_uart_wq;
-
static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
{
offset <<= up->port.regshift;
@@ -1684,7 +1682,6 @@ static int serial_omap_probe(struct platform_device *pdev)
up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
pm_qos_add_request(&up->pm_qos_request,
PM_QOS_CPU_DMA_LATENCY, up->latency);
- serial_omap_uart_wq = create_singlethread_workqueue(up->name);
INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
platform_set_drvdata(pdev, up);
--
1.9.1.286.g5172cb3
UART IRQ Identification bitfield is 3
bits long (bits 3:1) but current mask only
masks 2 bits. Fix it.
Signed-off-by: Felipe Balbi <[email protected]>
---
include/uapi/linux/serial_reg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index e632260..99b4705 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -32,7 +32,7 @@
#define UART_IIR 2 /* In: Interrupt ID Register */
#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
-#define UART_IIR_ID 0x06 /* Mask for the interrupt ID */
+#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
#define UART_IIR_MSI 0x00 /* Modem status interrupt */
#define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
#define UART_IIR_RDI 0x04 /* Receiver data interrupt */
--
1.9.1.286.g5172cb3
On Tue, Apr 22, 2014 at 05:28:43PM -0700, Greg KH wrote:
> On Tue, Apr 22, 2014 at 09:22:56AM -0500, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Mar 20, 2014 at 02:29:57PM -0500, Felipe Balbi wrote:
> > > per CodingStyle we should have those braces, no
> > > functional changes.
> > >
> > > Signed-off-by: Felipe Balbi <[email protected]>
> >
> > Greg, do you want me to refresh and resend this series ?
>
> If there are conflicts with my current tree, sure, I'd love to have
> that. I should be trying to dig out from my patch backlog for 3.16-rc1
> by the end of this week...
then I'll resend rebased on -rc2, should make your life a little easier,
hopefully.
cheers
--
balbi
On Tue, Apr 22, 2014 at 09:22:56AM -0500, Felipe Balbi wrote:
> Hi,
>
> On Thu, Mar 20, 2014 at 02:29:57PM -0500, Felipe Balbi wrote:
> > per CodingStyle we should have those braces, no
> > functional changes.
> >
> > Signed-off-by: Felipe Balbi <[email protected]>
>
> Greg, do you want me to refresh and resend this series ?
If there are conflicts with my current tree, sure, I'd love to have
that. I should be trying to dig out from my patch backlog for 3.16-rc1
by the end of this week...
thanks,
greg k-h
Hi,
On Thu, Mar 20, 2014 at 02:29:57PM -0500, Felipe Balbi wrote:
> per CodingStyle we should have those braces, no
> functional changes.
>
> Signed-off-by: Felipe Balbi <[email protected]>
Greg, do you want me to refresh and resend this series ?
--
balbi
On 20/03/14 19:30, Felipe Balbi wrote:
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
>
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
>
I think the work queue should be placed into hci_uart_tty_wakeup() and
not hci_uart_tx_wakeup() as added by this patch.
The reason is that the kernel thread hci_uart_send_frame() calls
hci_uart_tx_wakeup() and this patch unnecessarily introduces a work
queue in the program flow of that kernel thread.
In other words, I think this patch has undesirable side-effects such as
adding latency and increased processor loading for hci_uart_send_frame().
Regards,
Dean
> Reviewed-by: Peter Hurley <[email protected]>
> Reported-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> drivers/bluetooth/hci_uart.h | 1 +
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 6e06f6f..77af52f 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>
> int hci_uart_tx_wakeup(struct hci_uart *hu)
> {
> - struct tty_struct *tty = hu->tty;
> - struct hci_dev *hdev = hu->hdev;
> - struct sk_buff *skb;
> -
> if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
> set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> return 0;
> @@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>
> BT_DBG("");
>
> + schedule_work(&hu->write_work);
> +
> + return 0;
> +}
> +
> +static void hci_uart_write_work(struct work_struct *work)
> +{
> + struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
> + struct tty_struct *tty = hu->tty;
> + struct hci_dev *hdev = hu->hdev;
> + struct sk_buff *skb;
> +
> + /* REVISIT: should we cope with bad skbs or ->write() returning
> + * and error value ?
> + */
> +
> restart:
> clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
>
> @@ -153,7 +165,6 @@ restart:
> goto restart;
>
> clear_bit(HCI_UART_SENDING, &hu->tx_state);
> - return 0;
> }
>
> static void hci_uart_init_work(struct work_struct *work)
> @@ -281,6 +292,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
> tty->receive_room = 65536;
>
> INIT_WORK(&hu->init_ready, hci_uart_init_work);
> + INIT_WORK(&hu->write_work, hci_uart_write_work);
>
> spin_lock_init(&hu->rx_lock);
>
> @@ -318,6 +330,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> if (hdev)
> hci_uart_close(hdev);
>
> + cancel_work_sync(&hu->write_work);
> +
> if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> if (hdev) {
> if (test_bit(HCI_UART_REGISTERED, &hu->flags))
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index fffa61f..12df101 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -68,6 +68,7 @@ struct hci_uart {
> unsigned long hdev_flags;
>
> struct work_struct init_ready;
> + struct work_struct write_work;
>
> struct hci_uart_proto *proto;
> void *priv;