[PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Anand Moon]

From: Anand Moon <linux.amoon@gmail.com>

drop the spin_unlock/lock around uart_write_wakeup to protect
write wakeup for uart port.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
changes logs: drop the previous approch of spin_unlock_irqrestore/save
---
 drivers/tty/serial/samsung.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index d72cd73..0cb70a9 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -758,11 +758,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		goto out;
 	}
 
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&port->lock);
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
-		spin_lock(&port->lock);
-	}
 
 	if (uart_circ_empty(xmit))
 		s3c24xx_serial_stop_tx(port);
-- 
1.9.1

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Peter Hurley]

[ +cc Krzysztof Kozlowski ]

On 02/18/2016 09:15 PM, Anand Moon wrote:
> From: Anand Moon <linux.amoon@gmail.com>
> 
> drop the spin_unlock/lock around uart_write_wakeup to protect
> write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the
changelog should provide as much information as possible to
the maintainer(s) and driver author(s), and that you should
test that outcome.

Here's what I would have written for a commit message:


  Remove deadlock workaround for line disciplines that invoke
  the tty driver's write() method directly from their write_wakeup()
  method. As documented for the write_wakeup() line discipline method
  in tty_ldisc.h, line disciplines must not attempt i/o directly
  from write_wakeup() as this will deadlock. Reviews of in-tree line
  disciplines confirm all defer i/o.

  NB: This workaround was added in commit c15c3747ee32
  ("serial: samsung: fix potential soft lockup during uart write")
  which notes both slip and bluetooth hci attempt i/o directly from
  write_wakeup(). These issues were fixed in commits 661f7fda21b1
  ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
  ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.


Regards,
Peter Hurley


PS - Minor procedural note: please cc those who have reviewed previous
     patch versions when you submit new versions, ok?  Not a big deal,
     lots of submitters don't, but it's still preferred.


> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> changes logs: drop the previous approch of spin_unlock_irqrestore/save
> ---
>  drivers/tty/serial/samsung.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index d72cd73..0cb70a9 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -758,11 +758,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  		goto out;
>  	}
>  
> -	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
> -		spin_unlock(&port->lock);
> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>  		uart_write_wakeup(port);
> -		spin_lock(&port->lock);
> -	}
>  
>  	if (uart_circ_empty(xmit))
>  		s3c24xx_serial_stop_tx(port);
> 

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Anand Moon]

Hi Peter,

On 20 February 2016 at 01:00, Peter Hurley <peter@hurleysoftware.com> wrote:
> [ +cc Krzysztof Kozlowski ]
>
> On 02/18/2016 09:15 PM, Anand Moon wrote:
>> From: Anand Moon <linux.amoon@gmail.com>
>>
>> drop the spin_unlock/lock around uart_write_wakeup to protect
>> write wakeup for uart port.
>
> What Krzysztof was saying wrt v1 of this patch is that the
> changelog should provide as much information as possible to
> the maintainer(s) and driver author(s), and that you should
> test that outcome.
>
> Here's what I would have written for a commit message:
>
>
>   Remove deadlock workaround for line disciplines that invoke
>   the tty driver's write() method directly from their write_wakeup()
>   method. As documented for the write_wakeup() line discipline method
>   in tty_ldisc.h, line disciplines must not attempt i/o directly
>   from write_wakeup() as this will deadlock. Reviews of in-tree line
>   disciplines confirm all defer i/o.
>
>   NB: This workaround was added in commit c15c3747ee32
>   ("serial: samsung: fix potential soft lockup during uart write")
>   which notes both slip and bluetooth hci attempt i/o directly from
>   write_wakeup(). These issues were fixed in commits 661f7fda21b1
>   ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
>   ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.
>
>
> Regards,
> Peter Hurley
>
>
> PS - Minor procedural note: please cc those who have reviewed previous
>      patch versions when you submit new versions, ok?  Not a big deal,
>      lots of submitters don't, but it's still preferred.
>
>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>> changes logs: drop the previous approch of spin_unlock_irqrestore/save
>> ---
>>  drivers/tty/serial/samsung.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index d72cd73..0cb70a9 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -758,11 +758,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>>               goto out;
>>       }
>>
>> -     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
>> -             spin_unlock(&port->lock);
>> +     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>>               uart_write_wakeup(port);
>> -             spin_lock(&port->lock);
>> -     }
>>
>>       if (uart_circ_empty(xmit))
>>               s3c24xx_serial_stop_tx(port);
>>
>

Thanks I will append your suggestions and resend this patch.
I will keep a note of your suggestions thanks you.

Best Regards.
-Anand Moon

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Krzysztof Kozlowski]

2016-02-20 4:30 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
> [ +cc Krzysztof Kozlowski ]
>
> On 02/18/2016 09:15 PM, Anand Moon wrote:
>> From: Anand Moon <linux.amoon@gmail.com>
>>
>> drop the spin_unlock/lock around uart_write_wakeup to protect
>> write wakeup for uart port.
>
> What Krzysztof was saying wrt v1 of this patch is that the
> changelog should provide as much information as possible to
> the maintainer(s) and driver author(s), and that you should
> test that outcome.
>
> Here's what I would have written for a commit message:
>
>
>   Remove deadlock workaround for line disciplines that invoke
>   the tty driver's write() method directly from their write_wakeup()
>   method. As documented for the write_wakeup() line discipline method
>   in tty_ldisc.h, line disciplines must not attempt i/o directly
>   from write_wakeup() as this will deadlock. Reviews of in-tree line
>   disciplines confirm all defer i/o.
>
>   NB: This workaround was added in commit c15c3747ee32
>   ("serial: samsung: fix potential soft lockup during uart write")
>   which notes both slip and bluetooth hci attempt i/o directly from
>   write_wakeup(). These issues were fixed in commits 661f7fda21b1
>   ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
>   ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

Thanks Peter for thorough analysis. It shouldn't be done by you but by
the patch submitter... and I have big worries that Anand did not
perform that analysis.

Anand, could you at least test that this lockup does not happen
anymore? You will need board with Bluetooth for that (and not USB
Bluetooth...). If you cannot test it, maybe guys from Polish R&D could
help you (Cc-ed), because they were working on DMA for serial used in
Bluetooth.

Best regards,
Krzysztof

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Anand Moon]

Hi Krzysztof,

On 21 February 2016 at 07:07, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2016-02-20 4:30 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>> [ +cc Krzysztof Kozlowski ]
>>
>> On 02/18/2016 09:15 PM, Anand Moon wrote:
>>> From: Anand Moon <linux.amoon@gmail.com>
>>>
>>> drop the spin_unlock/lock around uart_write_wakeup to protect
>>> write wakeup for uart port.
>>
>> What Krzysztof was saying wrt v1 of this patch is that the
>> changelog should provide as much information as possible to
>> the maintainer(s) and driver author(s), and that you should
>> test that outcome.
>>
>> Here's what I would have written for a commit message:
>>
>>
>>   Remove deadlock workaround for line disciplines that invoke
>>   the tty driver's write() method directly from their write_wakeup()
>>   method. As documented for the write_wakeup() line discipline method
>>   in tty_ldisc.h, line disciplines must not attempt i/o directly
>>   from write_wakeup() as this will deadlock. Reviews of in-tree line
>>   disciplines confirm all defer i/o.
>>
>>   NB: This workaround was added in commit c15c3747ee32
>>   ("serial: samsung: fix potential soft lockup during uart write")
>>   which notes both slip and bluetooth hci attempt i/o directly from
>>   write_wakeup(). These issues were fixed in commits 661f7fda21b1
>>   ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
>>   ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.
>
> Thanks Peter for thorough analysis. It shouldn't be done by you but by
> the patch submitter... and I have big worries that Anand did not
> perform that analysis.
>
> Anand, could you at least test that this lockup does not happen
> anymore? You will need board with Bluetooth for that (and not USB
> Bluetooth...). If you cannot test it, maybe guys from Polish R&D could
> help you (Cc-ed), because they were working on DMA for serial used in
> Bluetooth.
>
> Best regards,
> Krzysztof

I have looked into the history of the changes,
commit c15c3747ee32 (serial: samsung: fix potential soft lockup during
uart write)
was added long time ago, that's why have missed this.

I don't have on-board Bluetooth enable boards with me,
so if their is potential lockup you people observer
do not consider this patch.

My change's were on the perception of the locking/unlocking.

Thanks for your inputs.

Best regards,
-Anand Moon

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Krzysztof Kozlowski]

On 21.02.2016 11:59, Anand Moon wrote:
> Hi Krzysztof,
> 
> On 21 February 2016 at 07:07, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2016-02-20 4:30 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>>> [ +cc Krzysztof Kozlowski ]
>>>
>>> On 02/18/2016 09:15 PM, Anand Moon wrote:
>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>
>>>> drop the spin_unlock/lock around uart_write_wakeup to protect
>>>> write wakeup for uart port.
>>>
>>> What Krzysztof was saying wrt v1 of this patch is that the
>>> changelog should provide as much information as possible to
>>> the maintainer(s) and driver author(s), and that you should
>>> test that outcome.
>>>
>>> Here's what I would have written for a commit message:
>>>
>>>
>>>   Remove deadlock workaround for line disciplines that invoke
>>>   the tty driver's write() method directly from their write_wakeup()
>>>   method. As documented for the write_wakeup() line discipline method
>>>   in tty_ldisc.h, line disciplines must not attempt i/o directly
>>>   from write_wakeup() as this will deadlock. Reviews of in-tree line
>>>   disciplines confirm all defer i/o.
>>>
>>>   NB: This workaround was added in commit c15c3747ee32
>>>   ("serial: samsung: fix potential soft lockup during uart write")
>>>   which notes both slip and bluetooth hci attempt i/o directly from
>>>   write_wakeup(). These issues were fixed in commits 661f7fda21b1
>>>   ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
>>>   ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.
>>
>> Thanks Peter for thorough analysis. It shouldn't be done by you but by
>> the patch submitter... and I have big worries that Anand did not
>> perform that analysis.
>>
>> Anand, could you at least test that this lockup does not happen
>> anymore? You will need board with Bluetooth for that (and not USB
>> Bluetooth...). If you cannot test it, maybe guys from Polish R&D could
>> help you (Cc-ed), because they were working on DMA for serial used in
>> Bluetooth.
>>
>> Best regards,
>> Krzysztof
> 
> I have looked into the history of the changes,
> commit c15c3747ee32 (serial: samsung: fix potential soft lockup during
> uart write)
> was added long time ago, that's why have missed this.
> 
> I don't have on-board Bluetooth enable boards with me,
> so if their is potential lockup you people observer
> do not consider this patch.

Which means that you cannot test it... Find someone who can test it with
Bluetooth (or SLIP) and will provide you a Tested-by tag.

Best regards,
Krzysztof

Re: [PATCHv2] serial: samsung: drop the spinlock around uart_write_wakeup

[Anand Moon]

Hi Krzysztof,

On 22 February 2016 at 05:38, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 21.02.2016 11:59, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 21 February 2016 at 07:07, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2016-02-20 4:30 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> [ +cc Krzysztof Kozlowski ]
>>>>
>>>> On 02/18/2016 09:15 PM, Anand Moon wrote:
>>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>>
>>>>> drop the spin_unlock/lock around uart_write_wakeup to protect
>>>>> write wakeup for uart port.
>>>>
>>>> What Krzysztof was saying wrt v1 of this patch is that the
>>>> changelog should provide as much information as possible to
>>>> the maintainer(s) and driver author(s), and that you should
>>>> test that outcome.
>>>>
>>>> Here's what I would have written for a commit message:
>>>>
>>>>
>>>>   Remove deadlock workaround for line disciplines that invoke
>>>>   the tty driver's write() method directly from their write_wakeup()
>>>>   method. As documented for the write_wakeup() line discipline method
>>>>   in tty_ldisc.h, line disciplines must not attempt i/o directly
>>>>   from write_wakeup() as this will deadlock. Reviews of in-tree line
>>>>   disciplines confirm all defer i/o.
>>>>
>>>>   NB: This workaround was added in commit c15c3747ee32
>>>>   ("serial: samsung: fix potential soft lockup during uart write")
>>>>   which notes both slip and bluetooth hci attempt i/o directly from
>>>>   write_wakeup(). These issues were fixed in commits 661f7fda21b1
>>>>   ("slip: Fix deadlock in write_wakeup") and da64c27d3c93
>>>>   ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.
>>>
>>> Thanks Peter for thorough analysis. It shouldn't be done by you but by
>>> the patch submitter... and I have big worries that Anand did not
>>> perform that analysis.
>>>
>>> Anand, could you at least test that this lockup does not happen
>>> anymore? You will need board with Bluetooth for that (and not USB
>>> Bluetooth...). If you cannot test it, maybe guys from Polish R&D could
>>> help you (Cc-ed), because they were working on DMA for serial used in
>>> Bluetooth.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> I have looked into the history of the changes,
>> commit c15c3747ee32 (serial: samsung: fix potential soft lockup during
>> uart write)
>> was added long time ago, that's why have missed this.
>>
>> I don't have on-board Bluetooth enable boards with me,
>> so if their is potential lockup you people observer
>> do not consider this patch.
>
> Which means that you cannot test it... Find someone who can test it with
> Bluetooth (or SLIP) and will provide you a Tested-by tag.
>
> Best regards,
> Krzysztof
>

It will be better if some body can verify the change.

Best Regards.
-Anand Moon