tty: serdev: fix serdev_device_write return value

PATCH In review Rob Herring 4 participants 5 messages May 2, 2017 – May 3, 2017

Subsystems (2)

TTY layer and serial drivers Serial device bus

lore

Share & tools

lore

Rob Herring

author

2017-05-02 00:17 UTC #0 lore raw

PATCH 1/1 tty: serdev: fix serdev_device_write return value

Commit 6fe729c4bdae ("serdev: Add serdev_device_write subroutine") provides a compatibility wrapper for the existing serdev_device_write_buf, but it fails to return the number of bytes written causing users to timeout.

Fixes 6fe729c4bdae ("serdev: Add serdev_device_write subroutine")

Cc Andrey Smirnov <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Rob Herring <[email protected]>

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

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

index 433de5ea9b02..ccfe56355c4f 100644
@@ -127,7 +127,7 @@ int serdev_device_write(struct serdev_device *serdev,
 			unsigned long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
-	int ret;
+	int ret, wr_cnt = 0;
 
 	if (!ctrl || !ctrl->ops->write_buf ||
 	    (timeout && !serdev->ops->write_wakeup))

@@ -143,12 +143,13 @@ int serdev_device_write(struct serdev_device *serdev,
 
 		buf += ret;
 		count -= ret;
+		wr_cnt += ret;
 
 	} while (count &&
 		 (timeout = wait_for_completion_timeout(&serdev->write_comp,
 							timeout)));
 	mutex_unlock(&serdev->write_lock);
-	return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+	return ret < 0 ? ret : (count ? -ETIMEDOUT : wr_cnt);
 }
 EXPORT_SYMBOL_GPL(serdev_device_write);

Signature · 1 line

2.11.0

Johan Hovold

2017-05-02 09:25 UTC #1 lore raw

Quoted 4 lines · Rob Herring

Commit 6fe729c4bdae ("serdev: Add serdev_device_write subroutine") provides a compatibility wrapper for the existing serdev_device_write_buf, but it fails to return the number of bytes written causing users to timeout.

So this would also be fixed for serdev_device_write_buf() by Stefan Wahren's patch restoring that function implementation, but returning the amount written is perhaps desirable also for blocking writes for consistency reasons.

Quoted 24 lines

Fixes 6fe729c4bdae ("serdev: Add serdev_device_write subroutine")

Cc Andrey Smirnov <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Rob Herring <[email protected]>

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

drivers/tty/serdev/core.c +1 -1

index 433de5ea9b02..ccfe56355c4f 100644
@@ -127,7 +127,7 @@ int serdev_device_write(struct serdev_device *serdev,
 			unsigned long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
-	int ret;
+	int ret, wr_cnt = 0;
 
 	if (!ctrl || !ctrl->ops->write_buf ||
 	    (timeout && !serdev->ops->write_wakeup))

@@ -143,12 +143,13 @@ int serdev_device_write(struct serdev_device *serdev,
 
 		buf += ret;

drivers/tty/serdev/core.c int serdev_device_write(struct serdev_device *serdev, · line 153

Quoted 10 lines

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

 		count -= ret;
+		wr_cnt += ret;
 
 	} while (count &&
 		 (timeout = wait_for_completion_timeout(&serdev->write_comp,
 							timeout)));
 
 	mutex_unlock(&serdev->write_lock);
-	return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+	return ret < 0 ? ret : (count ? -ETIMEDOUT : wr_cnt);

That's some nasty use of the ternary operator. Ditching it completely would be more readable.

	if (ret < 0)
		return ret;

	if (count)
		return -ETIMEDOUT;

	return wr_count;

and here wr_count is the value of count passed to the function (and could just be stored on entry instead).

drivers/tty/serdev/core.c int serdev_device_write(struct serdev_device *serdev, · line 154

Quoted 2 lines

drivers/tty/serdev/core.c +0 -0
 }
 EXPORT_SYMBOL_GPL(serdev_device_write);

Thanks,
Johan

Rob Herring

author

2017-05-02 12:30 UTC #2 lore raw

Quoted 10 lines · Johan Hovold

Quoted 4 lines · Rob Herring
Commit 6fe729c4bdae ("serdev: Add serdev_device_write subroutine") provides a compatibility wrapper for the existing serdev_device_write_buf, but it fails to return the number of bytes written causing users to timeout.
So this would also be fixed for serdev_device_write_buf() by Stefan Wahren's patch restoring that function implementation, but returning the amount written is perhaps desirable also for blocking writes for consistency reasons.

Yes, I saw it after I wrote this. We should apply both IMO.

Quoted 46 lines

Quoted 24 lines

Fixes 6fe729c4bdae ("serdev: Add serdev_device_write subroutine")

Cc Andrey Smirnov <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Rob Herring <[email protected]>

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

drivers/tty/serdev/core.c +1 -1

index 433de5ea9b02..ccfe56355c4f 100644
@@ -127,7 +127,7 @@ int serdev_device_write(struct serdev_device *serdev,
                      unsigned long timeout)
 {
      struct serdev_controller *ctrl = serdev->ctrl;
-     int ret;
+     int ret, wr_cnt = 0;
 
      if (!ctrl || !ctrl->ops->write_buf ||
          (timeout && !serdev->ops->write_wakeup))

@@ -143,12 +143,13 @@ int serdev_device_write(struct serdev_device *serdev,
 
              buf += ret;

drivers/tty/serdev/core.c int serdev_device_write(struct serdev_device *serdev, · line 153

Quoted 10 lines

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

              count -= ret;
+             wr_cnt += ret;
 
      } while (count &&
               (timeout = wait_for_completion_timeout(&serdev->write_comp,
                                                      timeout)));
 
      mutex_unlock(&serdev->write_lock);
-     return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+     return ret < 0 ? ret : (count ? -ETIMEDOUT : wr_cnt);

That's some nasty use of the ternary operator. Ditching it completely would be more readable.

        if (ret < 0)
                return ret;

        if (count)
                return -ETIMEDOUT;

        return wr_count;

Quoted 2 lines

and here wr_count is the value of count passed to the function (and could just be stored on entry instead).

Okay.

I'll wait for Greg to apply Stefan's patch and respin on top of it.

Rob

Andy Shevchenko

2017-05-03 17:44 UTC #3 lore raw

In reply to Johan Hovold

Quoted 8 lines · Johan Hovold

drivers/tty/serdev/core.c int serdev_device_write(struct serdev_device *serdev, · line 152
Quoted 2 lines · Rob Herring
drivers/tty/serdev/core.c +1 -1
-     return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+     return ret < 0 ? ret : (count ? -ETIMEDOUT : wr_cnt);
That's some nasty use of the ternary operator. Ditching it completely would be more readable.

Quoted 7 lines

        if (ret < 0)
                return ret;

        if (count)
                return -ETIMEDOUT;

        return wr_count;

While I agree on the first part, I would go still with one ternary at the end:

            return count ? -ETIMEDOUT : wr_count;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2017-05-03 18:06 UTC #4 lore raw

Quoted 15 lines · Andy Shevchenko

Quoted 14 lines · Johan Hovold
drivers/tty/serdev/core.c int serdev_device_write(struct serdev_device *serdev, · line 152
Quoted 2 lines · Rob Herring
drivers/tty/serdev/core.c +1 -1
-     return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
+     return ret < 0 ? ret : (count ? -ETIMEDOUT : wr_cnt);
That's some nasty use of the ternary operator. Ditching it completely would be more readable.
        if (ret < 0)
                return ret;

        if (count)
                return -ETIMEDOUT;

Quoted 6 lines

Quoted 1 line
        return wr_count;
While I agree on the first part, I would go still with one ternary at the end:
            return count ? -ETIMEDOUT : wr_count;

Ick, no, make it easy to read, we write code for developers first, the compiler second. Ditching it completly is a good idea.

thanks,

greg k-h