uart: fix uart_opal_flush to take console lock over uart_con_flush

Message ID 20180409034954.31110-1-npiggin@gmail.com
State Accepted
Headers show
Series
  • uart: fix uart_opal_flush to take console lock over uart_con_flush
Related show

Commit Message

Nicholas Piggin April 9, 2018, 3:49 a.m.
Cc: Russell Currey <ruscur@russell.cc>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/lpc-uart.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Russell Currey April 9, 2018, 3:53 a.m. | #1
On Mon, 2018-04-09 at 13:49 +1000, Nicholas Piggin wrote:
> Cc: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Russell Currey <ruscur@russell.cc>
Stewart Smith April 30, 2018, 7:42 a.m. | #2
Nicholas Piggin <npiggin@gmail.com> writes:
> Cc: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/lpc-uart.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Whoops! Merged to master as of 23dc884f8a0f0b52d3e9312b64b9b02fcd8453a1.

I'm kinda going umm-ahh on if should pick back to any stable
branch. Maybe? Have we seen an actual problem because of it? Will we?
Nicholas Piggin April 30, 2018, 7:53 a.m. | #3
On Mon, 30 Apr 2018 17:42:20 +1000
Stewart Smith <stewart@linux.ibm.com> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> > Cc: Russell Currey <ruscur@russell.cc>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/lpc-uart.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)  
> 
> Whoops! Merged to master as of 23dc884f8a0f0b52d3e9312b64b9b02fcd8453a1.
> 
> I'm kinda going umm-ahh on if should pick back to any stable
> branch. Maybe? Have we seen an actual problem because of it? Will we?
> 

Yeah it would be good if we can. We haven't seen a problem but Linux
rarely uses the call at the moment. I'd like to start using it more,
but I don't know how advisable that would be for the foreseeable
future :(

Thanks,
Nick
Stewart Smith April 30, 2018, 10:01 p.m. | #4
Nicholas Piggin <npiggin@gmail.com> writes:
> On Mon, 30 Apr 2018 17:42:20 +1000
> Stewart Smith <stewart@linux.ibm.com> wrote:
>
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> > Cc: Russell Currey <ruscur@russell.cc>
>> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> > ---
>> >  hw/lpc-uart.c | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)  
>> 
>> Whoops! Merged to master as of 23dc884f8a0f0b52d3e9312b64b9b02fcd8453a1.
>> 
>> I'm kinda going umm-ahh on if should pick back to any stable
>> branch. Maybe? Have we seen an actual problem because of it? Will we?
>> 
>
> Yeah it would be good if we can. We haven't seen a problem but Linux
> rarely uses the call at the moment. I'd like to start using it more,
> but I don't know how advisable that would be for the foreseeable
> future :(

We could always create a FLUSH2 call which is "flush, but with the
locking bug fixed" and that way Linux could know that it's not going to
be buggy and miss the lock.

Patch

diff --git a/hw/lpc-uart.c b/hw/lpc-uart.c
index 3224de9f..74a86b13 100644
--- a/hw/lpc-uart.c
+++ b/hw/lpc-uart.c
@@ -218,7 +218,7 @@  static uint8_t *out_buf;
 static uint32_t out_buf_prod;
 static uint32_t out_buf_cons;
 
-/* Asynchronous flush */
+/* Asynchronous flush, uart_lock must be held */
 static int64_t uart_con_flush(void)
 {
 	bool tx_was_full = tx_full;
@@ -388,10 +388,16 @@  static int64_t uart_opal_read(int64_t term_number, int64_t *length,
 
 static int64_t uart_opal_flush(int64_t term_number)
 {
+	int64_t rc;
+
 	if (term_number != 0)
 		return OPAL_PARAMETER;
 
-	return uart_con_flush();
+	lock(&uart_lock);
+	rc = uart_con_flush();
+	unlock(&uart_lock);
+
+	return rc;
 }
 
 static void __uart_do_poll(u8 trace_ctx)