[PATCH 0/4] serial: doc: Low Level Serial API Documentation Improvements (take two)

[PATCH 0/4] serial: doc: Low Level Serial API Documentation Improvements (take two)

[Geert Uytterhoeven]

	Hi Greg, Jiri, Jon, Peter, Russell,

This patch series contains improvements to the low level serial driver
API documentation.

It is an incremental update (sort of v2) of "[PATCH 0/9] serial: doc:
Low Level Serial API Documentation Improvements", which was already
applied to the docs tree.

Thanks for your comments!

Geert Uytterhoeven (4):
  serial: doc: Re-add paragraph documenting uart_console_write()
  serial: doc: .(un)throttle() depends on hardware assisted flow control
  serial: doc: .(un)throttle() are serialized by the tty layer
  serial: doc: .break_ctl() may sleep

 Documentation/serial/driver | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/4] serial: doc: Re-add paragraph documenting uart_console_write()

[Geert Uytterhoeven]

Commit 834392a7d92677ff ("serial: doc: Un-document non-existing
uart_write_console()") removed a paragraph about a helper function that
seemed to never exist.

Peter Hurley pointed out that the function does exist, but is called
differently. Re-add the paragraph, with the function name corrected.

Fixes: 834392a7d92677ff ("serial: doc: Un-document non-existing uart_write_console()")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/serial/driver | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index 65de49a4b39e5baf..03369d076ca20a98 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -28,6 +28,11 @@ The serial core provides a few helper functions.  This includes identifing
 the correct port structure (via uart_get_console) and decoding command line
 arguments (uart_parse_options).
 
+There is also a helper function (uart_console_write) which performs a
+character by character write, translating newlines to CRLF sequences.
+Driver writers are recommended to use this function rather than implementing
+their own version.
+
 
 Locking
 -------
-- 
1.9.1

[PATCH 2/4] serial: doc: .(un)throttle() depends on hardware assisted flow control

[Geert Uytterhoeven]

Document that .throttle() and .unthrottle() are relevant only if
hardware assisted flow control is enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/serial/driver | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index 03369d076ca20a98..03b703cf93182216 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -135,6 +135,7 @@ hardware.
 	Notify the serial driver that input buffers for the line discipline are
 	close to full, and it should somehow signal that no more characters
 	should be sent to the serial port.
+	This will be called only if hardware assisted flow control is enabled.
 
 	Locking: none.
 
@@ -142,6 +143,7 @@ hardware.
 	Notify the serial driver that characters can now be sent to the serial
 	port without fear of overrunning the input buffers of the line
 	disciplines.
+	This will be called only if hardware assisted flow control is enabled.
 
 	Locking: none.
 
-- 
1.9.1

[PATCH 3/4] serial: doc: .(un)throttle() are serialized by the tty layer

[Geert Uytterhoeven]

Document that .(un)throttle() are serialized with each other, and with
termios modification by the tty layer.

Reported-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/serial/driver | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index 03b703cf93182216..7fb80682e394eb76 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -137,7 +137,8 @@ hardware.
 	should be sent to the serial port.
 	This will be called only if hardware assisted flow control is enabled.
 
-	Locking: none.
+	Locking: serialized with .unthrottle() and termios modification by the
+		 tty layer.
 
   unthrottle(port)
 	Notify the serial driver that characters can now be sent to the serial
@@ -145,7 +146,8 @@ hardware.
 	disciplines.
 	This will be called only if hardware assisted flow control is enabled.
 
-	Locking: none.
+	Locking: serialized with .throttle() and termios modification by the
+		 tty layer.
 
   send_xchar(port,ch)
 	Transmit a high priority character, even if the port is stopped.
-- 
1.9.1

[PATCH 4/4] serial: doc: .break_ctl() may sleep

[Geert Uytterhoeven]

As mutex_lock() must not be called with interrupts disabled,
.break_ctl() may sleep.

Reported-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/serial/driver | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index 7fb80682e394eb76..39701515832b70b0 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -187,7 +187,6 @@ hardware.
 	ctl.
 
 	Locking: caller holds port->mutex
-	This call must not sleep
 
   startup(port)
 	Grab any interrupt resources and initialise any low level driver
-- 
1.9.1

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Jonathan Corbet]

On Thu, 14 Apr 2016 11:08:11 +0200
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> As mutex_lock() must not be called with interrupts disabled,
> .break_ctl() may sleep.

So I've applied the first three to the docs tree, but this one stopped
me.  The changelog doesn't really say why the patch is correct; what we
really need to know is that break_ctl() won't be called in atomic
context.  I'm assuming that's the case, but if it truly is, can I get a
version with a changelog saying that?

Thanks,

jon

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Peter Hurley]

On 04/15/2016 03:01 PM, Jonathan Corbet wrote:
> On Thu, 14 Apr 2016 11:08:11 +0200
> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
>> As mutex_lock() must not be called with interrupts disabled,
>> .break_ctl() may sleep.
> 
> So I've applied the first three to the docs tree, but this one stopped
> me.  The changelog doesn't really say why the patch is correct; what we
> really need to know is that break_ctl() won't be called in atomic
> context.

The only caller of the uart driver's break_ctl() method is
uart_break_ctl(), which is serial core's proxy tty driver break_ctl()
method. uart_break_ctl() claims the struct tty_port::mutex to prevent
concurrent tiocmset().

Thus, the uart driver's break_ctl() method won't be called in atomic
context.

Regards,
Peter Hurley

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Jonathan Corbet]

On Fri, 15 Apr 2016 15:31:30 -0700
Peter Hurley <peter@hurleysoftware.com> wrote:

> The only caller of the uart driver's break_ctl() method is
> uart_break_ctl(), which is serial core's proxy tty driver break_ctl()
> method. uart_break_ctl() claims the struct tty_port::mutex to prevent
> concurrent tiocmset().
> 
> Thus, the uart driver's break_ctl() method won't be called in atomic
> context.

I'm missing something here.  I can fully believe that uart_break_ctl()
won't call break_ctl() in atomic context, but the fact that it holds a
mutex in no way guarantees that.  If uart_break_ctl() makes that promise
we should just say so.

Sorry to be obnoxious, but I'd rather not put confusing stuff into the
changelog if possible.

jon

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Peter Hurley]

On 04/15/2016 03:41 PM, Jonathan Corbet wrote:
> On Fri, 15 Apr 2016 15:31:30 -0700
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
>> The only caller of the uart driver's break_ctl() method is
>> uart_break_ctl(), which is serial core's proxy tty driver break_ctl()
>> method. uart_break_ctl() claims the struct tty_port::mutex to prevent
>> concurrent tiocmset().
>>
>> Thus, the uart driver's break_ctl() method won't be called in atomic
>> context.
> 
> I'm missing something here.

Yes.

The analysis above is required to show that the API contract asserted by
the proposed change to the documentation is currently true in the code,
which is what I care about.

I don't mind if that's not in the changelog, though.

> I can fully believe that uart_break_ctl()
> won't call break_ctl() in atomic context, but the fact that it holds a
> mutex in no way guarantees that.  If uart_break_ctl() makes that promise
> we should just say so.
> 
> Sorry to be obnoxious, but I'd rather not put confusing stuff into the
> changelog if possible.

Sure, that's fine.

Regards,
Peter Hurley

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Jonathan Corbet]

On Fri, 15 Apr 2016 16:07:39 -0700
Peter Hurley <peter@hurleysoftware.com> wrote:

> > I'm missing something here.  
> 
> Yes.
> 
> The analysis above is required to show that the API contract asserted by
> the proposed change to the documentation is currently true in the code,
> which is what I care about.

Yes, but the analysis says nothing about what uart_break_ctl() itself
might do, so by itself, it provides no guarantee for break_ctl().  That
was my sticking point since somebody clearly put that line in there for a
reason.

Looking at the code, it's pretty obvious that uart_break_ctl() isn't
acquiring any spinlocks.  The documentation line in question has been
there, unchanged, since the beginning of the Git era.  The patch is
obviously fine, and I've applied it, but I did tweak the changelog some.

Thanks,

jon

Re: [PATCH 4/4] serial: doc: .break_ctl() may sleep

[Geert Uytterhoeven]

Hi Jon,

On Sat, Apr 16, 2016 at 6:47 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Fri, 15 Apr 2016 16:07:39 -0700
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> > I'm missing something here.
>>
>> Yes.
>>
>> The analysis above is required to show that the API contract asserted by
>> the proposed change to the documentation is currently true in the code,
>> which is what I care about.
>
> Yes, but the analysis says nothing about what uart_break_ctl() itself
> might do, so by itself, it provides no guarantee for break_ctl().  That
> was my sticking point since somebody clearly put that line in there for a
> reason.
>
> Looking at the code, it's pretty obvious that uart_break_ctl() isn't
> acquiring any spinlocks.  The documentation line in question has been
> there, unchanged, since the beginning of the Git era.  The patch is
> obviously fine, and I've applied it, but I did tweak the changelog some.

Sorry, this indeed needed more clarification.
Thanks for fixing it up!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds