lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Mar 2017 16:25:06 -0500 From: Rob Herring <robh@...nel.org> To: Andrey Smirnov <andrew.smirnov@...il.com> Cc: Andy Shevchenko <andy.shevchenko@...il.com>, Chris Healy <cphealy@...il.com>, Guenter Roeck <linux@...ck-us.net>, "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov <andrew.smirnov@...il.com> wrote: > On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko > <andy.shevchenko@...il.com> wrote: >> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov >> <andrew.smirnov@...il.com> wrote: >>> Convert serdev_device_write_buf's code to be able to transfer amount of >>> data potentially exceeding "write room" at the moment of invocation. >>> >>> To support that, also add serdev_device_write_wakeup. >>> >>> Drivers wanting to use full extent of serdev_device_write >>> functionality are expected to provide serdev_device_write_wakeup as a >>> sole handler of .write_wakeup event or call it as a part of driver's >>> custom .write_wakeup code. >>> >>> Drivers wanting to retain old serdev_device_write_buf behaviour can >> >>> replace those call to calls to serdev_device_write with timeout of >>> 0. Providing .write_wakeup handler in such case is optional. >> >> Some indentation would be better if, for example, 0 will be kept on >> previous line. >> > > OK, sure. > >> So, what I would see if no one objects is patch series of two: >> 1) introduction of new API >> 2) removing old one. >> >> It will benefit for easier review and any potential code anthropologist. >> > > Second version of the patch preserves the old API an just > re-implements it in terms of a new one. I am not sure I see the > benefit in splitting it into two patches, but I'll leave it up to Rob > to decide. I think it is fine as-is, but maybe the subject now is a bit misleading. >>> --- a/drivers/tty/serdev/core.c >>> +++ b/drivers/tty/serdev/core.c >>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev) >>> } >>> EXPORT_SYMBOL_GPL(serdev_device_close); >>> >>> -int serdev_device_write_buf(struct serdev_device *serdev, >>> - const unsigned char *buf, size_t count) >>> +void serdev_device_write_wakeup(struct serdev_device *serdev) >>> +{ >>> + complete(&serdev->write_comp); >>> +} >>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup); >>> + >>> +int serdev_device_write(struct serdev_device *serdev, >>> + const unsigned char *buf, size_t count, >>> + unsigned long timeout) >>> { >>> struct serdev_controller *ctrl = serdev->ctrl; >>> + int ret; >>> >>> - if (!ctrl || !ctrl->ops->write_buf) >>> + if (!ctrl || !ctrl->ops->write_buf || >>> + (timeout && !serdev->ops->write_wakeup)) >>> return -EINVAL; >>> >>> - return ctrl->ops->write_buf(ctrl, buf, count); >>> + mutex_lock(&serdev->write_lock); >>> + do { >>> + reinit_completion(&serdev->write_comp); >>> + >>> + ret = ctrl->ops->write_buf(ctrl, buf, count); >>> + if (ret < 0) >>> + break; >>> + >> >>> + buf += ret; >> >> Extra white spaces. > > Which is there on purpose to re-align "+=" with "-=" on the next line. > I'll remove it. > >> >>> + count -= ret; >>> + >> >>> + } while (count && >>> + (timeout = wait_for_completion_timeout(&serdev->write_comp, >>> + timeout))); >> >> So, would it be better to support interrupts here and return a >> corresponding error code to the user? >> > > I don't have a use-case for that and as far as I can tell, neither SPI > nor I2C slave device API offer such functionality universally, so I am > inclined to say no. Since the change from wait_for_completion to > wait_for_completion_timeout was made per Rob's request, I'd leave it > up to him to decided about this change as well. Honestly, I don't know. It's added easily enough if needed later. Rob
Powered by blists - more mailing lists