tty: serdev: fix serdev_device_write return value

Message ID 20170502001714.11576-1-robh@kernel.org
State New
Headers show

Commit Message

Rob Herring May 2, 2017, 12:17 a.m.
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 <andrew.smirnov@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Rob Herring <robh@kernel.org>

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

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Johan Hovold May 2, 2017, 9:25 a.m. | #1
On Mon, May 01, 2017 at 07:17:14PM -0500, Rob Herring wrote:
> 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.

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

> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

> ---

>  drivers/tty/serdev/core.c | 5 +++--

>  1 file changed, 3 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c

> index 433de5ea9b02..ccfe56355c4f 100644

> --- a/drivers/tty/serdev/core.c

> +++ b/drivers/tty/serdev/core.c

> @@ -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);


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

>  }

>  EXPORT_SYMBOL_GPL(serdev_device_write);


Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 2, 2017, 12:30 p.m. | #2
On Tue, May 2, 2017 at 4:25 AM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, May 01, 2017 at 07:17:14PM -0500, Rob Herring wrote:

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

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

>> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>

>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

>> ---

>>  drivers/tty/serdev/core.c | 5 +++--

>>  1 file changed, 3 insertions(+), 2 deletions(-)

>>

>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c

>> index 433de5ea9b02..ccfe56355c4f 100644

>> --- a/drivers/tty/serdev/core.c

>> +++ b/drivers/tty/serdev/core.c

>> @@ -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);

>

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


Okay.

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

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko May 3, 2017, 5:44 p.m. | #3
On Tue, May 2, 2017 at 12:25 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, May 01, 2017 at 07:17:14PM -0500, Rob Herring wrote:


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



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

            return count ? -ETIMEDOUT : wr_count;

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH May 3, 2017, 6:06 p.m. | #4
On Wed, May 03, 2017 at 08:44:07PM +0300, Andy Shevchenko wrote:
> On Tue, May 2, 2017 at 12:25 PM, Johan Hovold <johan@kernel.org> wrote:

> > On Mon, May 01, 2017 at 07:17:14PM -0500, Rob Herring wrote:

> 

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

> 

> 

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 433de5ea9b02..ccfe56355c4f 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -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);