[PATCHv3,02/10] serdev: add serdev_device_wait_until_sent
diff mbox series

Message ID 20170328155939.31566-3-sre@kernel.org
State New, archived
Headers show
Series
  • Nokia H4+ support
Related show

Commit Message

Sebastian Reichel March 28, 2017, 3:59 p.m. UTC
Add method, which waits until the transmission buffer has been sent.
Note, that the change in ttyport_write_wakeup is related, since
tty_wait_until_sent will hang without that change.

Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Changes since PATCHv2:
 * Avoid goto in ttyport_write_wakeup
---
 drivers/tty/serdev/core.c           | 11 +++++++++++
 drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
 include/linux/serdev.h              |  3 +++
 3 files changed, 28 insertions(+), 4 deletions(-)

Comments

Greg Kroah-Hartman April 8, 2017, 4:57 p.m. UTC | #1
On Tue, Mar 28, 2017 at 05:59:31PM +0200, Sebastian Reichel wrote:
> Add method, which waits until the transmission buffer has been sent.
> Note, that the change in ttyport_write_wakeup is related, since
> tty_wait_until_sent will hang without that change.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
> Changes since PATCHv2:
>  * Avoid goto in ttyport_write_wakeup
> ---
>  drivers/tty/serdev/core.c           | 11 +++++++++++
>  drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
>  include/linux/serdev.h              |  3 +++
>  3 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index f4c6c90add78..a63b74031e22 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -173,6 +173,17 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
>  
> +void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->wait_until_sent)
> +		return;
> +
> +	ctrl->ops->wait_until_sent(ctrl, timeout);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);

Is this still needed now that we have serdev_device_write() with an
unlimited timeout available?

thanks,

greg k-h
Rob Herring April 10, 2017, 1:46 p.m. UTC | #2
On Sat, Apr 8, 2017 at 11:57 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Mar 28, 2017 at 05:59:31PM +0200, Sebastian Reichel wrote:
>> Add method, which waits until the transmission buffer has been sent.
>> Note, that the change in ttyport_write_wakeup is related, since
>> tty_wait_until_sent will hang without that change.
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Sebastian Reichel <sre@kernel.org>
>> ---
>> Changes since PATCHv2:
>>  * Avoid goto in ttyport_write_wakeup
>> ---
>>  drivers/tty/serdev/core.c           | 11 +++++++++++
>>  drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
>>  include/linux/serdev.h              |  3 +++
>>  3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index f4c6c90add78..a63b74031e22 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>> @@ -173,6 +173,17 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
>>  }
>>  EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
>>
>> +void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
>> +{
>> +     struct serdev_controller *ctrl = serdev->ctrl;
>> +
>> +     if (!ctrl || !ctrl->ops->wait_until_sent)
>> +             return;
>> +
>> +     ctrl->ops->wait_until_sent(ctrl, timeout);
>> +}
>> +EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);
>
> Is this still needed now that we have serdev_device_write() with an
> unlimited timeout available?

Yes, because only this waits until the data is on the wire.

Rob
Greg Kroah-Hartman April 10, 2017, 2:03 p.m. UTC | #3
On Mon, Apr 10, 2017 at 08:46:57AM -0500, Rob Herring wrote:
> On Sat, Apr 8, 2017 at 11:57 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Mar 28, 2017 at 05:59:31PM +0200, Sebastian Reichel wrote:
> >> Add method, which waits until the transmission buffer has been sent.
> >> Note, that the change in ttyport_write_wakeup is related, since
> >> tty_wait_until_sent will hang without that change.
> >>
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Acked-by: Pavel Machek <pavel@ucw.cz>
> >> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> >> ---
> >> Changes since PATCHv2:
> >>  * Avoid goto in ttyport_write_wakeup
> >> ---
> >>  drivers/tty/serdev/core.c           | 11 +++++++++++
> >>  drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
> >>  include/linux/serdev.h              |  3 +++
> >>  3 files changed, 28 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> index f4c6c90add78..a63b74031e22 100644
> >> --- a/drivers/tty/serdev/core.c
> >> +++ b/drivers/tty/serdev/core.c
> >> @@ -173,6 +173,17 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
> >>  }
> >>  EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
> >>
> >> +void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
> >> +{
> >> +     struct serdev_controller *ctrl = serdev->ctrl;
> >> +
> >> +     if (!ctrl || !ctrl->ops->wait_until_sent)
> >> +             return;
> >> +
> >> +     ctrl->ops->wait_until_sent(ctrl, timeout);
> >> +}
> >> +EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);
> >
> > Is this still needed now that we have serdev_device_write() with an
> > unlimited timeout available?
> 
> Yes, because only this waits until the data is on the wire.

What "wire" is that?  The serial wire?  How do you know this?  Many usb
to serial devices have no way to determine this, given that there is
another uart hanging off of the end of a USB connection.

Doesn't serdev_device_write() return when the write is finished?  I
think we need some good documentation here for all of the different
variants of how to send data, as I'm sure confused...

thanks,

greg k-h
Rob Herring April 10, 2017, 4:12 p.m. UTC | #4
+ Andrey

On Mon, Apr 10, 2017 at 9:03 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Apr 10, 2017 at 08:46:57AM -0500, Rob Herring wrote:
>> On Sat, Apr 8, 2017 at 11:57 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Tue, Mar 28, 2017 at 05:59:31PM +0200, Sebastian Reichel wrote:
>> >> Add method, which waits until the transmission buffer has been sent.
>> >> Note, that the change in ttyport_write_wakeup is related, since
>> >> tty_wait_until_sent will hang without that change.
>> >>
>> >> Acked-by: Rob Herring <robh@kernel.org>
>> >> Acked-by: Pavel Machek <pavel@ucw.cz>
>> >> Signed-off-by: Sebastian Reichel <sre@kernel.org>
>> >> ---
>> >> Changes since PATCHv2:
>> >>  * Avoid goto in ttyport_write_wakeup
>> >> ---
>> >>  drivers/tty/serdev/core.c           | 11 +++++++++++
>> >>  drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
>> >>  include/linux/serdev.h              |  3 +++
>> >>  3 files changed, 28 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> >> index f4c6c90add78..a63b74031e22 100644
>> >> --- a/drivers/tty/serdev/core.c
>> >> +++ b/drivers/tty/serdev/core.c
>> >> @@ -173,6 +173,17 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
>> >>
>> >> +void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
>> >> +{
>> >> +     struct serdev_controller *ctrl = serdev->ctrl;
>> >> +
>> >> +     if (!ctrl || !ctrl->ops->wait_until_sent)
>> >> +             return;
>> >> +
>> >> +     ctrl->ops->wait_until_sent(ctrl, timeout);
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);
>> >
>> > Is this still needed now that we have serdev_device_write() with an
>> > unlimited timeout available?
>>
>> Yes, because only this waits until the data is on the wire.
>
> What "wire" is that?  The serial wire?  How do you know this?  Many usb
> to serial devices have no way to determine this, given that there is
> another uart hanging off of the end of a USB connection.

Okay, maybe it's just out of linux s/w buffers for h/w which you don't
know. It is the same semantics as tty_wait_until_sent which is
documented as: "Wait for characters pending in a tty driver to hit the
wire, or for a timeout to occur (eg due to flow control)"

> Doesn't serdev_device_write() return when the write is finished?

No, it returns when data is accepted by the tty layer (as serdev has
no buffering of its own).

>  I
> think we need some good documentation here for all of the different
> variants of how to send data, as I'm sure confused...

Fair enough. However, I think that's somewhat orthogonal to this function.

There's 2 modes of sending data which are basically sync and async operation.

- serdev_device_write_buf - Only sends what can be immediately
accepted by lower layers. Caller should provide write_wakeup callback
to be notified.

- serdev_device_write - Sleeps until all data is accepted by lower
layers or a timeout occurs.

It's valid to call serdev_device_wait_until_sent in either case.


Yes, the naming is not the best, but we kept serdev_device_write_buf
to avoid any cross tree merges. We can remove or rename later as it is
just a wrapper now for serdev_device_write.

Rob
Sebastian Reichel April 10, 2017, 5:10 p.m. UTC | #5
Hi,

On Mon, Apr 10, 2017 at 11:12:39AM -0500, Rob Herring wrote:
> On Mon, Apr 10, 2017 at 9:03 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Apr 10, 2017 at 08:46:57AM -0500, Rob Herring wrote:
> >> On Sat, Apr 8, 2017 at 11:57 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Tue, Mar 28, 2017 at 05:59:31PM +0200, Sebastian Reichel wrote:
> >> >> Add method, which waits until the transmission buffer has been sent.
> >> >> Note, that the change in ttyport_write_wakeup is related, since
> >> >> tty_wait_until_sent will hang without that change.
> >> >>
> >> >> Acked-by: Rob Herring <robh@kernel.org>
> >> >> Acked-by: Pavel Machek <pavel@ucw.cz>
> >> >> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> >> >> ---
> >> >> Changes since PATCHv2:
> >> >>  * Avoid goto in ttyport_write_wakeup
> >> >> ---
> >> >>  drivers/tty/serdev/core.c           | 11 +++++++++++
> >> >>  drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++----
> >> >>  include/linux/serdev.h              |  3 +++
> >> >>  3 files changed, 28 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> >> index f4c6c90add78..a63b74031e22 100644
> >> >> --- a/drivers/tty/serdev/core.c
> >> >> +++ b/drivers/tty/serdev/core.c
> >> >> @@ -173,6 +173,17 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
> >> >>
> >> >> +void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
> >> >> +{
> >> >> +     struct serdev_controller *ctrl = serdev->ctrl;
> >> >> +
> >> >> +     if (!ctrl || !ctrl->ops->wait_until_sent)
> >> >> +             return;
> >> >> +
> >> >> +     ctrl->ops->wait_until_sent(ctrl, timeout);
> >> >> +}
> >> >> +EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);
> >> >
> >> > Is this still needed now that we have serdev_device_write() with an
> >> > unlimited timeout available?
> >>
> >> Yes, because only this waits until the data is on the wire.
> >
> > What "wire" is that?  The serial wire?  How do you know this?  Many usb
> > to serial devices have no way to determine this, given that there is
> > another uart hanging off of the end of a USB connection.
> 
> Okay, maybe it's just out of linux s/w buffers for h/w which you don't
> know. It is the same semantics as tty_wait_until_sent which is
> documented as: "Wait for characters pending in a tty driver to hit the
> wire, or for a timeout to occur (eg due to flow control)"

For embedded h/w it usually means the serial wire. tty_wait_until_sent()
first waits for the tty buffer to be empty and then calls wait_until_sent()
in the driver providing the tty. In case of serial-core that is
implemented by uart_wait_until_sent(), which waits for the serial
driver's tx_empty() operation becoming true (grepping for
".tx_empty" returned 81 hits for me). Also at least some of the usb to
serial adapters seem to support this using usb_serial_generic_wait_until_sent()
and ".tx_empty":

$ git grep "\.tx_empty"
cp210x.c:       .tx_empty               = cp210x_tx_empty,
f81534.c:       .tx_empty =             f81534_tx_empty,
ftdi_sio.c:     .tx_empty =             ftdi_tx_empty,
io_ti.c:        .tx_empty               = edge_tx_empty,
io_ti.c:        .tx_empty               = edge_tx_empty,
mxuport.c:      .tx_empty               = mxuport_tx_empty,
ti_usb_3410_5052.c:     .tx_empty               = ti_tx_empty,
ti_usb_3410_5052.c:     .tx_empty               = ti_tx_empty,

The other ones will only wait for empty s/w buffer, but that's
already the case for tty_wait_until_sent(), so IMHO a different
problem.

-- Sebastian

Patch
diff mbox series

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f4c6c90add78..a63b74031e22 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -173,6 +173,17 @@  void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 
+void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->wait_until_sent)
+		return;
+
+	ctrl->ops->wait_until_sent(ctrl, timeout);
+}
+EXPORT_SYMBOL_GPL(serdev_device_wait_until_sent);
+
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d05393594f15..50dc75c4d204 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -14,6 +14,7 @@ 
 #include <linux/serdev.h>
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
+#include <linux/poll.h>
 
 #define SERPORT_ACTIVE		1
 
@@ -46,11 +47,11 @@  static void ttyport_write_wakeup(struct tty_port *port)
 	struct serdev_controller *ctrl = port->client_data;
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 
-	if (!test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags))
-		return;
-
-	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+	if (test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags) &&
+	    test_bit(SERPORT_ACTIVE, &serport->flags))
 		serdev_controller_write_wakeup(ctrl);
+
+	wake_up_interruptible_poll(&port->tty->write_wait, POLLOUT);
 }
 
 static const struct tty_port_client_operations client_ops = {
@@ -167,6 +168,14 @@  static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable
 	tty_set_termios(tty, &ktermios);
 }
 
+static void ttyport_wait_until_sent(struct serdev_controller *ctrl, long timeout)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	tty_wait_until_sent(tty, timeout);
+}
+
 static const struct serdev_controller_ops ctrl_ops = {
 	.write_buf = ttyport_write_buf,
 	.write_flush = ttyport_write_flush,
@@ -175,6 +184,7 @@  static const struct serdev_controller_ops ctrl_ops = {
 	.close = ttyport_close,
 	.set_flow_control = ttyport_set_flow_control,
 	.set_baudrate = ttyport_set_baudrate,
+	.wait_until_sent = ttyport_wait_until_sent,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 9519da6253a8..a308b206d204 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -81,6 +81,7 @@  struct serdev_controller_ops {
 	void (*close)(struct serdev_controller *);
 	void (*set_flow_control)(struct serdev_controller *, bool);
 	unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);
+	void (*wait_until_sent)(struct serdev_controller *, long);
 };
 
 /**
@@ -186,6 +187,7 @@  int serdev_device_open(struct serdev_device *);
 void serdev_device_close(struct serdev_device *);
 unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
 void serdev_device_set_flow_control(struct serdev_device *, bool);
+void serdev_device_wait_until_sent(struct serdev_device *, long);
 int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
 void serdev_device_write_flush(struct serdev_device *);
 int serdev_device_write_room(struct serdev_device *);
@@ -223,6 +225,7 @@  static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev
 	return 0;
 }
 static inline void serdev_device_set_flow_control(struct serdev_device *sdev, bool enable) {}
+static inline void serdev_device_wait_until_sent(struct serdev_device *sdev, long timeout) {}
 static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsigned char *buf, size_t count)
 {
 	return -ENODEV;