[PATCH 0/4] serdev: make serdev_device_write() more usable

[PATCH 0/4] serdev: make serdev_device_write() more usable

[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).

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(-)

-- 
2.19.1

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

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/tty/serdev/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 9db93f500b4e..c7d637d2bc56 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -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);
-- 
2.19.1

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

[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 <johan@kernel.org>
---
 drivers/gnss/serial.c     |  2 +-
 drivers/gnss/sirf.c       |  2 +-
 drivers/tty/serdev/core.c | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
index 31e891f00175..def64b36d994 100644
--- a/drivers/gnss/serial.c
+++ b/drivers/gnss/serial.c
@@ -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? */
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 71d014edd167..b3a4c0e91947 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -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? */
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c7d637d2bc56..ee4c40336633 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -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);
 
-- 
2.19.1

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

[Rob Herring]

On Wed, Nov 14, 2018 at 9:10 AM Johan Hovold <johan@kernel.org> wrote:
>
> 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 <johan@kernel.org>
> ---
>  drivers/gnss/serial.c     |  2 +-
>  drivers/gnss/sirf.c       |  2 +-
>  drivers/tty/serdev/core.c | 12 ++++++++++--
>  3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
> index 31e891f00175..def64b36d994 100644
> --- a/drivers/gnss/serial.c
> +++ b/drivers/gnss/serial.c
> @@ -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.

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

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

[Johan Hovold]

On Mon, Nov 19, 2018 at 09:15:37AM -0600, Rob Herring wrote:
> On Wed, Nov 14, 2018 at 9:10 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > 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 <johan@kernel.org>
> > ---
> >  drivers/gnss/serial.c     |  2 +-
> >  drivers/gnss/sirf.c       |  2 +-
> >  drivers/tty/serdev/core.c | 12 ++++++++++--
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
> > index 31e891f00175..def64b36d994 100644
> > --- a/drivers/gnss/serial.c
> > +++ b/drivers/gnss/serial.c
> > @@ -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.

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.

> 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.

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

Thanks,
Johan

[PATCH 3/4] serdev: make synchronous write helper interruptible

[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 <johan@kernel.org>
---
 drivers/tty/serdev/core.c | 20 ++++++++++++++------
 include/linux/serdev.h    |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index ee4c40336633..c7006bbb793a 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -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;
 }
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index f153b2c7f0cd..070bf4e92df7 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -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 *);
 
-- 
2.19.1

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

[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 <johan@kernel.org>
---
 drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c7006bbb793a..a0ac16ee6575 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -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)
-- 
2.19.1

Re: [PATCH 0/4] serdev: make serdev_device_write() more usable

[Rob Herring]

On Wed, Nov 14, 2018 at 9:10 AM Johan Hovold <johan@kernel.org> wrote:
>
> 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

Other than the one nit, for the series:

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/4] serdev: make serdev_device_write() more usable

[Johan Hovold]

On Mon, Nov 19, 2018 at 09:16:35AM -0600, Rob Herring wrote:
> On Wed, Nov 14, 2018 at 9:10 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > 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
> 
> Other than the one nit, for the series:
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for reviewing.

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

Johan