serdev: make serdev_device_write() more usable

PATCH In review Johan Hovold 2 participants 9 messages Nov 14, 2018 – Nov 20, 2018

Subsystems (3)

TTY layer and serial drivers Serial device bus Gnss

lore

Share & tools

lore

Johan Hovold

author maintainer

2018-11-14 15:09 UTC #0 lore raw

cover serdev: make serdev_device_write() more usable

This series make the synchronous serdev_device_write() helper more usable by

1) allowing drivers to pass a zero timeout to indicate that they want to wait forever;

2) returning the number of bytes actually written (buffered) if the helper is interrupted;

3) make the helper use interruptible wait so that the helper can be used on behalf of user space.

Finally, the two write functions are documented using kernel-doc.

Turns out I was using the wrong timeout for two gnss drivers that expected the helper to wait indefinitely. I've fixed up those separately (by using MAX_SCHEDULE_TIMEOUT for now), but for the helper to be usable when using flow control we really want it to be interruptible.

Besides the two gnss drivers, there's currently only one other in-kernel user of this helper and that driver (rave-sp) uses a non-zero timeout and doesn't check the return value and therefore does not need to be updated.

Note that this series depends on the two above mentioned GNSS fixes (submitted for v4.20-rc3).

Johan

Johan Hovold (4):
serdev: use zero to indicate infinite write timeout
serdev: make synchronous write return bytes written
serdev: make synchronous write helper interruptible
serdev: document the write functions using kernel-doc

 drivers/gnss/serial.c     |  2 +-
 drivers/gnss/sirf.c       |  2 +-
 drivers/tty/serdev/core.c | 70 +++++++++++++++++++++++++++++++++++----
 include/linux/serdev.h    |  2 +-
 4 files changed, 66 insertions(+), 10 deletions(-)

Signature · 1 line

2.19.1

Johan Hovold

author maintainer

2018-11-14 15:09 UTC #1 lore raw

PATCH 1/4 serdev: use zero to indicate infinite write timeout

Use zero to indicate infinite timeout for the synchronous serdev_device_write() helper.

This allows drivers to specify an infinite timeout without knowing about serdev implementation details, while also allowing the same timeout argument to be used for both serdev_device_write() and serdev_device_wait_until_sent().

Note that passing zero to the current helper makes no sense; just call the asynchronous serdev_device_write_buf() directly instead.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/tty/serdev/core.c +5 -2

index 9db93f500b4e..c7d637d2bc56 100644
@@ -15,6 +15,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/sched.h>
 #include <linux/serdev.h>
 #include <linux/slab.h>

@@ -235,10 +236,12 @@ int serdev_device_write(struct serdev_device *serdev,
 	struct serdev_controller *ctrl = serdev->ctrl;
 	int ret;
 
-	if (!ctrl || !ctrl->ops->write_buf ||
-	    (timeout && !serdev->ops->write_wakeup))
+	if (!ctrl || !ctrl->ops->write_buf || !serdev->ops->write_wakeup)
 		return -EINVAL;
 
+	if (timeout == 0)
+		timeout = MAX_SCHEDULE_TIMEOUT;
+
 	mutex_lock(&serdev->write_lock);
 	do {
 		reinit_completion(&serdev->write_comp);

Signature · 1 line

2.19.1

Johan Hovold

author maintainer

2018-11-14 15:09 UTC #2 lore raw

PATCH 2/4 serdev: make synchronous write return bytes written

In reply to Johan Hovold

Make the synchronous serdev_device_write() helper behave analogous to the asynchronous serdev_device_write_buf() by returning the number of bytes written (or rather buffered) also on timeout.

This will allow drivers to distinguish the case where data was partially written from the case where no data was written.

Also update the only two users that checked the return value.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gnss/serial.c     |  2 +-
 drivers/gnss/sirf.c       |  2 +-
 drivers/tty/serdev/core.c | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

drivers/gnss/serial.c +1 -1

index 31e891f00175..def64b36d994 100644
@@ -65,7 +65,7 @@ static int gnss_serial_write_raw(struct gnss_device *gdev,
 
 	/* write is only buffered synchronously */
 	ret = serdev_device_write(serdev, buf, count, MAX_SCHEDULE_TIMEOUT);
-	if (ret < 0)
+	if (ret < 0 || ret < count)
 		return ret;
 
 	/* FIXME: determine if interrupted? */

drivers/gnss/sirf.c +1 -1

index 71d014edd167..b3a4c0e91947 100644
@@ -85,7 +85,7 @@ static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
 
 	/* write is only buffered synchronously */
 	ret = serdev_device_write(serdev, buf, count, MAX_SCHEDULE_TIMEOUT);
-	if (ret < 0)
+	if (ret < 0 || ret < count)
 		return ret;
 
 	/* FIXME: determine if interrupted? */

drivers/tty/serdev/core.c +10 -2

index c7d637d2bc56..ee4c40336633 100644
@@ -234,6 +234,7 @@ int serdev_device_write(struct serdev_device *serdev,
 			unsigned long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
+	int written = 0;
 	int ret;
 
 	if (!ctrl || !ctrl->ops->write_buf || !serdev->ops->write_wakeup)

@@ -250,14 +251,21 @@ int serdev_device_write(struct serdev_device *serdev,
 		if (ret < 0)
 			break;
 
+		written += ret;
 		buf += ret;
 		count -= ret;
-
 	} while (count &&
 		 (timeout = wait_for_completion_timeout(&serdev->write_comp,
 							timeout)));
 	mutex_unlock(&serdev->write_lock);
-	return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+
+	if (ret < 0)
+		return ret;
+
+	if (timeout == 0 && written == 0)
+		return -ETIMEDOUT;
+
+	return written;
 }
 EXPORT_SYMBOL_GPL(serdev_device_write);

Signature · 1 line

2.19.1

Rob Herring

2018-11-19 15:15 UTC #3 lore raw

Re: [PATCH 2/4] serdev: make synchronous write return bytes written

Quoted 22 lines · Johan Hovold

Make the synchronous serdev_device_write() helper behave analogous to the asynchronous serdev_device_write_buf() by returning the number of bytes written (or rather buffered) also on timeout.
This will allow drivers to distinguish the case where data was partially written from the case where no data was written.
Also update the only two users that checked the return value.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gnss/serial.c     |  2 +-
 drivers/gnss/sirf.c       |  2 +-
 drivers/tty/serdev/core.c | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)
drivers/gnss/serial.c +0 -0
index 31e891f00175..def64b36d994 100644

drivers/gnss/serial.c static int gnss_serial_write_raw(struct gnss_device *gdev, · line 68

Quoted 5 lines

drivers/gnss/serial.c +1 -1

@@ -65,7 +65,7 @@ static int gnss_serial_write_raw(struct gnss_device *gdev,
 
        /* write is only buffered synchronously */
        ret = serdev_device_write(serdev, buf, count, MAX_SCHEDULE_TIMEOUT);
-       if (ret < 0)
+       if (ret < 0 || ret < count)

The 2nd condition covers the 1st condition. Though I guess this will all change anyways when you address the FIXME below.

Quoted 3 lines

drivers/gnss/serial.c +0 -0

                return ret;
 
        /* FIXME: determine if interrupted? */

Johan Hovold

author maintainer

2018-11-20 17:09 UTC #4 lore raw

Re: [PATCH 2/4] serdev: make synchronous write return bytes written

Quoted 24 lines · Rob Herring

Quoted 23 lines · Johan Hovold
Make the synchronous serdev_device_write() helper behave analogous to the asynchronous serdev_device_write_buf() by returning the number of bytes written (or rather buffered) also on timeout.
This will allow drivers to distinguish the case where data was partially written from the case where no data was written.
Also update the only two users that checked the return value.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gnss/serial.c     |  2 +-
 drivers/gnss/sirf.c       |  2 +-
 drivers/tty/serdev/core.c | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)
drivers/gnss/serial.c +0 -0
index 31e891f00175..def64b36d994 100644
@@ -65,7 +65,7 @@ static int gnss_serial_write_raw(struct gnss_device *gdev,
 

Quoted 6 lines

drivers/gnss/serial.c static int gnss_serial_write_raw(struct gnss_device *gdev, · line 68
Quoted 4 lines
drivers/gnss/serial.c +1 -1
        /* write is only buffered synchronously */
        ret = serdev_device_write(serdev, buf, count, MAX_SCHEDULE_TIMEOUT);
-       if (ret < 0)
+       if (ret < 0 || ret < count)
The 2nd condition covers the 1st condition.

Actually it does not; ret is signed and count is unsigned, so this is needed to catch negative errnos which would otherwise become larger than count due to integer promotion.

Quoted 2 lines

Though I guess this will all change anyways when you address the FIXME below.

Not sure about that yet. If wait_until_sent is interrupted the data has already been buffered and we need to return that count. So I think I'll just drop the FIXME.

Quoted 3 lines

drivers/gnss/serial.c static int gnss_serial_write_raw(struct gnss_device *gdev, · line 71
Quoted 3 lines
drivers/gnss/serial.c +0 -0
                return ret;
 
        /* FIXME: determine if interrupted? */

Thanks,
Johan

Johan Hovold

author maintainer

2018-11-14 15:09 UTC #5 lore raw

PATCH 3/4 serdev: make synchronous write helper interruptible

In reply to Johan Hovold

Allow the synchronous serdev_device_write() helper to be interrupted.

This is useful for cases where I/O is performed on behalf of user space and we don't want to block indefinitely when using flow control.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/core.c | 20 ++++++++++++++------
 include/linux/serdev.h    |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

drivers/tty/serdev/core.c +14 -6

index ee4c40336633..c7006bbb793a 100644
@@ -231,7 +231,7 @@ EXPORT_SYMBOL_GPL(serdev_device_write_buf);
 
 int serdev_device_write(struct serdev_device *serdev,
 			const unsigned char *buf, size_t count,
-			unsigned long timeout)
+			long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
 	int written = 0;

@@ -254,16 +254,24 @@ int serdev_device_write(struct serdev_device *serdev,
 		written += ret;
 		buf += ret;
 		count -= ret;
-	} while (count &&
-		 (timeout = wait_for_completion_timeout(&serdev->write_comp,
-							timeout)));
+
+		if (count == 0)
+			break;
+
+		timeout = wait_for_completion_interruptible_timeout(&serdev->write_comp,
+								    timeout);
+	} while (timeout > 0);
 	mutex_unlock(&serdev->write_lock);
 
 	if (ret < 0)
 		return ret;
 
-	if (timeout == 0 && written == 0)
-		return -ETIMEDOUT;
+	if (timeout <= 0 && written == 0) {
+		if (timeout == -ERESTARTSYS)
+			return -ERESTARTSYS;
+		else
+			return -ETIMEDOUT;
+	}
 
 	return written;
 }

include/linux/serdev.h +1 -1

index f153b2c7f0cd..070bf4e92df7 100644
@@ -210,7 +210,7 @@ void serdev_device_wait_until_sent(struct serdev_device *, long);
 int serdev_device_get_tiocm(struct serdev_device *);
 int serdev_device_set_tiocm(struct serdev_device *, int, int);
 void serdev_device_write_wakeup(struct serdev_device *);
-int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
+int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, long);
 void serdev_device_write_flush(struct serdev_device *);
 int serdev_device_write_room(struct serdev_device *);

Signature · 1 line

2.19.1

Johan Hovold

author maintainer

2018-11-14 15:09 UTC #6 lore raw

PATCH 4/4 serdev: document the write functions using kernel-doc

In reply to Johan Hovold

Document the asynchronous serdev_device_write_buf() and synchronous serdev_device_write() functions using kernel-doc.

Specifically, mention that writing data only means that data has been buffered by the controller, and that the synchronous helper depends on serdev_device_write_wakeup() being called in the driver write_wakeup() callback.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

drivers/tty/serdev/core.c +37 -0

index c7006bbb793a..a0ac16ee6575 100644
@@ -217,6 +217,21 @@ void serdev_device_write_wakeup(struct serdev_device *serdev)
 }
 EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
 
+/**
+ * serdev_device_write_buf() - write data asynchronously
+ * @serdev:	serdev device
+ * @buf:	data to be written
+ * @count:	number of bytes to write
+ *
+ * Write data to the device asynchronously.
+ *
+ * Note that any accepted data has only been buffered by the controller; use
+ * serdev_device_wait_until_sent() to make sure the controller write buffer
+ * has actually been emptied.
+ *
+ * Return: The number of bytes written (less than count if not enough room in
+ * the write buffer), or a negative errno on errors.
+ */
 int serdev_device_write_buf(struct serdev_device *serdev,
 			    const unsigned char *buf, size_t count)
 {

@@ -229,6 +244,28 @@ int serdev_device_write_buf(struct serdev_device *serdev,
 }
 EXPORT_SYMBOL_GPL(serdev_device_write_buf);
 
+/**
+ * serdev_device_write() - write data synchronously
+ * @serdev:	serdev device
+ * @buf:	data to be written
+ * @count:	number of bytes to write
+ * @timeout:	timeout in jiffies, or 0 to wait indefinitely
+ *
+ * Write data to the device synchronously by repeatedly calling
+ * serdev_device_write() until the controller has accepted all data (unless
+ * interrupted by a timeout or a signal).
+ *
+ * Note that any accepted data has only been buffered by the controller; use
+ * serdev_device_wait_until_sent() to make sure the controller write buffer
+ * has actually been emptied.
+ *
+ * Note that this function depends on serdev_device_write_wakeup() being
+ * called in the serdev driver write_wakeup() callback.
+ *
+ * Return: The number of bytes written (less than count if interrupted),
+ * -ETIMEDOUT or -ERESTARTSYS if interrupted before any bytes were written, or
+ * a negative errno on errors.
+ */
 int serdev_device_write(struct serdev_device *serdev,
 			const unsigned char *buf, size_t count,
 			long timeout)

Signature · 1 line

2.19.1

Rob Herring

Reviewed-by on the v1 series

2018-11-19 15:16 UTC #7 lore raw

In reply to Johan Hovold

Quoted 28 lines · Johan Hovold

This series make the synchronous serdev_device_write() helper more usable by
1) allowing drivers to pass a zero timeout to indicate that they want to wait forever;
2) returning the number of bytes actually written (buffered) if the helper is interrupted;
3) make the helper use interruptible wait so that the helper can be used on behalf of user space.
Finally, the two write functions are documented using kernel-doc.
Turns out I was using the wrong timeout for two gnss drivers that expected the helper to wait indefinitely. I've fixed up those separately (by using MAX_SCHEDULE_TIMEOUT for now), but for the helper to be usable when using flow control we really want it to be interruptible.
Besides the two gnss drivers, there's currently only one other in-kernel user of this helper and that driver (rave-sp) uses a non-zero timeout and doesn't check the return value and therefore does not need to be updated.
Note that this series depends on the two above mentioned GNSS fixes (submitted for v4.20-rc3).

Quoted 8 lines

Johan
Johan Hovold (4):
serdev: use zero to indicate infinite write timeout
serdev: make synchronous write return bytes written
serdev: make synchronous write helper interruptible
serdev: document the write functions using kernel-doc

Other than the one nit, for the series:

Reviewed-by Rob Herring <[email protected]>

Johan Hovold

author maintainer

2018-11-20 17:11 UTC #8 lore raw

Quoted 38 lines · Rob Herring

Quoted 28 lines · Johan Hovold
This series make the synchronous serdev_device_write() helper more usable by
1) allowing drivers to pass a zero timeout to indicate that they want to wait forever;
2) returning the number of bytes actually written (buffered) if the helper is interrupted;
3) make the helper use interruptible wait so that the helper can be used on behalf of user space.
Finally, the two write functions are documented using kernel-doc.
Turns out I was using the wrong timeout for two gnss drivers that expected the helper to wait indefinitely. I've fixed up those separately (by using MAX_SCHEDULE_TIMEOUT for now), but for the helper to be usable when using flow control we really want it to be interruptible.
Besides the two gnss drivers, there's currently only one other in-kernel user of this helper and that driver (rave-sp) uses a non-zero timeout and doesn't check the return value and therefore does not need to be updated.
Note that this series depends on the two above mentioned GNSS fixes (submitted for v4.20-rc3).
Quoted 8 lines
Johan
Johan Hovold (4):
serdev: use zero to indicate infinite write timeout
serdev: make synchronous write return bytes written
serdev: make synchronous write helper interruptible
serdev: document the write functions using kernel-doc

Quoted 3 lines

Other than the one nit, for the series:
Reviewed-by Rob Herring <[email protected]>

Thanks for reviewing.

Greg, note that these still depend on the two gnss fixes currently in your char-misc-linus branch.

Johan