serial: samsung: fix the inconsistency in spinlock

PATCH In review Anand Moon 3 participants 11 messages Feb 18, 2016 – Feb 21, 2016

Subsystems (2)

Arm Samsung Exynos TTY layer and serial drivers

lore

Share & tools

lore

Anand Moon

author

2016-02-18 17:40 UTC #0 lore raw

PATCH 1/1 serial: samsung: fix the inconsistency in spinlock

From: Anand Moon <[email protected]>

changes fix the correct order of the spin_lock_irqrestore/save.

Signed-off-by Anand Moon <[email protected]>

---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/serial/samsung.c +2 -2

index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&port->lock);
+		spin_unlock_irqrestore(&port->lock, flags);
 		uart_write_wakeup(port);
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->lock, flags);
 	}
 
 	if (uart_circ_empty(xmit))

Signature · 1 line

1.9.1

Peter Hurley

2016-02-18 17:48 UTC #1 lore raw

Hi Anand,

Quoted 14 lines · Anand Moon

From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644

drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764

Quoted 8 lines

drivers/tty/serial/samsung.c +2 -2

@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&port->lock);
+		spin_unlock_irqrestore(&port->lock, flags);
 		uart_write_wakeup(port);
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->lock, flags);

This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.

Regards,
Peter Hurley

Quoted 4 lines

drivers/tty/serial/samsung.c +0 -0
 	}
 
 	if (uart_circ_empty(xmit))

Anand Moon

author

2016-02-18 19:14 UTC #2 lore raw

Hi Peter,

Quoted 19 lines · Peter Hurley

Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 

Quoted 11 lines

drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.

Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.

Quoted 9 lines

Regards,
Peter Hurley
Quoted 4 lines
drivers/tty/serial/samsung.c +0 -0
      }
 
      if (uart_circ_empty(xmit))

Best Regards
-Anand Moon

Peter Hurley

2016-02-18 20:03 UTC #3 lore raw

Quoted 5 lines · Anand Moon

Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.

Yes.

Krzysztof Kozlowski

2016-02-19 06:09 UTC #4 lore raw

In reply to Anand Moon

2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:

Quoted 34 lines

Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.

Quoted 5 lines

Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.

Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.

Best regards,
Krzysztof

Anand Moon

author

2016-02-19 06:51 UTC #5 lore raw

Hi Krzysztof,

Quoted 41 lines · Krzysztof Kozlowski

2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:
Quoted 34 lines
Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.
Quoted 5 lines
Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.

Quoted 8 lines

Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.
Best regards,
Krzysztof

Yes I understood the meaning of the sentence. Already the s3c24xx_serial_tx_chars function.
holds the lock port->lock for safe IRQ execution.

http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L701

Best Regards
-Anand Moon

Krzysztof Kozlowski

2016-02-19 07:44 UTC #6 lore raw

Quoted 51 lines · Anand Moon

Hi Krzysztof,
Quoted 47 lines · Krzysztof Kozlowski
2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:
Quoted 34 lines
Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.
Quoted 5 lines
Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.
Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.

Quoted 6 lines

Quoted 2 lines
Best regards,
Krzysztof
Yes I understood the meaning of the sentence. Already the s3c24xx_serial_tx_chars function.
holds the lock port->lock for safe IRQ execution.

I am sorry but I don't get your explanation. I mentioned Peter's thoughts about lockups after adding locking over uart_write(). However you are referring to s3c24xx_serial_tx_chars() holding the spin lock... I am missing the point...

Best regards,
Krzysztof

Anand Moon

author

2016-02-19 08:23 UTC #7 lore raw

Hi Krzysztof,

Quoted 59 lines · Krzysztof Kozlowski

Quoted 51 lines · Anand Moon
Hi Krzysztof,
Quoted 47 lines · Krzysztof Kozlowski
2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:
Quoted 34 lines
Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.
Quoted 5 lines
Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.
Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.
Quoted 6 lines
Quoted 2 lines
Best regards,
Krzysztof
Yes I understood the meaning of the sentence. Already the s3c24xx_serial_tx_chars function.
holds the lock port->lock for safe IRQ execution.

Quoted 8 lines

I am sorry but I don't get your explanation. I mentioned Peter's thoughts about lockups after adding locking over uart_write(). However you are referring to s3c24xx_serial_tx_chars() holding the spin lock... I am missing the point...
Best regards,
Krzysztof

I should be sorry I could not explain you in technical terms. Interrupt routine already hold the port->lock

s3c24xx_serial_tx_chars

     \
     spin_lock_irqsave(&port->lock, flags);
     \...
    spin_unlock(&port->lock);
     uart_write_wakeup(port);
     spin_lock(&port->lock);
     \
     spin_unlock_irqrestore(&port->lock, flags);

In my next patch I have tried to remove the spin_unlock/spin_lock over uart_write_wakeup(port);

Best Regards.
-Anand Moon

Krzysztof Kozlowski

2016-02-19 08:27 UTC #8 lore raw

Quoted 76 lines · Anand Moon

Hi Krzysztof,
Quoted 59 lines · Krzysztof Kozlowski
Quoted 51 lines · Anand Moon
Hi Krzysztof,
Quoted 47 lines · Krzysztof Kozlowski
2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:
Quoted 34 lines
Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.
Quoted 5 lines
Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.
Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.
Quoted 6 lines
Quoted 2 lines
Best regards,
Krzysztof
Yes I understood the meaning of the sentence. Already the s3c24xx_serial_tx_chars function.
holds the lock port->lock for safe IRQ execution.
Quoted 8 lines
I am sorry but I don't get your explanation. I mentioned Peter's thoughts about lockups after adding locking over uart_write(). However you are referring to s3c24xx_serial_tx_chars() holding the spin lock... I am missing the point...
Best regards,
Krzysztof
I should be sorry I could not explain you in technical terms. Interrupt routine already hold the port->lock
s3c24xx_serial_tx_chars

Quoted 9 lines

     \
     spin_lock_irqsave(&port->lock, flags);
     \...
    spin_unlock(&port->lock);
     uart_write_wakeup(port);
     spin_lock(&port->lock);
     \
     spin_unlock_irqrestore(&port->lock, flags);

This is obvious.

Quoted 2 lines

In my next patch I have tried to remove the spin_unlock/spin_lock over uart_write_wakeup(port);

Which may create lockups. Previously there was no port locking around uart_write_wakeup. Now there will be. You are effectively adding locking over uart_write_wakeup().
Again, we are back at the Peter's message - just check the damned lockups...

BR,
Krzysztof

Anand Moon

author

2016-02-19 08:34 UTC #9 lore raw

Hi Krzysztof,

Quoted 97 lines · Krzysztof Kozlowski

Quoted 76 lines · Anand Moon
Hi Krzysztof,
Quoted 59 lines · Krzysztof Kozlowski
Quoted 51 lines · Anand Moon
Hi Krzysztof,
Quoted 47 lines · Krzysztof Kozlowski
2016-02-19 4:14 GMT+09:00 Anand Moon <[email protected]>:
Quoted 34 lines
Hi Peter,
Quoted 19 lines · Peter Hurley
Hi Anand,
Quoted 16 lines · Anand Moon
From: Anand Moon <[email protected]>
changes fix the correct order of the spin_lock_irqrestore/save.
Signed-off-by Anand Moon <[email protected]>
---
 drivers/tty/serial/samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/samsung.c +0 -0
index d72cd73..96fe14d 100644
@@ -759,9 +759,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
      }
 
Quoted 11 lines
drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 764
Quoted 6 lines
drivers/tty/serial/samsung.c +2 -2
      if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+             spin_unlock_irqrestore(&port->lock, flags);
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
+             spin_lock_irqsave(&port->lock, flags);
This driver shouldn't be dropping the spin lock at for write wakeup. If this is causing lock-ups in a line discipline, the line discipline needs fixed.
Quoted 5 lines
Thanks for pointing out.
Their is no lock up, just the inconstancy of the spin_lock. Then I will resend this patch dropping the spin_unlock/spin_lock around uart_write_wakeup.
Is that ok with you.
Anand, before doing that, can you check Peter's second sentence? I mean the "If this is causing lock-ups in a line discipline, the line discipline needs fixed.".
Don't drop the spin-locks "just because". I would be happy to see more detailed explanation in changelog.
Quoted 6 lines
Quoted 2 lines
Best regards,
Krzysztof
Yes I understood the meaning of the sentence. Already the s3c24xx_serial_tx_chars function.
holds the lock port->lock for safe IRQ execution.
Quoted 8 lines
I am sorry but I don't get your explanation. I mentioned Peter's thoughts about lockups after adding locking over uart_write(). However you are referring to s3c24xx_serial_tx_chars() holding the spin lock... I am missing the point...
Best regards,
Krzysztof
I should be sorry I could not explain you in technical terms. Interrupt routine already hold the port->lock
s3c24xx_serial_tx_chars
Quoted 9 lines
     \
     spin_lock_irqsave(&port->lock, flags);
     \...
    spin_unlock(&port->lock);
     uart_write_wakeup(port);
     spin_lock(&port->lock);
     \
     spin_unlock_irqrestore(&port->lock, flags);
This is obvious.
Quoted 2 lines
In my next patch I have tried to remove the spin_unlock/spin_lock over uart_write_wakeup(port);
Which may create lockups. Previously there was no port locking around uart_write_wakeup. Now there will be. You are effectively adding locking over uart_write_wakeup().
Again, we are back at the Peter's message - just check the damned lockups...

Quoted 5 lines

BR,
Krzysztof
BR

Lets drop this patch. I have send new one earlier.
https://lkml.org/lkml/2016/2/19/2

If you have any comment on that.
Sorry for the confusion.

Best Regards.
-Anand Moon

Krzysztof Kozlowski

2016-02-21 01:30 UTC #10 lore raw

Quoted 11 lines

Quoted 11 lines
Quoted 2 lines
In my next patch I have tried to remove the spin_unlock/spin_lock over uart_write_wakeup(port);
Which may create lockups. Previously there was no port locking around uart_write_wakeup. Now there will be. You are effectively adding locking over uart_write_wakeup().
Again, we are back at the Peter's message - just check the damned lockups...
BR,
Krzysztof

Quoted 8 lines

Quoted 2 lines
BR
Lets drop this patch. I have send new one earlier.
https://lkml.org/lkml/2016/2/19/2
If you have any comment on that.
Sorry for the confusion.

I was commenting your future patch about dropping spinlocks. Apparently there is huge misunderstanding here...

Best regards,
Krzysztof