| Message ID | 20170502001714.11576-1-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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);
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