* Re: [PATCH v4 1/6] iio: Add output buffer support
@ 2021-08-22 15:49 kernel test robot
2021-08-25 15:31 ` kernel test robot
0 siblings, 1 reply; 18+ messages in thread
From: kernel test robot @ 2021-08-22 15:49 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 14984 bytes --]
CC: clang-built-linux(a)googlegroups.com
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210820165927.4524-2-mihail.chindris@analog.com>
References: <20210820165927.4524-2-mihail.chindris@analog.com>
TO: Mihail Chindris <mihail.chindris@analog.com>
TO: linux-kernel(a)vger.kernel.org
TO: linux-iio(a)vger.kernel.org
CC: lars(a)metafoo.de
CC: Michael.Hennerich(a)analog.com
CC: jic23(a)kernel.org
CC: nuno.sa(a)analog.com
CC: dragos.bogdan(a)analog.com
CC: alexandru.ardelean(a)analog.com
CC: Mihail Chindris <mihail.chindris@analog.com>
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-c007-20210822 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9e9d70591e72fc6762b4b9a226b68ed1307419bf)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
6 warnings generated.
drivers/iio/industrialio-buffer.c:221:15: warning: The left operand of '==' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
} while (ret == 0);
^
drivers/iio/industrialio-buffer.c:282:6: note: Assuming the condition is false
if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:282:2: note: Taking false branch
if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
^
drivers/iio/industrialio-buffer.c:285:9: note: Calling 'iio_buffer_write'
return iio_buffer_write(filp, buf, n, f_ps);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:181:2: note: 'ret' declared without an initial value
int ret;
^~~~~~~
drivers/iio/industrialio-buffer.c:183:6: note: Assuming 'rb' is non-null
if (!rb || !rb->access->write)
^~~
drivers/iio/industrialio-buffer.c:183:6: note: Left side of '||' is false
drivers/iio/industrialio-buffer.c:183:13: note: Assuming field 'write' is non-null
if (!rb || !rb->access->write)
^~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:183:2: note: Taking false branch
if (!rb || !rb->access->write)
^
drivers/iio/industrialio-buffer.c:192:6: note: Assuming 'datum_size' is not equal to 0
if (!datum_size)
^~~~~~~~~~~
drivers/iio/industrialio-buffer.c:192:2: note: Taking false branch
if (!datum_size)
^
drivers/iio/industrialio-buffer.c:195:6: note: Assuming the condition is false
if (filp->f_flags & O_NONBLOCK)
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:195:2: note: Taking false branch
if (filp->f_flags & O_NONBLOCK)
^
drivers/iio/industrialio-buffer.c:198:13: note: Assuming '__UNIQUE_ID___x247' is >= '__UNIQUE_ID___y248'
to_wait = min_t(size_t, n / datum_size, rb->watermark);
^
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^~~~~~~~~~
drivers/iio/industrialio-buffer.c:198:13: note: '?' condition is false
to_wait = min_t(size_t, n / datum_size, rb->watermark);
^
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^
drivers/iio/industrialio-buffer.c:202:7: note: Assuming field 'info' is non-null
if (!indio_dev->info) {
^~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:202:3: note: Taking false branch
if (!indio_dev->info) {
^
drivers/iio/industrialio-buffer.c:207:7: note: Assuming the condition is true
if (iio_buffer_space_available(rb) < to_wait) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:207:3: note: Taking true branch
if (iio_buffer_space_available(rb) < to_wait) {
^
drivers/iio/industrialio-buffer.c:208:8: note: Assuming the condition is false
if (signal_pending(current)) {
^~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:208:4: note: Taking false branch
if (signal_pending(current)) {
^
drivers/iio/industrialio-buffer.c:215:4: note: Execution continues on line 221
continue;
^
drivers/iio/industrialio-buffer.c:221:15: note: The left operand of '==' is a garbage value
} while (ret == 0);
~~~ ^
>> drivers/iio/industrialio-buffer.c:325:9: warning: Called function pointer is null (null dereference) [clang-analyzer-core.CallAndMessage]
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming 'buffer' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
drivers/iio/industrialio-buffer.c:322:17: note: Assuming field 'access' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:322:36: note: Assuming field 'remove_from' is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming pointer value is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:2: note: Taking false branch
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:325:9: note: Called function pointer is null (null dereference)
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:855:2: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
^
include/linux/list.h:630:13: note: expanded from macro 'list_for_each_entry'
pos = list_next_entry(pos, member))
^
include/linux/list.h:555:2: note: expanded from macro 'list_next_entry'
list_entry((pos)->member.next, typeof(*(pos)), member)
^
include/linux/list.h:511:2: note: expanded from macro 'list_entry'
container_of(ptr, type, member)
^
include/linux/kernel.h:494:25: note: expanded from macro 'container_of'
void *__mptr = (void *)(ptr); \
^
drivers/iio/industrialio-buffer.c:1306:30: note: Left side of '&&' is false
struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
^
include/linux/iio/sysfs.h:32:2: note: expanded from macro 'to_iio_dev_attr'
container_of(_dev_attr, struct iio_dev_attr, dev_attr)
^
include/linux/kernel.h:495:61: note: expanded from macro 'container_of'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
drivers/iio/industrialio-buffer.c:1306:30: note: Taking false branch
struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
^
include/linux/iio/sysfs.h:32:2: note: expanded from macro 'to_iio_dev_attr'
container_of(_dev_attr, struct iio_dev_attr, dev_attr)
^
include/linux/kernel.h:495:2: note: expanded from macro 'container_of'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
include/linux/compiler_types.h:328:2: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
^
include/linux/compiler_types.h:316:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
include/linux/compiler_types.h:308:3: note: expanded from macro '__compiletime_assert'
if (!(condition)) \
^
drivers/iio/industrialio-buffer.c:1306:30: note: Loop condition is false. Exiting loop
struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
^
include/linux/iio/sysfs.h:32:2: note: expanded from macro 'to_iio_dev_attr'
container_of(_dev_attr, struct iio_dev_attr, dev_attr)
^
include/linux/kernel.h:495:2: note: expanded from macro 'container_of'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
include/linux/compiler_types.h:328:2: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
^
include/linux/compiler_types.h:316:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
include/linux/compiler_types.h:306:2: note: expanded from macro '__compiletime_assert'
do { \
^
drivers/iio/industrialio-buffer.c:1309:8: note: Calling 'strtobool'
ret = strtobool(buf, &requested_state);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/kstrtox.h:152:9: note: Value assigned to 'requested_state', which participates in a condition later
return kstrtobool(s, res);
^~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:1309:8: note: Returning from 'strtobool'
ret = strtobool(buf, &requested_state);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:1310:6: note: Assuming 'ret' is >= 0
vim +325 drivers/iio/industrialio-buffer.c
d2f0a48f36aea3 Lars-Peter Clausen 2013-10-04 319
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 320 int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 321 {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 322 if (!buffer || !buffer->access || buffer->access->remove_from)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 323 return -EINVAL;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 324
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 @325 return buffer->access->remove_from(buffer, data);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 326 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 327 EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 328
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34492 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-22 15:49 [PATCH v4 1/6] iio: Add output buffer support kernel test robot
@ 2021-08-25 15:31 ` kernel test robot
0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-25 15:31 UTC (permalink / raw)
To: Mihail Chindris, Linux Kernel Mailing List, linux-iio
Cc: clang-built-linux, kbuild-all, lars, Michael.Hennerich, jic23,
nuno.sa, dragos.bogdan, alexandru.ardelean
[-- Attachment #1: Type: text/plain, Size: 4019 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-c007-20210822 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9e9d70591e72fc6762b4b9a226b68ed1307419bf)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
>> drivers/iio/industrialio-buffer.c:325:9: warning: Called function pointer is null (null dereference) [clang-analyzer-core.CallAndMessage]
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming 'buffer' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
drivers/iio/industrialio-buffer.c:322:17: note: Assuming field 'access' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:322:36: note: Assuming field 'remove_from' is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming pointer value is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:2: note: Taking false branch
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:325:9: note: Called function pointer is null (null dereference)
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +325 drivers/iio/industrialio-buffer.c
d2f0a48f36aea3 Lars-Peter Clausen 2013-10-04 319
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 320 int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 321 {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 322 if (!buffer || !buffer->access || buffer->access->remove_from)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 323 return -EINVAL;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 324
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 @325 return buffer->access->remove_from(buffer, data);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 326 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 327 EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 328
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34492 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
@ 2021-08-25 15:31 ` kernel test robot
0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-25 15:31 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4092 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-c007-20210822 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9e9d70591e72fc6762b4b9a226b68ed1307419bf)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
>> drivers/iio/industrialio-buffer.c:325:9: warning: Called function pointer is null (null dereference) [clang-analyzer-core.CallAndMessage]
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming 'buffer' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
drivers/iio/industrialio-buffer.c:322:17: note: Assuming field 'access' is non-null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Left side of '||' is false
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:322:36: note: Assuming field 'remove_from' is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:6: note: Assuming pointer value is null
if (!buffer || !buffer->access || buffer->access->remove_from)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/industrialio-buffer.c:322:2: note: Taking false branch
if (!buffer || !buffer->access || buffer->access->remove_from)
^
drivers/iio/industrialio-buffer.c:325:9: note: Called function pointer is null (null dereference)
return buffer->access->remove_from(buffer, data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +325 drivers/iio/industrialio-buffer.c
d2f0a48f36aea3 Lars-Peter Clausen 2013-10-04 319
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 320 int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 321 {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 322 if (!buffer || !buffer->access || buffer->access->remove_from)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 323 return -EINVAL;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 324
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 @325 return buffer->access->remove_from(buffer, data);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 326 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 327 EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 328
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34492 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v4 1/6] iio: Add output buffer support
2021-08-30 16:05 ` Jonathan Cameron
2021-09-01 8:54 ` Sa, Nuno
@ 2021-09-16 10:57 ` Chindris, Mihail
1 sibling, 0 replies; 18+ messages in thread
From: Chindris, Mihail @ 2021-09-16 10:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-iio, lars, Hennerich, Michael, Sa, Nuno,
Bogdan, Dragos, alexandru.ardelean
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, 30 August 2021 19:06
> To: Chindris, Mihail <Mihail.Chindris@analog.com>
> Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; lars@metafoo.de;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; Bogdan, Dragos <Dragos.Bogdan@analog.com>;
> alexandru.ardelean@analog.com
> Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
>
> On Fri, 20 Aug 2021 16:59:22 +0000
> Mihail Chindris <mihail.chindris@analog.com> wrote:
>
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Currently IIO only supports buffer mode for capture devices like ADCs.
> > Add support for buffered mode for output devices like DACs.
> >
> > The output buffer implementation is analogous to the input buffer
> > implementation. Instead of using read() to get data from the buffer
> > write() is used to copy data into the buffer.
> >
> > poll() with POLLOUT will wakeup if there is space available for more
> > or equal to the configured watermark of samples.
> >
> > Drivers can remove data from a buffer using
> > iio_buffer_remove_sample(), the function can e.g. called from a
> > trigger handler to write the data to hardware.
> >
> > A buffer can only be either a output buffer or an input, but not both.
> > So, for a device that has an ADC and DAC path, this will mean 2 IIO
> > buffers (one for each direction).
> >
> > The direction of the buffer is decided by the new direction field of
> > the iio_buffer struct and should be set after allocating and before
> > registering it.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> Hi Mihial,
>
> Welcome to IIO (I don't think I've seen you before?)
>
> Given the somewhat odd sign off trail I'd add some comments to the
> description (probably not saying that everyone who works on this ends up
> leaving Analog.
> It's not cursed! Really it's not ;) Lars and I discussed this at least 7+ years ago
> and he lasted ages at Analog after that *evil laugh*
>
> I'm not really clear how the concept of a watermark applies here. It feels like
> it's getting used for two unrelated things:
> 1) Space in buffer for polling form userspace. We let userspace know it can
> write more data once the watermark worth of scans is empty.
> 2) Writing to the kfifo. If a large write is attempted we do smaller writes to
> transfer some of the data into the kfifo which can then drain to the hardware.
> I can sort of see this might be beneficial as it provides batching.
> They are somewhat related but it's not totally clear to me they should be the
> same parameter. Perhaps we need some more docs to explain how watermark
> is used for output buffers?
>
> As it stands there are some corner cases around this that look ominous to me...
> See inline.
>
Hi Jonathan,
Yes, it my first time here, nice to meet you.
I will remove the watermark related code in the next version. It seems in our
Kernel it is not used and I can't think of an use case where it can really be needed.
> > ---
> > drivers/iio/iio_core.h | 4 +
> > drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++++++++++++++-
> > drivers/iio/industrialio-core.c | 1 +
> > include/linux/iio/buffer.h | 7 ++
> > include/linux/iio/buffer_impl.h | 11 +++
> > 5 files changed, 154 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h index
> > 8f4a9b264962..61e318431de9 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file *filp,
> > struct poll_table_struct *wait); ssize_t
> > iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> > size_t n, loff_t *f_ps);
> > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> > + size_t n, loff_t *f_ps);
> >
> > int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
> >
> > #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper) #define
> > iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
> >
> > void iio_disable_all_buffers(struct iio_dev *indio_dev); void
> > iio_buffer_wakeup_poll(struct iio_dev *indio_dev); @@ -83,6 +86,7 @@
> > void iio_device_detach_buffers(struct iio_dev *indio_dev);
> >
> > #define iio_buffer_poll_addr NULL
> > #define iio_buffer_read_outer_addr NULL
> > +#define iio_buffer_write_outer_addr NULL
> >
> > static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev) { diff --git a/drivers/iio/industrialio-buffer.c
> > b/drivers/iio/industrialio-buffer.c
> > index a95cc2da56be..73d4451a0572 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file *filp, char
> __user *buf,
> > return ret;
> > }
> >
> > +static size_t iio_buffer_space_available(struct iio_buffer *buf) {
> > + if (buf->access->space_available)
> > + return buf->access->space_available(buf);
> > +
> > + return SIZE_MAX;
> > +}
> > +
> > +static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > + struct iio_dev *indio_dev = ib->indio_dev;
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + size_t datum_size;
> > + size_t to_wait;
> > + int ret;
> > +
> > + if (!rb || !rb->access->write)
> > + return -EINVAL;
> > +
> > + datum_size = rb->bytes_per_datum;
> > +
> > + /*
> > + * If datum_size is 0 there will never be anything to read from the
> > + * buffer, so signal end of file now.
> > + */
> > + if (!datum_size)
> > + return 0;
> > +
> > + if (filp->f_flags & O_NONBLOCK)
> > + to_wait = 0;
> > + else
> > + to_wait = min_t(size_t, n / datum_size, rb->watermark);
>
> Why is the watermark relevant here? We need enough space for the data as
> written whatever the watermark is set to.
> Been a while since I looked at equivalent write path, but I think there we are
> interested in ensuring a hwfifo flushes out. I'm don't think the same concept
> exists in this direction.
>
> > +
> > + add_wait_queue(&rb->pollq, &wait);
> > + do {
> > + if (!indio_dev->info) {
> > + ret = -ENODEV;
> > + break;
> > + }
> > +
> > + if (iio_buffer_space_available(rb) < to_wait) {
> > + if (signal_pending(current)) {
> > + ret = -ERESTARTSYS;
> > + break;
> > + }
> > +
> > + wait_woken(&wait, TASK_INTERRUPTIBLE,
> > + MAX_SCHEDULE_TIMEOUT);
> > + continue;
> > + }
> > +
> > + ret = rb->access->write(rb, n, buf);
> > + if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> > + ret = -EAGAIN;
>
> Do we need to advance the buf pointer here and reduce n? We may have
> written some but not all the data.
>
> > + } while (ret == 0);
> > + remove_wait_queue(&rb->pollq, &wait);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * iio_buffer_poll() - poll the buffer to find out if it has data
> > * @filp: File structure pointer for device access
> > @@ -181,8 +244,18 @@ static __poll_t iio_buffer_poll(struct file *filp,
> > return 0;
> >
> > poll_wait(filp, &rb->pollq, wait);
> > - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > - return EPOLLIN | EPOLLRDNORM;
> > +
> > + switch (rb->direction) {
> > + case IIO_BUFFER_DIRECTION_IN:
> > + if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > + return EPOLLIN | EPOLLRDNORM;
> > + break;
> > + case IIO_BUFFER_DIRECTION_OUT:
> > + if (iio_buffer_space_available(rb) >= rb->watermark)
>
> That's interesting. We should probably make sure we update docs to make it
> clear that it has a different meaning for output buffers. Guess that might be
> later in this set though.
>
> > + return EPOLLOUT | EPOLLWRNORM;
> > + break;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -199,6 +272,19 @@ ssize_t iio_buffer_read_wrapper(struct file *filp,
> char __user *buf,
> > return iio_buffer_read(filp, buf, n, f_ps); }
> >
> > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > +
> > + /* check if buffer was opened through new API */
>
> This is new. We don't need to support the old API. If we can make sure it
> never appears in the old API at all that would be great.
>
If I don't add this function I can't write to /dev/iio:deviceX from bash and our current tools
won't work without it, ex. Libiio/iiod as I found doing some test. So, I think it will be useful
to have it, if it is ok.
> > + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> > + return -EBUSY;
> > +
> > + return iio_buffer_write(filp, buf, n, f_ps); }
> > +
> > __poll_t iio_buffer_poll_wrapper(struct file *filp,
> > struct poll_table_struct *wait)
> > {
> > @@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev
> *indio_dev)
> > }
> > }
> >
> > +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
>
> sample or scan? Sample would be a single value for a single channel, scan
> would be updates for all channels that are enabled.
>
> > +{
> > + if (!buffer || !buffer->access || buffer->access->remove_from)
> > + return -EINVAL;
> > +
> > + return buffer->access->remove_from(buffer, data); }
> > +EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
> > +
> > void iio_buffer_init(struct iio_buffer *buffer) {
> > INIT_LIST_HEAD(&buffer->demux_list);
> > @@ -807,6 +902,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
> > }
> >
> > if (insert_buffer) {
> > + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> > + strict_scanmask = true;
>
> As below, I'm surprised we can get to here..
>
> > bitmap_or(compound_mask, compound_mask,
> > insert_buffer->scan_mask, indio_dev->masklength);
> > scan_timestamp |= insert_buffer->scan_timestamp; @@ -
> 948,6 +1045,8
> > @@ static int iio_update_demux(struct iio_dev *indio_dev)
> > int ret;
> >
> > list_for_each_entry(buffer, &iio_dev_opaque->buffer_list,
> > buffer_list) {
> > + if (buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> > + continue;
>
> Given the below, how did it get into the list? I think that check should be
> enough that we don't need to check it elsewhere.
>
> > ret = iio_buffer_update_demux(indio_dev, buffer);
> > if (ret < 0)
> > goto error_clear_mux_table;
> > @@ -1159,6 +1258,11 @@ int iio_update_buffers(struct iio_dev *indio_dev,
> > mutex_lock(&iio_dev_opaque->info_exist_lock);
> > mutex_lock(&indio_dev->mlock);
> >
> > + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT) {
>
> Can you do this outside of the lock as a sanity check before this function really
> gets going?
>
> > + ret = -EINVAL;
> > + goto out_unlock;
> > + }
> > +
> > if (insert_buffer && iio_buffer_is_active(insert_buffer))
> > insert_buffer = NULL;
> >
> > @@ -1277,6 +1381,22 @@ static ssize_t
> iio_dma_show_data_available(struct device *dev,
> > return sysfs_emit(buf, "%zu\n", iio_buffer_data_available(buffer));
> > }
> >
> > +static ssize_t direction_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
> > +
> > + switch (buffer->direction) {
> > + case IIO_BUFFER_DIRECTION_IN:
> > + return sprintf(buf, "in\n");
> > + case IIO_BUFFER_DIRECTION_OUT:
> > + return sprintf(buf, "out\n");
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> > iio_buffer_write_length);
> > static struct device_attribute dev_attr_length_ro = __ATTR(length, @@
> > -1289,12 +1409,20 @@ static struct device_attribute dev_attr_watermark_ro
> = __ATTR(watermark,
> > S_IRUGO, iio_buffer_show_watermark, NULL); static
> > DEVICE_ATTR(data_available, S_IRUGO,
> > iio_dma_show_data_available, NULL);
> > +static DEVICE_ATTR_RO(direction);
> >
> > +/**
> > + * When adding new attributes here, put the at the end, at least
> > +until
> > + * the code that handles the lengh/length_ro & watermark/watermark_ro
> > + * assignments gets cleaned up. Otherwise these can create some weird
> > + * duplicate attributes errors under some setups.
> > + */
> > static struct attribute *iio_buffer_attrs[] = {
> > &dev_attr_length.attr,
> > &dev_attr_enable.attr,
> > &dev_attr_watermark.attr,
> > &dev_attr_data_available.attr,
> > + &dev_attr_direction.attr,
> > };
> >
> > #define to_dev_attr(_attr) container_of(_attr, struct
> > device_attribute, attr) @@ -1397,6 +1525,7 @@ static const struct
> file_operations iio_buffer_chrdev_fileops = {
> > .owner = THIS_MODULE,
> > .llseek = noop_llseek,
> > .read = iio_buffer_read,
> > + .write = iio_buffer_write,
> > .poll = iio_buffer_poll,
> > .release = iio_buffer_chrdev_release, }; diff --git
> > a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 2dbb37e09b8c..537a08549a69 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1822,6 +1822,7 @@ static const struct file_operations
> iio_buffer_fileops = {
> > .owner = THIS_MODULE,
> > .llseek = noop_llseek,
> > .read = iio_buffer_read_outer_addr,
> > + .write = iio_buffer_write_outer_addr,
> > .poll = iio_buffer_poll_addr,
> > .unlocked_ioctl = iio_ioctl,
> > .compat_ioctl = compat_ptr_ioctl,
> > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> > index b6928ac5c63d..e87b8773253d 100644
> > --- a/include/linux/iio/buffer.h
> > +++ b/include/linux/iio/buffer.h
> > @@ -11,8 +11,15 @@
> >
> > struct iio_buffer;
> >
> > +enum iio_buffer_direction {
> > + IIO_BUFFER_DIRECTION_IN,
> > + IIO_BUFFER_DIRECTION_OUT,
> > +};
> > +
> > int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
> >
> > +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data);
> > +
> > /**
> > * iio_push_to_buffers_with_timestamp() - push data and timestamp to
> buffers
> > * @indio_dev: iio_dev structure for device.
> > diff --git a/include/linux/iio/buffer_impl.h
> > b/include/linux/iio/buffer_impl.h index 245b32918ae1..8a44c5321e19
> > 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -7,6 +7,7 @@
> > #ifdef CONFIG_IIO_BUFFER
> >
> > #include <uapi/linux/iio/buffer.h>
> > +#include <linux/iio/buffer.h>
>
> >
> > struct iio_dev;
> > struct iio_buffer;
> > @@ -23,6 +24,10 @@ struct iio_buffer;
> > * @read: try to get a specified number of bytes (must exist)
> > * @data_available: indicates how much data is available for reading from
> > * the buffer.
> > + * @remove_from: remove sample from buffer. Drivers should calls this to
> > + * remove a sample from a buffer.
> > + * @write: try to write a number of bytes
> > + * @space_available: returns the amount of bytes available in a buffer
> > * @request_update: if a parameter change has been marked, update
> underlying
> > * storage.
> > * @set_bytes_per_datum:set number of bytes per datum @@ -49,6 +54,9
> > @@ struct iio_buffer_access_funcs {
> > int (*store_to)(struct iio_buffer *buffer, const void *data);
> > int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
> > size_t (*data_available)(struct iio_buffer *buffer);
> > + int (*remove_from)(struct iio_buffer *buffer, void *data);
> > + int (*write)(struct iio_buffer *buffer, size_t n, const char __user *buf);
> > + size_t (*space_available)(struct iio_buffer *buffer);
> >
> > int (*request_update)(struct iio_buffer *buffer);
> >
> > @@ -80,6 +88,9 @@ struct iio_buffer {
> > /** @bytes_per_datum: Size of individual datum including timestamp.
> */
> > size_t bytes_per_datum;
> >
> > + /* @direction: Direction of the data stream (in/out). */
> > + enum iio_buffer_direction direction;
> > +
> > /**
> > * @access: Buffer access functions associated with the
> > * implementation.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-09-01 8:54 ` Sa, Nuno
@ 2021-09-05 9:55 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-09-05 9:55 UTC (permalink / raw)
To: Sa, Nuno
Cc: Chindris, Mihail, linux-kernel, linux-iio, lars, Hennerich,
Michael, Bogdan, Dragos
> > > + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> > > + return -EBUSY;
> > > +
> > > + return iio_buffer_write(filp, buf, n, f_ps);
> > > +}
> > > +
> > > __poll_t iio_buffer_poll_wrapper(struct file *filp,
> > > struct poll_table_struct *wait)
> > > {
> > > @@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev
> > *indio_dev)
> > > }
> > > }
> > >
> > > +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
> >
> > sample or scan? Sample would be a single value for a single channel,
> > scan would be updates for all channels that are enabled.
>
> Maybe iio_pop_from_buffer()? To be consistent with iio_push_to_buffer()..
Works for me.
J
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-09-01 8:50 ` Sa, Nuno
@ 2021-09-05 9:54 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-09-05 9:54 UTC (permalink / raw)
To: Sa, Nuno
Cc: Nuno Sá,
Chindris, Mihail, linux-kernel, linux-iio, lars, Hennerich,
Michael, Bogdan, Dragos
On Wed, 1 Sep 2021 08:50:25 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Monday, August 30, 2021 5:43 PM
> > To: Nuno Sá <noname.nuno@gmail.com>
> > Cc: Chindris, Mihail <Mihail.Chindris@analog.com>; linux-
> > kernel@vger.kernel.org; linux-iio@vger.kernel.org; lars@metafoo.de;
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno
> > <Nuno.Sa@analog.com>; Bogdan, Dragos
> > <Dragos.Bogdan@analog.com>; alexandru.ardelean@analog.com
> > Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
> >
> > On Mon, 23 Aug 2021 15:50:14 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> > > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > >
> > > > Currently IIO only supports buffer mode for capture devices like
> > > > ADCs. Add
> > > > support for buffered mode for output devices like DACs.
> > > >
> > > > The output buffer implementation is analogous to the input buffer
> > > > implementation. Instead of using read() to get data from the
> > buffer
> > > > write()
> > > > is used to copy data into the buffer.
> > > >
> > > > poll() with POLLOUT will wakeup if there is space available for more
> > > > or
> > > > equal to the configured watermark of samples.
> > > >
> > > > Drivers can remove data from a buffer using
> > > > iio_buffer_remove_sample(), the
> > > > function can e.g. called from a trigger handler to write the data to
> > > > hardware.
> > > >
> > > > A buffer can only be either a output buffer or an input, but not
> > > > both. So,
> > > > for a device that has an ADC and DAC path, this will mean 2 IIO
> > > > buffers
> > > > (one for each direction).
> > > >
> > > > The direction of the buffer is decided by the new direction field of
> > > > the
> > > > iio_buffer struct and should be set after allocating and before
> > > > registering
> > > > it.
> > > >
> > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > > Signed-off-by: Alexandru Ardelean
> > <alexandru.ardelean@analog.com>
> > > > Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> > > > ---
> > > > drivers/iio/iio_core.h | 4 +
> > > > drivers/iio/industrialio-buffer.c | 133
> > > > +++++++++++++++++++++++++++++-
> > > > drivers/iio/industrialio-core.c | 1 +
> > > > include/linux/iio/buffer.h | 7 ++
> > > > include/linux/iio/buffer_impl.h | 11 +++
> > > > 5 files changed, 154 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > index 8f4a9b264962..61e318431de9 100644
> > > > --- a/drivers/iio/iio_core.h
> > > > +++ b/drivers/iio/iio_core.h
> > > > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> > > > *filp,
> > > > struct poll_table_struct *wait);
> > > > ssize_t iio_buffer_read_wrapper(struct file *filp, char __user
> > *buf,
> > > > size_t n, loff_t *f_ps);
> > > > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> > > > __user *buf,
> > > > + size_t n, loff_t *f_ps);
> > > >
> > > > int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > > > void iio_buffers_free_sysfs_and_mask(struct iio_dev
> > *indio_dev);
> > > >
> > > > #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > > > #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > > > +#define iio_buffer_write_outer_addr
> > (&iio_buffer_write_wrapper)
> > > >
> > > > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > > > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > > > @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> > > > *indio_dev);
> > > >
> > > > #define iio_buffer_poll_addr NULL
> > > > #define iio_buffer_read_outer_addr NULL
> > > > +#define iio_buffer_write_outer_addr NULL
> > > >
> > > > static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> > > > *indio_dev)
> > > > {
> > > > diff --git a/drivers/iio/industrialio-buffer.c
> > > > b/drivers/iio/industrialio-buffer.c
> > > > index a95cc2da56be..73d4451a0572 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> > > > *filp, char __user *buf,
> > > > return ret;
> > > > }
> > > >
> > > > +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> > > > +{
> > > > + if (buf->access->space_available)
> > > > + return buf->access->space_available(buf);
> > > > +
> > > > + return SIZE_MAX;
> > > > +}
> > > > +
> > > > +static ssize_t iio_buffer_write(struct file *filp, const char __user
> > > > *buf,
> > > > + size_t n, loff_t *f_ps)
> > > > +{
> > > > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > > > + struct iio_buffer *rb = ib->buffer;
> > > > + struct iio_dev *indio_dev = ib->indio_dev;
> > > > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > > > + size_t datum_size;
> > > > + size_t to_wait;
> > > > + int ret;
> > > > +
> > >
> > > Even though I do not agree that this is suficient, we should have the
> > > same check as we have for input buffer:
> > >
> > >
> > > if (!indio_dev->info)
> > > return -ENODEV;
> > >
> > > > + if (!rb || !rb->access->write)
> > > > + return -EINVAL;
> > > > +
> > > > + datum_size = rb->bytes_per_datum;
> > > > +
> > > > + /*
> > > > + * If datum_size is 0 there will never be anything to read from
> > > > the
> > > > + * buffer, so signal end of file now.
> > > > + */
> > > > + if (!datum_size)
> > > > + return 0;
> > > > +
> > > > + if (filp->f_flags & O_NONBLOCK)
> > > > + to_wait = 0;
> > > > + else
> > > > + to_wait = min_t(size_t, n / datum_size, rb-
> > >watermark);
> > > > +
> > >
> > > I had a bit of a crazy thought... Typically, output devices do not
> > > really have a typical trigger as we are used to have in input devices.
> > > Hence, typically we just use a hrtimer (maybe a pwm-trigger can also
> > be
> > > added) trigger where we kind of poll the buffer for new data to send
> > to
> > > the device.
> >
> > htrimer-trigger effectively gives you the hrtimer option but lets you
> > swap
> > it for other types. Maybe updating your DAC in sync with an ADC
> > dataready to
> > change some input to a network you are measuring?
> >
> > An external pwm type trigger (effectively a hwtrigger) would indeed
> > be
> > useful for much the same reason those get used for ADCs on SoCs. For
> > ADCs
> > that are doing self clocking you can think of that as an internal pwm
> > trigger
> > that we can't see at all.
> >
> > > So, I was wondering if we could just optionally bypass the
> > > kbuf path in output devices (via some optional buffer option)? At this
> > > point, we pretty much already know that we have data to consume
> > :).
> > > This would be kind of a synchronous interface. One issue I see with
> > > this is that we cannot really have a deterministic (or close) sampling
> > > frequency as we have for example with a pwm based trigger.
> >
> > If you are running without a buffer, the overhead of sysfs is unlikely to
> > be a particularly big problem, so do it that way if you want synchronous
> > control. The purpose of the buffer is to smooth out those interactions.
>
> What I meant was kind of just bypassing the buffer implementation
> (in typical cases, kfifo) not the complete buffer infrastructure (as passing
> a buffer with all scan bytes is better that writing each element
> separately in sysfs)... So, we
> would still have the write file ops but we would have a mechanism to
> synchronously pass control the driver interested in this data. My point
> was that at this level, we already now we have data for 'someone' to
> consume. Taking a second look I guess we could even keep the FIFO
> (even though, at first glance, it would not make much sense). Or maybe,
> we could have a special trigger as you said where we would just do
> ' iio_trigger_poll()' from the write() fops...
Given a write to the device takes a finite amount of time (often quite large)
you still want buffering even if potentially you want to update the device
as quickly as possible. So you need to have some sort of management thread
in place.
For input devices we have something similar in
https://elixir.bootlin.com/linux/latest/source/drivers/iio/trigger/iio-trig-loop.c
but things seem a little fiddlier for an output device given you want some
sort of 'loop until kfifo' empty. Perhaps pausing the thread at that point
and waking it up again once there is new data would work.
>
> > I guess it 'might' make sense to have a special trigger that is a similar
> > to the poll trigger in that it will push data to the device as quickly
> > as possible as long as there is some there... That would look like a
> > synchronous interface.
> >
>
> Anyways, as I said, this was just me throwing some stuff into the air. Mos likely,
> this does not bring that much added value when compared with the possible
> issues and subtleties I'm not seeing now.. Probably not doable and definitely
> not a priority for now.
Definitely doable but agreed it needs a user who cares! The one mentioned
above was for drones where the most important thing was to get the data
off the sensors as fast as it possibly could. Initial implementation had
a thread added to certain drivers to do this, but we moved to the loop trigger
as a cleaner way to do it without having to modify lots of drivers.
That code was the only one that ever got a "Tested-by: It's flying behind me" :)
Jonathan
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v4 1/6] iio: Add output buffer support
2021-08-30 16:05 ` Jonathan Cameron
@ 2021-09-01 8:54 ` Sa, Nuno
2021-09-05 9:55 ` Jonathan Cameron
2021-09-16 10:57 ` Chindris, Mihail
1 sibling, 1 reply; 18+ messages in thread
From: Sa, Nuno @ 2021-09-01 8:54 UTC (permalink / raw)
To: Jonathan Cameron, Chindris, Mihail
Cc: linux-kernel, linux-iio, lars, Hennerich, Michael, Bogdan, Dragos
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, August 30, 2021 6:06 PM
> To: Chindris, Mihail <Mihail.Chindris@analog.com>
> Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org;
> lars@metafoo.de; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; Bogdan, Dragos
> <Dragos.Bogdan@analog.com>; alexandru.ardelean@analog.com
> Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
>
> On Fri, 20 Aug 2021 16:59:22 +0000
> Mihail Chindris <mihail.chindris@analog.com> wrote:
>
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Currently IIO only supports buffer mode for capture devices like
> ADCs. Add
> > support for buffered mode for output devices like DACs.
> >
> > The output buffer implementation is analogous to the input buffer
> > implementation. Instead of using read() to get data from the buffer
> write()
> > is used to copy data into the buffer.
> >
> > poll() with POLLOUT will wakeup if there is space available for more
> or
> > equal to the configured watermark of samples.
> >
> > Drivers can remove data from a buffer using
> iio_buffer_remove_sample(), the
> > function can e.g. called from a trigger handler to write the data to
> > hardware.
> >
> > A buffer can only be either a output buffer or an input, but not both.
> So,
> > for a device that has an ADC and DAC path, this will mean 2 IIO
> buffers
> > (one for each direction).
> >
> > The direction of the buffer is decided by the new direction field of
> the
> > iio_buffer struct and should be set after allocating and before
> registering
> > it.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean
> <alexandru.ardelean@analog.com>
> > Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> Hi Mihial,
>
> Welcome to IIO (I don't think I've seen you before?)
>
> Given the somewhat odd sign off trail I'd add some comments to the
> description
> (probably not saying that everyone who works on this ends up leaving
> Analog.
> It's not cursed! Really it's not ;) Lars and I discussed this at least 7+
> years
> ago and he lasted ages at Analog after that *evil laugh*
>
> I'm not really clear how the concept of a watermark applies here. It
> feels
> like it's getting used for two unrelated things:
> 1) Space in buffer for polling form userspace. We let userspace know it
> can
> write more data once the watermark worth of scans is empty.
> 2) Writing to the kfifo. If a large write is attempted we do smaller
> writes to
> transfer some of the data into the kfifo which can then drain to the
> hardware.
> I can sort of see this might be beneficial as it provides batching.
> They are somewhat related but it's not totally clear to me they should
> be the
> same parameter. Perhaps we need some more docs to explain how
> watermark is
> used for output buffers?
>
> As it stands there are some corner cases around this that look ominous
> to me...
> See inline.
>
> > ---
> > drivers/iio/iio_core.h | 4 +
> > drivers/iio/industrialio-buffer.c | 133
> +++++++++++++++++++++++++++++-
> > drivers/iio/industrialio-core.c | 1 +
> > include/linux/iio/buffer.h | 7 ++
> > include/linux/iio/buffer_impl.h | 11 +++
> > 5 files changed, 154 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 8f4a9b264962..61e318431de9 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> *filp,
> > struct poll_table_struct *wait);
> > ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> > size_t n, loff_t *f_ps);
> > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user
> *buf,
> > + size_t n, loff_t *f_ps);
> >
> > int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
> >
> > #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > +#define iio_buffer_write_outer_addr
> (&iio_buffer_write_wrapper)
> >
> > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> *indio_dev);
> >
> > #define iio_buffer_poll_addr NULL
> > #define iio_buffer_read_outer_addr NULL
> > +#define iio_buffer_write_outer_addr NULL
> >
> > static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> *indio_dev)
> > {
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> buffer.c
> > index a95cc2da56be..73d4451a0572 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file *filp,
> char __user *buf,
> > return ret;
> > }
> >
> > +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> > +{
> > + if (buf->access->space_available)
> > + return buf->access->space_available(buf);
> > +
> > + return SIZE_MAX;
> > +}
> > +
> > +static ssize_t iio_buffer_write(struct file *filp, const char __user
> *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > + struct iio_dev *indio_dev = ib->indio_dev;
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + size_t datum_size;
> > + size_t to_wait;
> > + int ret;
> > +
> > + if (!rb || !rb->access->write)
> > + return -EINVAL;
> > +
> > + datum_size = rb->bytes_per_datum;
> > +
> > + /*
> > + * If datum_size is 0 there will never be anything to read from
> the
> > + * buffer, so signal end of file now.
> > + */
> > + if (!datum_size)
> > + return 0;
> > +
> > + if (filp->f_flags & O_NONBLOCK)
> > + to_wait = 0;
> > + else
> > + to_wait = min_t(size_t, n / datum_size, rb-
> >watermark);
>
> Why is the watermark relevant here? We need enough space for the
> data
> as written whatever the watermark is set to.
> Been a while since I looked at equivalent write path, but I think there
> we are interested in ensuring a hwfifo flushes out. I'm don't think
> the same concept exists in this direction.
>
> > +
> > + add_wait_queue(&rb->pollq, &wait);
> > + do {
> > + if (!indio_dev->info) {
> > + ret = -ENODEV;
> > + break;
> > + }
> > +
> > + if (iio_buffer_space_available(rb) < to_wait) {
> > + if (signal_pending(current)) {
> > + ret = -ERESTARTSYS;
> > + break;
> > + }
> > +
> > + wait_woken(&wait, TASK_INTERRUPTIBLE,
> > + MAX_SCHEDULE_TIMEOUT);
> > + continue;
> > + }
> > +
> > + ret = rb->access->write(rb, n, buf);
> > + if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> > + ret = -EAGAIN;
>
> Do we need to advance the buf pointer here and reduce n? We may
> have written
> some but not all the data.
>
> > + } while (ret == 0);
> > + remove_wait_queue(&rb->pollq, &wait);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * iio_buffer_poll() - poll the buffer to find out if it has data
> > * @filp: File structure pointer for device access
> > @@ -181,8 +244,18 @@ static __poll_t iio_buffer_poll(struct file
> *filp,
> > return 0;
> >
> > poll_wait(filp, &rb->pollq, wait);
> > - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > - return EPOLLIN | EPOLLRDNORM;
> > +
> > + switch (rb->direction) {
> > + case IIO_BUFFER_DIRECTION_IN:
> > + if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > + return EPOLLIN | EPOLLRDNORM;
> > + break;
> > + case IIO_BUFFER_DIRECTION_OUT:
> > + if (iio_buffer_space_available(rb) >= rb->watermark)
>
> That's interesting. We should probably make sure we update docs to
> make
> it clear that it has a different meaning for output buffers. Guess that
> might be later in this set though.
>
> > + return EPOLLOUT | EPOLLWRNORM;
> > + break;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -199,6 +272,19 @@ ssize_t iio_buffer_read_wrapper(struct file
> *filp, char __user *buf,
> > return iio_buffer_read(filp, buf, n, f_ps);
> > }
> >
> > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user
> *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > +
> > + /* check if buffer was opened through new API */
>
> This is new. We don't need to support the old API. If we can make
> sure
> it never appears in the old API at all that would be great.
>
> > + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> > + return -EBUSY;
> > +
> > + return iio_buffer_write(filp, buf, n, f_ps);
> > +}
> > +
> > __poll_t iio_buffer_poll_wrapper(struct file *filp,
> > struct poll_table_struct *wait)
> > {
> > @@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev
> *indio_dev)
> > }
> > }
> >
> > +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
>
> sample or scan? Sample would be a single value for a single channel,
> scan would be updates for all channels that are enabled.
Maybe iio_pop_from_buffer()? To be consistent with iio_push_to_buffer()...
- Nuno Sá
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v4 1/6] iio: Add output buffer support
2021-08-30 15:42 ` Jonathan Cameron
@ 2021-09-01 8:50 ` Sa, Nuno
2021-09-05 9:54 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Sa, Nuno @ 2021-09-01 8:50 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sá
Cc: Chindris, Mihail, linux-kernel, linux-iio, lars, Hennerich,
Michael, Bogdan, Dragos
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, August 30, 2021 5:43 PM
> To: Nuno Sá <noname.nuno@gmail.com>
> Cc: Chindris, Mihail <Mihail.Chindris@analog.com>; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org; lars@metafoo.de;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; Bogdan, Dragos
> <Dragos.Bogdan@analog.com>; alexandru.ardelean@analog.com
> Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
>
> On Mon, 23 Aug 2021 15:50:14 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > >
> > > Currently IIO only supports buffer mode for capture devices like
> > > ADCs. Add
> > > support for buffered mode for output devices like DACs.
> > >
> > > The output buffer implementation is analogous to the input buffer
> > > implementation. Instead of using read() to get data from the
> buffer
> > > write()
> > > is used to copy data into the buffer.
> > >
> > > poll() with POLLOUT will wakeup if there is space available for more
> > > or
> > > equal to the configured watermark of samples.
> > >
> > > Drivers can remove data from a buffer using
> > > iio_buffer_remove_sample(), the
> > > function can e.g. called from a trigger handler to write the data to
> > > hardware.
> > >
> > > A buffer can only be either a output buffer or an input, but not
> > > both. So,
> > > for a device that has an ADC and DAC path, this will mean 2 IIO
> > > buffers
> > > (one for each direction).
> > >
> > > The direction of the buffer is decided by the new direction field of
> > > the
> > > iio_buffer struct and should be set after allocating and before
> > > registering
> > > it.
> > >
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean
> <alexandru.ardelean@analog.com>
> > > Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> > > ---
> > > drivers/iio/iio_core.h | 4 +
> > > drivers/iio/industrialio-buffer.c | 133
> > > +++++++++++++++++++++++++++++-
> > > drivers/iio/industrialio-core.c | 1 +
> > > include/linux/iio/buffer.h | 7 ++
> > > include/linux/iio/buffer_impl.h | 11 +++
> > > 5 files changed, 154 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 8f4a9b264962..61e318431de9 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> > > *filp,
> > > struct poll_table_struct *wait);
> > > ssize_t iio_buffer_read_wrapper(struct file *filp, char __user
> *buf,
> > > size_t n, loff_t *f_ps);
> > > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> > > __user *buf,
> > > + size_t n, loff_t *f_ps);
> > >
> > > int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > > void iio_buffers_free_sysfs_and_mask(struct iio_dev
> *indio_dev);
> > >
> > > #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > > #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > > +#define iio_buffer_write_outer_addr
> (&iio_buffer_write_wrapper)
> > >
> > > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > > @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> > > *indio_dev);
> > >
> > > #define iio_buffer_poll_addr NULL
> > > #define iio_buffer_read_outer_addr NULL
> > > +#define iio_buffer_write_outer_addr NULL
> > >
> > > static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > {
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index a95cc2da56be..73d4451a0572 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> > > *filp, char __user *buf,
> > > return ret;
> > > }
> > >
> > > +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> > > +{
> > > + if (buf->access->space_available)
> > > + return buf->access->space_available(buf);
> > > +
> > > + return SIZE_MAX;
> > > +}
> > > +
> > > +static ssize_t iio_buffer_write(struct file *filp, const char __user
> > > *buf,
> > > + size_t n, loff_t *f_ps)
> > > +{
> > > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > > + struct iio_buffer *rb = ib->buffer;
> > > + struct iio_dev *indio_dev = ib->indio_dev;
> > > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > > + size_t datum_size;
> > > + size_t to_wait;
> > > + int ret;
> > > +
> >
> > Even though I do not agree that this is suficient, we should have the
> > same check as we have for input buffer:
> >
> >
> > if (!indio_dev->info)
> > return -ENODEV;
> >
> > > + if (!rb || !rb->access->write)
> > > + return -EINVAL;
> > > +
> > > + datum_size = rb->bytes_per_datum;
> > > +
> > > + /*
> > > + * If datum_size is 0 there will never be anything to read from
> > > the
> > > + * buffer, so signal end of file now.
> > > + */
> > > + if (!datum_size)
> > > + return 0;
> > > +
> > > + if (filp->f_flags & O_NONBLOCK)
> > > + to_wait = 0;
> > > + else
> > > + to_wait = min_t(size_t, n / datum_size, rb-
> >watermark);
> > > +
> >
> > I had a bit of a crazy thought... Typically, output devices do not
> > really have a typical trigger as we are used to have in input devices.
> > Hence, typically we just use a hrtimer (maybe a pwm-trigger can also
> be
> > added) trigger where we kind of poll the buffer for new data to send
> to
> > the device.
>
> htrimer-trigger effectively gives you the hrtimer option but lets you
> swap
> it for other types. Maybe updating your DAC in sync with an ADC
> dataready to
> change some input to a network you are measuring?
>
> An external pwm type trigger (effectively a hwtrigger) would indeed
> be
> useful for much the same reason those get used for ADCs on SoCs. For
> ADCs
> that are doing self clocking you can think of that as an internal pwm
> trigger
> that we can't see at all.
>
> > So, I was wondering if we could just optionally bypass the
> > kbuf path in output devices (via some optional buffer option)? At this
> > point, we pretty much already know that we have data to consume
> :).
> > This would be kind of a synchronous interface. One issue I see with
> > this is that we cannot really have a deterministic (or close) sampling
> > frequency as we have for example with a pwm based trigger.
>
> If you are running without a buffer, the overhead of sysfs is unlikely to
> be a particularly big problem, so do it that way if you want synchronous
> control. The purpose of the buffer is to smooth out those interactions.
What I meant was kind of just bypassing the buffer implementation
(in typical cases, kfifo) not the complete buffer infrastructure (as passing
a buffer with all scan bytes is better that writing each element
separately in sysfs)... So, we
would still have the write file ops but we would have a mechanism to
synchronously pass control the driver interested in this data. My point
was that at this level, we already now we have data for 'someone' to
consume. Taking a second look I guess we could even keep the FIFO
(even though, at first glance, it would not make much sense). Or maybe,
we could have a special trigger as you said where we would just do
' iio_trigger_poll()' from the write() fops...
> I guess it 'might' make sense to have a special trigger that is a similar
> to the poll trigger in that it will push data to the device as quickly
> as possible as long as there is some there... That would look like a
> synchronous interface.
>
Anyways, as I said, this was just me throwing some stuff into the air. Mos likely,
this does not bring that much added value when compared with the possible
issues and subtleties I'm not seeing now.. Probably not doable and definitely
not a priority for now.
- Nuno Sá
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
` (3 preceding siblings ...)
2021-08-25 8:35 ` Alexandru Ardelean
@ 2021-08-30 16:05 ` Jonathan Cameron
2021-09-01 8:54 ` Sa, Nuno
2021-09-16 10:57 ` Chindris, Mihail
4 siblings, 2 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-08-30 16:05 UTC (permalink / raw)
To: Mihail Chindris
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, alexandru.ardelean
On Fri, 20 Aug 2021 16:59:22 +0000
Mihail Chindris <mihail.chindris@analog.com> wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> Currently IIO only supports buffer mode for capture devices like ADCs. Add
> support for buffered mode for output devices like DACs.
>
> The output buffer implementation is analogous to the input buffer
> implementation. Instead of using read() to get data from the buffer write()
> is used to copy data into the buffer.
>
> poll() with POLLOUT will wakeup if there is space available for more or
> equal to the configured watermark of samples.
>
> Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
> function can e.g. called from a trigger handler to write the data to
> hardware.
>
> A buffer can only be either a output buffer or an input, but not both. So,
> for a device that has an ADC and DAC path, this will mean 2 IIO buffers
> (one for each direction).
>
> The direction of the buffer is decided by the new direction field of the
> iio_buffer struct and should be set after allocating and before registering
> it.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
Hi Mihial,
Welcome to IIO (I don't think I've seen you before?)
Given the somewhat odd sign off trail I'd add some comments to the description
(probably not saying that everyone who works on this ends up leaving Analog.
It's not cursed! Really it's not ;) Lars and I discussed this at least 7+ years
ago and he lasted ages at Analog after that *evil laugh*
I'm not really clear how the concept of a watermark applies here. It feels
like it's getting used for two unrelated things:
1) Space in buffer for polling form userspace. We let userspace know it can
write more data once the watermark worth of scans is empty.
2) Writing to the kfifo. If a large write is attempted we do smaller writes to
transfer some of the data into the kfifo which can then drain to the hardware.
I can sort of see this might be beneficial as it provides batching.
They are somewhat related but it's not totally clear to me they should be the
same parameter. Perhaps we need some more docs to explain how watermark is
used for output buffers?
As it stands there are some corner cases around this that look ominous to me...
See inline.
> ---
> drivers/iio/iio_core.h | 4 +
> drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++++++++++++++-
> drivers/iio/industrialio-core.c | 1 +
> include/linux/iio/buffer.h | 7 ++
> include/linux/iio/buffer_impl.h | 11 +++
> 5 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 8f4a9b264962..61e318431de9 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file *filp,
> struct poll_table_struct *wait);
> ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> size_t n, loff_t *f_ps);
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps);
>
> int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
>
> #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
>
> void iio_disable_all_buffers(struct iio_dev *indio_dev);
> void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev *indio_dev);
>
> #define iio_buffer_poll_addr NULL
> #define iio_buffer_read_outer_addr NULL
> +#define iio_buffer_write_outer_addr NULL
>
> static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> {
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a95cc2da56be..73d4451a0572 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
> return ret;
> }
>
> +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> +{
> + if (buf->access->space_available)
> + return buf->access->space_available(buf);
> +
> + return SIZE_MAX;
> +}
> +
> +static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> + struct iio_dev *indio_dev = ib->indio_dev;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + size_t datum_size;
> + size_t to_wait;
> + int ret;
> +
> + if (!rb || !rb->access->write)
> + return -EINVAL;
> +
> + datum_size = rb->bytes_per_datum;
> +
> + /*
> + * If datum_size is 0 there will never be anything to read from the
> + * buffer, so signal end of file now.
> + */
> + if (!datum_size)
> + return 0;
> +
> + if (filp->f_flags & O_NONBLOCK)
> + to_wait = 0;
> + else
> + to_wait = min_t(size_t, n / datum_size, rb->watermark);
Why is the watermark relevant here? We need enough space for the data
as written whatever the watermark is set to.
Been a while since I looked at equivalent write path, but I think there
we are interested in ensuring a hwfifo flushes out. I'm don't think
the same concept exists in this direction.
> +
> + add_wait_queue(&rb->pollq, &wait);
> + do {
> + if (!indio_dev->info) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + if (iio_buffer_space_available(rb) < to_wait) {
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + break;
> + }
> +
> + wait_woken(&wait, TASK_INTERRUPTIBLE,
> + MAX_SCHEDULE_TIMEOUT);
> + continue;
> + }
> +
> + ret = rb->access->write(rb, n, buf);
> + if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> + ret = -EAGAIN;
Do we need to advance the buf pointer here and reduce n? We may have written
some but not all the data.
> + } while (ret == 0);
> + remove_wait_queue(&rb->pollq, &wait);
> +
> + return ret;
> +}
> +
> /**
> * iio_buffer_poll() - poll the buffer to find out if it has data
> * @filp: File structure pointer for device access
> @@ -181,8 +244,18 @@ static __poll_t iio_buffer_poll(struct file *filp,
> return 0;
>
> poll_wait(filp, &rb->pollq, wait);
> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> - return EPOLLIN | EPOLLRDNORM;
> +
> + switch (rb->direction) {
> + case IIO_BUFFER_DIRECTION_IN:
> + if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> + return EPOLLIN | EPOLLRDNORM;
> + break;
> + case IIO_BUFFER_DIRECTION_OUT:
> + if (iio_buffer_space_available(rb) >= rb->watermark)
That's interesting. We should probably make sure we update docs to make
it clear that it has a different meaning for output buffers. Guess that
might be later in this set though.
> + return EPOLLOUT | EPOLLWRNORM;
> + break;
> + }
> +
> return 0;
> }
>
> @@ -199,6 +272,19 @@ ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> return iio_buffer_read(filp, buf, n, f_ps);
> }
>
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> +
> + /* check if buffer was opened through new API */
This is new. We don't need to support the old API. If we can make sure
it never appears in the old API at all that would be great.
> + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> + return -EBUSY;
> +
> + return iio_buffer_write(filp, buf, n, f_ps);
> +}
> +
> __poll_t iio_buffer_poll_wrapper(struct file *filp,
> struct poll_table_struct *wait)
> {
> @@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> }
> }
>
> +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
sample or scan? Sample would be a single value for a single channel,
scan would be updates for all channels that are enabled.
> +{
> + if (!buffer || !buffer->access || buffer->access->remove_from)
> + return -EINVAL;
> +
> + return buffer->access->remove_from(buffer, data);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
> +
> void iio_buffer_init(struct iio_buffer *buffer)
> {
> INIT_LIST_HEAD(&buffer->demux_list);
> @@ -807,6 +902,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
> }
>
> if (insert_buffer) {
> + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> + strict_scanmask = true;
As below, I'm surprised we can get to here..
> bitmap_or(compound_mask, compound_mask,
> insert_buffer->scan_mask, indio_dev->masklength);
> scan_timestamp |= insert_buffer->scan_timestamp;
> @@ -948,6 +1045,8 @@ static int iio_update_demux(struct iio_dev *indio_dev)
> int ret;
>
> list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
> + if (buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> + continue;
Given the below, how did it get into the list? I think that check should be
enough that we don't need to check it elsewhere.
> ret = iio_buffer_update_demux(indio_dev, buffer);
> if (ret < 0)
> goto error_clear_mux_table;
> @@ -1159,6 +1258,11 @@ int iio_update_buffers(struct iio_dev *indio_dev,
> mutex_lock(&iio_dev_opaque->info_exist_lock);
> mutex_lock(&indio_dev->mlock);
>
> + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT) {
Can you do this outside of the lock as a sanity check before this function really
gets going?
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> if (insert_buffer && iio_buffer_is_active(insert_buffer))
> insert_buffer = NULL;
>
> @@ -1277,6 +1381,22 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
> return sysfs_emit(buf, "%zu\n", iio_buffer_data_available(buffer));
> }
>
> +static ssize_t direction_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
> +
> + switch (buffer->direction) {
> + case IIO_BUFFER_DIRECTION_IN:
> + return sprintf(buf, "in\n");
> + case IIO_BUFFER_DIRECTION_OUT:
> + return sprintf(buf, "out\n");
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> iio_buffer_write_length);
> static struct device_attribute dev_attr_length_ro = __ATTR(length,
> @@ -1289,12 +1409,20 @@ static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
> S_IRUGO, iio_buffer_show_watermark, NULL);
> static DEVICE_ATTR(data_available, S_IRUGO,
> iio_dma_show_data_available, NULL);
> +static DEVICE_ATTR_RO(direction);
>
> +/**
> + * When adding new attributes here, put the at the end, at least until
> + * the code that handles the lengh/length_ro & watermark/watermark_ro
> + * assignments gets cleaned up. Otherwise these can create some weird
> + * duplicate attributes errors under some setups.
> + */
> static struct attribute *iio_buffer_attrs[] = {
> &dev_attr_length.attr,
> &dev_attr_enable.attr,
> &dev_attr_watermark.attr,
> &dev_attr_data_available.attr,
> + &dev_attr_direction.attr,
> };
>
> #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> @@ -1397,6 +1525,7 @@ static const struct file_operations iio_buffer_chrdev_fileops = {
> .owner = THIS_MODULE,
> .llseek = noop_llseek,
> .read = iio_buffer_read,
> + .write = iio_buffer_write,
> .poll = iio_buffer_poll,
> .release = iio_buffer_chrdev_release,
> };
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2dbb37e09b8c..537a08549a69 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1822,6 +1822,7 @@ static const struct file_operations iio_buffer_fileops = {
> .owner = THIS_MODULE,
> .llseek = noop_llseek,
> .read = iio_buffer_read_outer_addr,
> + .write = iio_buffer_write_outer_addr,
> .poll = iio_buffer_poll_addr,
> .unlocked_ioctl = iio_ioctl,
> .compat_ioctl = compat_ptr_ioctl,
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index b6928ac5c63d..e87b8773253d 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -11,8 +11,15 @@
>
> struct iio_buffer;
>
> +enum iio_buffer_direction {
> + IIO_BUFFER_DIRECTION_IN,
> + IIO_BUFFER_DIRECTION_OUT,
> +};
> +
> int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
>
> +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data);
> +
> /**
> * iio_push_to_buffers_with_timestamp() - push data and timestamp to buffers
> * @indio_dev: iio_dev structure for device.
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 245b32918ae1..8a44c5321e19 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -7,6 +7,7 @@
> #ifdef CONFIG_IIO_BUFFER
>
> #include <uapi/linux/iio/buffer.h>
> +#include <linux/iio/buffer.h>
>
> struct iio_dev;
> struct iio_buffer;
> @@ -23,6 +24,10 @@ struct iio_buffer;
> * @read: try to get a specified number of bytes (must exist)
> * @data_available: indicates how much data is available for reading from
> * the buffer.
> + * @remove_from: remove sample from buffer. Drivers should calls this to
> + * remove a sample from a buffer.
> + * @write: try to write a number of bytes
> + * @space_available: returns the amount of bytes available in a buffer
> * @request_update: if a parameter change has been marked, update underlying
> * storage.
> * @set_bytes_per_datum:set number of bytes per datum
> @@ -49,6 +54,9 @@ struct iio_buffer_access_funcs {
> int (*store_to)(struct iio_buffer *buffer, const void *data);
> int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
> size_t (*data_available)(struct iio_buffer *buffer);
> + int (*remove_from)(struct iio_buffer *buffer, void *data);
> + int (*write)(struct iio_buffer *buffer, size_t n, const char __user *buf);
> + size_t (*space_available)(struct iio_buffer *buffer);
>
> int (*request_update)(struct iio_buffer *buffer);
>
> @@ -80,6 +88,9 @@ struct iio_buffer {
> /** @bytes_per_datum: Size of individual datum including timestamp. */
> size_t bytes_per_datum;
>
> + /* @direction: Direction of the data stream (in/out). */
> + enum iio_buffer_direction direction;
> +
> /**
> * @access: Buffer access functions associated with the
> * implementation.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-23 13:50 ` Nuno Sá
@ 2021-08-30 15:42 ` Jonathan Cameron
2021-09-01 8:50 ` Sa, Nuno
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2021-08-30 15:42 UTC (permalink / raw)
To: Nuno Sá
Cc: Mihail Chindris, linux-kernel, linux-iio, lars,
Michael.Hennerich, nuno.sa, dragos.bogdan, alexandru.ardelean
On Mon, 23 Aug 2021 15:50:14 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Currently IIO only supports buffer mode for capture devices like
> > ADCs. Add
> > support for buffered mode for output devices like DACs.
> >
> > The output buffer implementation is analogous to the input buffer
> > implementation. Instead of using read() to get data from the buffer
> > write()
> > is used to copy data into the buffer.
> >
> > poll() with POLLOUT will wakeup if there is space available for more
> > or
> > equal to the configured watermark of samples.
> >
> > Drivers can remove data from a buffer using
> > iio_buffer_remove_sample(), the
> > function can e.g. called from a trigger handler to write the data to
> > hardware.
> >
> > A buffer can only be either a output buffer or an input, but not
> > both. So,
> > for a device that has an ADC and DAC path, this will mean 2 IIO
> > buffers
> > (one for each direction).
> >
> > The direction of the buffer is decided by the new direction field of
> > the
> > iio_buffer struct and should be set after allocating and before
> > registering
> > it.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> > ---
> > drivers/iio/iio_core.h | 4 +
> > drivers/iio/industrialio-buffer.c | 133
> > +++++++++++++++++++++++++++++-
> > drivers/iio/industrialio-core.c | 1 +
> > include/linux/iio/buffer.h | 7 ++
> > include/linux/iio/buffer_impl.h | 11 +++
> > 5 files changed, 154 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 8f4a9b264962..61e318431de9 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> > *filp,
> > struct poll_table_struct *wait);
> > ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> > size_t n, loff_t *f_ps);
> > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> > __user *buf,
> > + size_t n, loff_t *f_ps);
> >
> > int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
> >
> > #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
> >
> > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> > *indio_dev);
> >
> > #define iio_buffer_poll_addr NULL
> > #define iio_buffer_read_outer_addr NULL
> > +#define iio_buffer_write_outer_addr NULL
> >
> > static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > {
> > diff --git a/drivers/iio/industrialio-buffer.c
> > b/drivers/iio/industrialio-buffer.c
> > index a95cc2da56be..73d4451a0572 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> > *filp, char __user *buf,
> > return ret;
> > }
> >
> > +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> > +{
> > + if (buf->access->space_available)
> > + return buf->access->space_available(buf);
> > +
> > + return SIZE_MAX;
> > +}
> > +
> > +static ssize_t iio_buffer_write(struct file *filp, const char __user
> > *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > + struct iio_dev *indio_dev = ib->indio_dev;
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + size_t datum_size;
> > + size_t to_wait;
> > + int ret;
> > +
>
> Even though I do not agree that this is suficient, we should have the
> same check as we have for input buffer:
>
>
> if (!indio_dev->info)
> return -ENODEV;
>
> > + if (!rb || !rb->access->write)
> > + return -EINVAL;
> > +
> > + datum_size = rb->bytes_per_datum;
> > +
> > + /*
> > + * If datum_size is 0 there will never be anything to read from
> > the
> > + * buffer, so signal end of file now.
> > + */
> > + if (!datum_size)
> > + return 0;
> > +
> > + if (filp->f_flags & O_NONBLOCK)
> > + to_wait = 0;
> > + else
> > + to_wait = min_t(size_t, n / datum_size, rb->watermark);
> > +
>
> I had a bit of a crazy thought... Typically, output devices do not
> really have a typical trigger as we are used to have in input devices.
> Hence, typically we just use a hrtimer (maybe a pwm-trigger can also be
> added) trigger where we kind of poll the buffer for new data to send to
> the device.
htrimer-trigger effectively gives you the hrtimer option but lets you swap
it for other types. Maybe updating your DAC in sync with an ADC dataready to
change some input to a network you are measuring?
An external pwm type trigger (effectively a hwtrigger) would indeed be
useful for much the same reason those get used for ADCs on SoCs. For ADCs
that are doing self clocking you can think of that as an internal pwm trigger
that we can't see at all.
> So, I was wondering if we could just optionally bypass the
> kbuf path in output devices (via some optional buffer option)? At this
> point, we pretty much already know that we have data to consume :).
> This would be kind of a synchronous interface. One issue I see with
> this is that we cannot really have a deterministic (or close) sampling
> frequency as we have for example with a pwm based trigger.
If you are running without a buffer, the overhead of sysfs is unlikely to
be a particularly big problem, so do it that way if you want synchronous
control. The purpose of the buffer is to smooth out those interactions.
I guess it 'might' make sense to have a special trigger that is a similar
to the poll trigger in that it will push data to the device as quickly
as possible as long as there is some there... That would look like a
synchronous interface.
>
> Anyways, just me throwing some ideas. This is not the most important
> thing for now...
>
> - Nuno Sá
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
` (2 preceding siblings ...)
2021-08-23 13:50 ` Nuno Sá
@ 2021-08-25 8:35 ` Alexandru Ardelean
2021-08-30 16:05 ` Jonathan Cameron
4 siblings, 0 replies; 18+ messages in thread
From: Alexandru Ardelean @ 2021-08-25 8:35 UTC (permalink / raw)
To: Mihail Chindris
Cc: LKML, linux-iio, Lars-Peter Clausen, Hennerich, Michael,
Jonathan Cameron, Nuno Sá,
Bogdan, Dragos, Alexandru Ardelean
On Fri, Aug 20, 2021 at 8:01 PM Mihail Chindris
<mihail.chindris@analog.com> wrote:
>
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> Currently IIO only supports buffer mode for capture devices like ADCs. Add
> support for buffered mode for output devices like DACs.
>
> The output buffer implementation is analogous to the input buffer
> implementation. Instead of using read() to get data from the buffer write()
> is used to copy data into the buffer.
>
> poll() with POLLOUT will wakeup if there is space available for more or
> equal to the configured watermark of samples.
>
> Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
> function can e.g. called from a trigger handler to write the data to
> hardware.
>
> A buffer can only be either a output buffer or an input, but not both. So,
> for a device that has an ADC and DAC path, this will mean 2 IIO buffers
> (one for each direction).
>
> The direction of the buffer is decided by the new direction field of the
> iio_buffer struct and should be set after allocating and before registering
> it.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> ---
> drivers/iio/iio_core.h | 4 +
> drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++++++++++++++-
> drivers/iio/industrialio-core.c | 1 +
> include/linux/iio/buffer.h | 7 ++
> include/linux/iio/buffer_impl.h | 11 +++
> 5 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 8f4a9b264962..61e318431de9 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file *filp,
> struct poll_table_struct *wait);
> ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> size_t n, loff_t *f_ps);
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps);
>
> int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
>
> #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
>
> void iio_disable_all_buffers(struct iio_dev *indio_dev);
> void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev *indio_dev);
>
> #define iio_buffer_poll_addr NULL
> #define iio_buffer_read_outer_addr NULL
> +#define iio_buffer_write_outer_addr NULL
>
> static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> {
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a95cc2da56be..73d4451a0572 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
> return ret;
> }
>
> +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> +{
> + if (buf->access->space_available)
> + return buf->access->space_available(buf);
> +
> + return SIZE_MAX;
> +}
> +
> +static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> + struct iio_dev *indio_dev = ib->indio_dev;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + size_t datum_size;
> + size_t to_wait;
> + int ret;
> +
> + if (!rb || !rb->access->write)
> + return -EINVAL;
> +
> + datum_size = rb->bytes_per_datum;
> +
> + /*
> + * If datum_size is 0 there will never be anything to read from the
> + * buffer, so signal end of file now.
> + */
> + if (!datum_size)
> + return 0;
> +
> + if (filp->f_flags & O_NONBLOCK)
> + to_wait = 0;
> + else
> + to_wait = min_t(size_t, n / datum_size, rb->watermark);
> +
> + add_wait_queue(&rb->pollq, &wait);
> + do {
> + if (!indio_dev->info) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + if (iio_buffer_space_available(rb) < to_wait) {
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + break;
> + }
> +
> + wait_woken(&wait, TASK_INTERRUPTIBLE,
> + MAX_SCHEDULE_TIMEOUT);
> + continue;
> + }
> +
> + ret = rb->access->write(rb, n, buf);
> + if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> + ret = -EAGAIN;
> + } while (ret == 0);
> + remove_wait_queue(&rb->pollq, &wait);
> +
> + return ret;
> +}
> +
> /**
> * iio_buffer_poll() - poll the buffer to find out if it has data
> * @filp: File structure pointer for device access
> @@ -181,8 +244,18 @@ static __poll_t iio_buffer_poll(struct file *filp,
> return 0;
>
> poll_wait(filp, &rb->pollq, wait);
> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> - return EPOLLIN | EPOLLRDNORM;
> +
> + switch (rb->direction) {
> + case IIO_BUFFER_DIRECTION_IN:
> + if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> + return EPOLLIN | EPOLLRDNORM;
> + break;
> + case IIO_BUFFER_DIRECTION_OUT:
> + if (iio_buffer_space_available(rb) >= rb->watermark)
> + return EPOLLOUT | EPOLLWRNORM;
> + break;
> + }
> +
> return 0;
> }
>
> @@ -199,6 +272,19 @@ ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> return iio_buffer_read(filp, buf, n, f_ps);
> }
>
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
> + size_t n, loff_t *f_ps)
> +{
I was wondering about adding this wrapper or not.
It's technically allowing some classical buffer access for DACs by
just writing to the /dev/iio:deviceX chardev.
Which should be fine.
And [personally] I do like this convenience for simple DACs.
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> +
> + /* check if buffer was opened through new API */
> + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> + return -EBUSY;
> +
> + return iio_buffer_write(filp, buf, n, f_ps);
> +}
> +
> __poll_t iio_buffer_poll_wrapper(struct file *filp,
> struct poll_table_struct *wait)
> {
> @@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> }
> }
>
> +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
> +{
> + if (!buffer || !buffer->access || buffer->access->remove_from)
> + return -EINVAL;
> +
> + return buffer->access->remove_from(buffer, data);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
> +
> void iio_buffer_init(struct iio_buffer *buffer)
> {
> INIT_LIST_HEAD(&buffer->demux_list);
> @@ -807,6 +902,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
> }
>
> if (insert_buffer) {
> + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> + strict_scanmask = true;
> bitmap_or(compound_mask, compound_mask,
> insert_buffer->scan_mask, indio_dev->masklength);
> scan_timestamp |= insert_buffer->scan_timestamp;
> @@ -948,6 +1045,8 @@ static int iio_update_demux(struct iio_dev *indio_dev)
> int ret;
>
> list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
> + if (buffer->direction == IIO_BUFFER_DIRECTION_OUT)
> + continue;
> ret = iio_buffer_update_demux(indio_dev, buffer);
> if (ret < 0)
> goto error_clear_mux_table;
> @@ -1159,6 +1258,11 @@ int iio_update_buffers(struct iio_dev *indio_dev,
> mutex_lock(&iio_dev_opaque->info_exist_lock);
> mutex_lock(&indio_dev->mlock);
>
> + if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> if (insert_buffer && iio_buffer_is_active(insert_buffer))
> insert_buffer = NULL;
>
> @@ -1277,6 +1381,22 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
> return sysfs_emit(buf, "%zu\n", iio_buffer_data_available(buffer));
> }
>
> +static ssize_t direction_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
> +
> + switch (buffer->direction) {
> + case IIO_BUFFER_DIRECTION_IN:
> + return sprintf(buf, "in\n");
> + case IIO_BUFFER_DIRECTION_OUT:
> + return sprintf(buf, "out\n");
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> iio_buffer_write_length);
> static struct device_attribute dev_attr_length_ro = __ATTR(length,
> @@ -1289,12 +1409,20 @@ static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
> S_IRUGO, iio_buffer_show_watermark, NULL);
> static DEVICE_ATTR(data_available, S_IRUGO,
> iio_dma_show_data_available, NULL);
> +static DEVICE_ATTR_RO(direction);
>
> +/**
> + * When adding new attributes here, put the at the end, at least until
> + * the code that handles the lengh/length_ro & watermark/watermark_ro
> + * assignments gets cleaned up. Otherwise these can create some weird
> + * duplicate attributes errors under some setups.
> + */
> static struct attribute *iio_buffer_attrs[] = {
> &dev_attr_length.attr,
> &dev_attr_enable.attr,
> &dev_attr_watermark.attr,
> &dev_attr_data_available.attr,
> + &dev_attr_direction.attr,
> };
>
> #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> @@ -1397,6 +1525,7 @@ static const struct file_operations iio_buffer_chrdev_fileops = {
> .owner = THIS_MODULE,
> .llseek = noop_llseek,
> .read = iio_buffer_read,
> + .write = iio_buffer_write,
> .poll = iio_buffer_poll,
> .release = iio_buffer_chrdev_release,
> };
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2dbb37e09b8c..537a08549a69 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1822,6 +1822,7 @@ static const struct file_operations iio_buffer_fileops = {
> .owner = THIS_MODULE,
> .llseek = noop_llseek,
> .read = iio_buffer_read_outer_addr,
> + .write = iio_buffer_write_outer_addr,
> .poll = iio_buffer_poll_addr,
> .unlocked_ioctl = iio_ioctl,
> .compat_ioctl = compat_ptr_ioctl,
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index b6928ac5c63d..e87b8773253d 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -11,8 +11,15 @@
>
> struct iio_buffer;
>
> +enum iio_buffer_direction {
> + IIO_BUFFER_DIRECTION_IN,
> + IIO_BUFFER_DIRECTION_OUT,
> +};
> +
> int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
>
> +int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data);
> +
> /**
> * iio_push_to_buffers_with_timestamp() - push data and timestamp to buffers
> * @indio_dev: iio_dev structure for device.
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 245b32918ae1..8a44c5321e19 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -7,6 +7,7 @@
> #ifdef CONFIG_IIO_BUFFER
>
> #include <uapi/linux/iio/buffer.h>
> +#include <linux/iio/buffer.h>
>
> struct iio_dev;
> struct iio_buffer;
> @@ -23,6 +24,10 @@ struct iio_buffer;
> * @read: try to get a specified number of bytes (must exist)
> * @data_available: indicates how much data is available for reading from
> * the buffer.
> + * @remove_from: remove sample from buffer. Drivers should calls this to
> + * remove a sample from a buffer.
> + * @write: try to write a number of bytes
> + * @space_available: returns the amount of bytes available in a buffer
> * @request_update: if a parameter change has been marked, update underlying
> * storage.
> * @set_bytes_per_datum:set number of bytes per datum
> @@ -49,6 +54,9 @@ struct iio_buffer_access_funcs {
> int (*store_to)(struct iio_buffer *buffer, const void *data);
> int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
> size_t (*data_available)(struct iio_buffer *buffer);
> + int (*remove_from)(struct iio_buffer *buffer, void *data);
> + int (*write)(struct iio_buffer *buffer, size_t n, const char __user *buf);
> + size_t (*space_available)(struct iio_buffer *buffer);
>
> int (*request_update)(struct iio_buffer *buffer);
>
> @@ -80,6 +88,9 @@ struct iio_buffer {
> /** @bytes_per_datum: Size of individual datum including timestamp. */
> size_t bytes_per_datum;
>
> + /* @direction: Direction of the data stream (in/out). */
> + enum iio_buffer_direction direction;
> +
> /**
> * @access: Buffer access functions associated with the
> * implementation.
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
2021-08-21 0:24 ` kernel test robot
2021-08-23 2:23 ` kernel test robot
@ 2021-08-23 13:50 ` Nuno Sá
2021-08-30 15:42 ` Jonathan Cameron
2021-08-25 8:35 ` Alexandru Ardelean
2021-08-30 16:05 ` Jonathan Cameron
4 siblings, 1 reply; 18+ messages in thread
From: Nuno Sá @ 2021-08-23 13:50 UTC (permalink / raw)
To: Mihail Chindris, linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan,
alexandru.ardelean
On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> Currently IIO only supports buffer mode for capture devices like
> ADCs. Add
> support for buffered mode for output devices like DACs.
>
> The output buffer implementation is analogous to the input buffer
> implementation. Instead of using read() to get data from the buffer
> write()
> is used to copy data into the buffer.
>
> poll() with POLLOUT will wakeup if there is space available for more
> or
> equal to the configured watermark of samples.
>
> Drivers can remove data from a buffer using
> iio_buffer_remove_sample(), the
> function can e.g. called from a trigger handler to write the data to
> hardware.
>
> A buffer can only be either a output buffer or an input, but not
> both. So,
> for a device that has an ADC and DAC path, this will mean 2 IIO
> buffers
> (one for each direction).
>
> The direction of the buffer is decided by the new direction field of
> the
> iio_buffer struct and should be set after allocating and before
> registering
> it.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> ---
> drivers/iio/iio_core.h | 4 +
> drivers/iio/industrialio-buffer.c | 133
> +++++++++++++++++++++++++++++-
> drivers/iio/industrialio-core.c | 1 +
> include/linux/iio/buffer.h | 7 ++
> include/linux/iio/buffer_impl.h | 11 +++
> 5 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 8f4a9b264962..61e318431de9 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> *filp,
> struct poll_table_struct *wait);
> ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> size_t n, loff_t *f_ps);
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> __user *buf,
> + size_t n, loff_t *f_ps);
>
> int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
>
> #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
>
> void iio_disable_all_buffers(struct iio_dev *indio_dev);
> void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> *indio_dev);
>
> #define iio_buffer_poll_addr NULL
> #define iio_buffer_read_outer_addr NULL
> +#define iio_buffer_write_outer_addr NULL
>
> static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> *indio_dev)
> {
> diff --git a/drivers/iio/industrialio-buffer.c
> b/drivers/iio/industrialio-buffer.c
> index a95cc2da56be..73d4451a0572 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> *filp, char __user *buf,
> return ret;
> }
>
> +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> +{
> + if (buf->access->space_available)
> + return buf->access->space_available(buf);
> +
> + return SIZE_MAX;
> +}
> +
> +static ssize_t iio_buffer_write(struct file *filp, const char __user
> *buf,
> + size_t n, loff_t *f_ps)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> + struct iio_dev *indio_dev = ib->indio_dev;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + size_t datum_size;
> + size_t to_wait;
> + int ret;
> +
Even though I do not agree that this is suficient, we should have the
same check as we have for input buffer:
if (!indio_dev->info)
return -ENODEV;
> + if (!rb || !rb->access->write)
> + return -EINVAL;
> +
> + datum_size = rb->bytes_per_datum;
> +
> + /*
> + * If datum_size is 0 there will never be anything to read from
> the
> + * buffer, so signal end of file now.
> + */
> + if (!datum_size)
> + return 0;
> +
> + if (filp->f_flags & O_NONBLOCK)
> + to_wait = 0;
> + else
> + to_wait = min_t(size_t, n / datum_size, rb->watermark);
> +
I had a bit of a crazy thought... Typically, output devices do not
really have a typical trigger as we are used to have in input devices.
Hence, typically we just use a hrtimer (maybe a pwm-trigger can also be
added) trigger where we kind of poll the buffer for new data to send to
the device. So, I was wondering if we could just optionally bypass the
kbuf path in output devices (via some optional buffer option)? At this
point, we pretty much already know that we have data to consume :).
This would be kind of a synchronous interface. One issue I see with
this is that we cannot really have a deterministic (or close) sampling
frequency as we have for example with a pwm based trigger.
Anyways, just me throwing some ideas. This is not the most important
thing for now...
- Nuno Sá
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
@ 2021-08-23 2:23 ` kernel test robot
2021-08-23 2:23 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-23 2:23 UTC (permalink / raw)
To: Mihail Chindris, linux-kernel, linux-iio
Cc: kbuild-all, lars, Michael.Hennerich, jic23, nuno.sa,
dragos.bogdan, alexandru.ardelean, Mihail Chindris
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
config: i386-randconfig-p002-20210821 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/industrialio-buffer.c:1415: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* When adding new attributes here, put the at the end, at least until
vim +1415 drivers/iio/industrialio-buffer.c
1413
1414 /**
> 1415 * When adding new attributes here, put the at the end, at least until
1416 * the code that handles the lengh/length_ro & watermark/watermark_ro
1417 * assignments gets cleaned up. Otherwise these can create some weird
1418 * duplicate attributes errors under some setups.
1419 */
1420 static struct attribute *iio_buffer_attrs[] = {
1421 &dev_attr_length.attr,
1422 &dev_attr_enable.attr,
1423 &dev_attr_watermark.attr,
1424 &dev_attr_data_available.attr,
1425 &dev_attr_direction.attr,
1426 };
1427
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37972 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
@ 2021-08-23 2:23 ` kernel test robot
0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-23 2:23 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
config: i386-randconfig-p002-20210821 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/industrialio-buffer.c:1415: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* When adding new attributes here, put the at the end, at least until
vim +1415 drivers/iio/industrialio-buffer.c
1413
1414 /**
> 1415 * When adding new attributes here, put the at the end, at least until
1416 * the code that handles the lengh/length_ro & watermark/watermark_ro
1417 * assignments gets cleaned up. Otherwise these can create some weird
1418 * duplicate attributes errors under some setups.
1419 */
1420 static struct attribute *iio_buffer_attrs[] = {
1421 &dev_attr_length.attr,
1422 &dev_attr_enable.attr,
1423 &dev_attr_watermark.attr,
1424 &dev_attr_data_available.attr,
1425 &dev_attr_direction.attr,
1426 };
1427
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37972 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
@ 2021-08-21 0:24 ` kernel test robot
2021-08-23 2:23 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-21 0:24 UTC (permalink / raw)
To: Mihail Chindris, linux-kernel, linux-iio
Cc: clang-built-linux, kbuild-all, lars, Michael.Hennerich, jic23,
nuno.sa, dragos.bogdan, alexandru.ardelean, Mihail Chindris
[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
config: x86_64-randconfig-a005-20210821 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d9c5613e856cf2addfbf892fc4c1ce9ef9feceaa)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/industrialio-buffer.c:1415: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* When adding new attributes here, put the at the end, at least until
vim +1415 drivers/iio/industrialio-buffer.c
1413
1414 /**
> 1415 * When adding new attributes here, put the at the end, at least until
1416 * the code that handles the lengh/length_ro & watermark/watermark_ro
1417 * assignments gets cleaned up. Otherwise these can create some weird
1418 * duplicate attributes errors under some setups.
1419 */
1420 static struct attribute *iio_buffer_attrs[] = {
1421 &dev_attr_length.attr,
1422 &dev_attr_enable.attr,
1423 &dev_attr_watermark.attr,
1424 &dev_attr_data_available.attr,
1425 &dev_attr_direction.attr,
1426 };
1427
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37506 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
@ 2021-08-21 0:24 ` kernel test robot
0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-21 0:24 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
config: x86_64-randconfig-a005-20210821 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d9c5613e856cf2addfbf892fc4c1ce9ef9feceaa)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b4f124803ed8bfe5936c756ed4c7aa9124a1468a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
git checkout b4f124803ed8bfe5936c756ed4c7aa9124a1468a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/industrialio-buffer.c:1415: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* When adding new attributes here, put the at the end, at least until
vim +1415 drivers/iio/industrialio-buffer.c
1413
1414 /**
> 1415 * When adding new attributes here, put the at the end, at least until
1416 * the code that handles the lengh/length_ro & watermark/watermark_ro
1417 * assignments gets cleaned up. Otherwise these can create some weird
1418 * duplicate attributes errors under some setups.
1419 */
1420 static struct attribute *iio_buffer_attrs[] = {
1421 &dev_attr_length.attr,
1422 &dev_attr_enable.attr,
1423 &dev_attr_watermark.attr,
1424 &dev_attr_data_available.attr,
1425 &dev_attr_direction.attr,
1426 };
1427
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37506 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/6] iio: Add output buffer support
@ 2021-08-21 0:16 kernel test robot
0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-08-21 0:16 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 5964 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210820165927.4524-2-mihail.chindris@analog.com>
References: <20210820165927.4524-2-mihail.chindris@analog.com>
TO: Mihail Chindris <mihail.chindris@analog.com>
TO: linux-kernel(a)vger.kernel.org
TO: linux-iio(a)vger.kernel.org
CC: lars(a)metafoo.de
CC: Michael.Hennerich(a)analog.com
CC: jic23(a)kernel.org
CC: nuno.sa(a)analog.com
CC: dragos.bogdan(a)analog.com
CC: alexandru.ardelean(a)analog.com
CC: Mihail Chindris <mihail.chindris@analog.com>
Hi Mihail,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 94a853eca720ac9e385e59f27e859b4a01123f58]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/iio-Add-output-buffer-support-and-DAC-example/20210821-010349
base: 94a853eca720ac9e385e59f27e859b4a01123f58
:::::: branch date: 7 hours ago
:::::: commit date: 7 hours ago
config: x86_64-randconfig-m001-20210821 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
New smatch warnings:
drivers/iio/industrialio-buffer.c:221 iio_buffer_write() error: uninitialized symbol 'ret'.
drivers/iio/industrialio-buffer.c:1266 iio_update_buffers() warn: variable dereferenced before check 'insert_buffer' (see line 1261)
Old smatch warnings:
drivers/iio/industrialio-buffer.c:1549 iio_device_buffer_getfd() warn: potential spectre issue 'iio_dev_opaque->attached_buffers' [r] (local cap)
drivers/iio/industrialio-buffer.c:1551 iio_device_buffer_getfd() warn: possible spectre second half. 'buffer'
vim +/ret +221 drivers/iio/industrialio-buffer.c
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 171
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 172 static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 173 size_t n, loff_t *f_ps)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 174 {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 175 struct iio_dev_buffer_pair *ib = filp->private_data;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 176 struct iio_buffer *rb = ib->buffer;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 177 struct iio_dev *indio_dev = ib->indio_dev;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 178 DEFINE_WAIT_FUNC(wait, woken_wake_function);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 179 size_t datum_size;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 180 size_t to_wait;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 181 int ret;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 182
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 183 if (!rb || !rb->access->write)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 184 return -EINVAL;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 185
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 186 datum_size = rb->bytes_per_datum;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 187
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 188 /*
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 189 * If datum_size is 0 there will never be anything to read from the
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 190 * buffer, so signal end of file now.
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 191 */
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 192 if (!datum_size)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 193 return 0;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 194
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 195 if (filp->f_flags & O_NONBLOCK)
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 196 to_wait = 0;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 197 else
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 198 to_wait = min_t(size_t, n / datum_size, rb->watermark);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 199
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 200 add_wait_queue(&rb->pollq, &wait);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 201 do {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 202 if (!indio_dev->info) {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 203 ret = -ENODEV;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 204 break;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 205 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 206
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 207 if (iio_buffer_space_available(rb) < to_wait) {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 208 if (signal_pending(current)) {
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 209 ret = -ERESTARTSYS;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 210 break;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 211 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 212
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 213 wait_woken(&wait, TASK_INTERRUPTIBLE,
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 214 MAX_SCHEDULE_TIMEOUT);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 215 continue;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 216 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 217
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 218 ret = rb->access->write(rb, n, buf);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 219 if (ret == 0 && (filp->f_flags & O_NONBLOCK))
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 220 ret = -EAGAIN;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 @221 } while (ret == 0);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 222 remove_wait_queue(&rb->pollq, &wait);
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 223
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 224 return ret;
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 225 }
b4f124803ed8bf Lars-Peter Clausen 2021-08-20 226
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37881 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/6] iio: Add output buffer support
2021-08-20 16:59 [PATCH v4 0/6] iio: Add output buffer support and DAC example Mihail Chindris
@ 2021-08-20 16:59 ` Mihail Chindris
2021-08-21 0:24 ` kernel test robot
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Mihail Chindris @ 2021-08-20 16:59 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan,
alexandru.ardelean, Mihail Chindris
From: Lars-Peter Clausen <lars@metafoo.de>
Currently IIO only supports buffer mode for capture devices like ADCs. Add
support for buffered mode for output devices like DACs.
The output buffer implementation is analogous to the input buffer
implementation. Instead of using read() to get data from the buffer write()
is used to copy data into the buffer.
poll() with POLLOUT will wakeup if there is space available for more or
equal to the configured watermark of samples.
Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
function can e.g. called from a trigger handler to write the data to
hardware.
A buffer can only be either a output buffer or an input, but not both. So,
for a device that has an ADC and DAC path, this will mean 2 IIO buffers
(one for each direction).
The direction of the buffer is decided by the new direction field of the
iio_buffer struct and should be set after allocating and before registering
it.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
---
drivers/iio/iio_core.h | 4 +
drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++++++++++++++-
drivers/iio/industrialio-core.c | 1 +
include/linux/iio/buffer.h | 7 ++
include/linux/iio/buffer_impl.h | 11 +++
5 files changed, 154 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 8f4a9b264962..61e318431de9 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file *filp,
struct poll_table_struct *wait);
ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
size_t n, loff_t *f_ps);
+ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
+ size_t n, loff_t *f_ps);
int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
#define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
#define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
+#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
void iio_disable_all_buffers(struct iio_dev *indio_dev);
void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
@@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev *indio_dev);
#define iio_buffer_poll_addr NULL
#define iio_buffer_read_outer_addr NULL
+#define iio_buffer_write_outer_addr NULL
static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a95cc2da56be..73d4451a0572 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
return ret;
}
+static size_t iio_buffer_space_available(struct iio_buffer *buf)
+{
+ if (buf->access->space_available)
+ return buf->access->space_available(buf);
+
+ return SIZE_MAX;
+}
+
+static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
+ size_t n, loff_t *f_ps)
+{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+ struct iio_dev *indio_dev = ib->indio_dev;
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
+ size_t datum_size;
+ size_t to_wait;
+ int ret;
+
+ if (!rb || !rb->access->write)
+ return -EINVAL;
+
+ datum_size = rb->bytes_per_datum;
+
+ /*
+ * If datum_size is 0 there will never be anything to read from the
+ * buffer, so signal end of file now.
+ */
+ if (!datum_size)
+ return 0;
+
+ if (filp->f_flags & O_NONBLOCK)
+ to_wait = 0;
+ else
+ to_wait = min_t(size_t, n / datum_size, rb->watermark);
+
+ add_wait_queue(&rb->pollq, &wait);
+ do {
+ if (!indio_dev->info) {
+ ret = -ENODEV;
+ break;
+ }
+
+ if (iio_buffer_space_available(rb) < to_wait) {
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
+
+ wait_woken(&wait, TASK_INTERRUPTIBLE,
+ MAX_SCHEDULE_TIMEOUT);
+ continue;
+ }
+
+ ret = rb->access->write(rb, n, buf);
+ if (ret == 0 && (filp->f_flags & O_NONBLOCK))
+ ret = -EAGAIN;
+ } while (ret == 0);
+ remove_wait_queue(&rb->pollq, &wait);
+
+ return ret;
+}
+
/**
* iio_buffer_poll() - poll the buffer to find out if it has data
* @filp: File structure pointer for device access
@@ -181,8 +244,18 @@ static __poll_t iio_buffer_poll(struct file *filp,
return 0;
poll_wait(filp, &rb->pollq, wait);
- if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
- return EPOLLIN | EPOLLRDNORM;
+
+ switch (rb->direction) {
+ case IIO_BUFFER_DIRECTION_IN:
+ if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+ return EPOLLIN | EPOLLRDNORM;
+ break;
+ case IIO_BUFFER_DIRECTION_OUT:
+ if (iio_buffer_space_available(rb) >= rb->watermark)
+ return EPOLLOUT | EPOLLWRNORM;
+ break;
+ }
+
return 0;
}
@@ -199,6 +272,19 @@ ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
return iio_buffer_read(filp, buf, n, f_ps);
}
+ssize_t iio_buffer_write_wrapper(struct file *filp, const char __user *buf,
+ size_t n, loff_t *f_ps)
+{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+
+ /* check if buffer was opened through new API */
+ if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
+ return -EBUSY;
+
+ return iio_buffer_write(filp, buf, n, f_ps);
+}
+
__poll_t iio_buffer_poll_wrapper(struct file *filp,
struct poll_table_struct *wait)
{
@@ -231,6 +317,15 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
}
}
+int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
+{
+ if (!buffer || !buffer->access || buffer->access->remove_from)
+ return -EINVAL;
+
+ return buffer->access->remove_from(buffer, data);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
+
void iio_buffer_init(struct iio_buffer *buffer)
{
INIT_LIST_HEAD(&buffer->demux_list);
@@ -807,6 +902,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
}
if (insert_buffer) {
+ if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
+ strict_scanmask = true;
bitmap_or(compound_mask, compound_mask,
insert_buffer->scan_mask, indio_dev->masklength);
scan_timestamp |= insert_buffer->scan_timestamp;
@@ -948,6 +1045,8 @@ static int iio_update_demux(struct iio_dev *indio_dev)
int ret;
list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
+ if (buffer->direction == IIO_BUFFER_DIRECTION_OUT)
+ continue;
ret = iio_buffer_update_demux(indio_dev, buffer);
if (ret < 0)
goto error_clear_mux_table;
@@ -1159,6 +1258,11 @@ int iio_update_buffers(struct iio_dev *indio_dev,
mutex_lock(&iio_dev_opaque->info_exist_lock);
mutex_lock(&indio_dev->mlock);
+ if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
if (insert_buffer && iio_buffer_is_active(insert_buffer))
insert_buffer = NULL;
@@ -1277,6 +1381,22 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
return sysfs_emit(buf, "%zu\n", iio_buffer_data_available(buffer));
}
+static ssize_t direction_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
+
+ switch (buffer->direction) {
+ case IIO_BUFFER_DIRECTION_IN:
+ return sprintf(buf, "in\n");
+ case IIO_BUFFER_DIRECTION_OUT:
+ return sprintf(buf, "out\n");
+ default:
+ return -EINVAL;
+ }
+}
+
static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
iio_buffer_write_length);
static struct device_attribute dev_attr_length_ro = __ATTR(length,
@@ -1289,12 +1409,20 @@ static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
S_IRUGO, iio_buffer_show_watermark, NULL);
static DEVICE_ATTR(data_available, S_IRUGO,
iio_dma_show_data_available, NULL);
+static DEVICE_ATTR_RO(direction);
+/**
+ * When adding new attributes here, put the at the end, at least until
+ * the code that handles the lengh/length_ro & watermark/watermark_ro
+ * assignments gets cleaned up. Otherwise these can create some weird
+ * duplicate attributes errors under some setups.
+ */
static struct attribute *iio_buffer_attrs[] = {
&dev_attr_length.attr,
&dev_attr_enable.attr,
&dev_attr_watermark.attr,
&dev_attr_data_available.attr,
+ &dev_attr_direction.attr,
};
#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
@@ -1397,6 +1525,7 @@ static const struct file_operations iio_buffer_chrdev_fileops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
.read = iio_buffer_read,
+ .write = iio_buffer_write,
.poll = iio_buffer_poll,
.release = iio_buffer_chrdev_release,
};
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2dbb37e09b8c..537a08549a69 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1822,6 +1822,7 @@ static const struct file_operations iio_buffer_fileops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
.read = iio_buffer_read_outer_addr,
+ .write = iio_buffer_write_outer_addr,
.poll = iio_buffer_poll_addr,
.unlocked_ioctl = iio_ioctl,
.compat_ioctl = compat_ptr_ioctl,
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index b6928ac5c63d..e87b8773253d 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,8 +11,15 @@
struct iio_buffer;
+enum iio_buffer_direction {
+ IIO_BUFFER_DIRECTION_IN,
+ IIO_BUFFER_DIRECTION_OUT,
+};
+
int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
+int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data);
+
/**
* iio_push_to_buffers_with_timestamp() - push data and timestamp to buffers
* @indio_dev: iio_dev structure for device.
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 245b32918ae1..8a44c5321e19 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -7,6 +7,7 @@
#ifdef CONFIG_IIO_BUFFER
#include <uapi/linux/iio/buffer.h>
+#include <linux/iio/buffer.h>
struct iio_dev;
struct iio_buffer;
@@ -23,6 +24,10 @@ struct iio_buffer;
* @read: try to get a specified number of bytes (must exist)
* @data_available: indicates how much data is available for reading from
* the buffer.
+ * @remove_from: remove sample from buffer. Drivers should calls this to
+ * remove a sample from a buffer.
+ * @write: try to write a number of bytes
+ * @space_available: returns the amount of bytes available in a buffer
* @request_update: if a parameter change has been marked, update underlying
* storage.
* @set_bytes_per_datum:set number of bytes per datum
@@ -49,6 +54,9 @@ struct iio_buffer_access_funcs {
int (*store_to)(struct iio_buffer *buffer, const void *data);
int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
size_t (*data_available)(struct iio_buffer *buffer);
+ int (*remove_from)(struct iio_buffer *buffer, void *data);
+ int (*write)(struct iio_buffer *buffer, size_t n, const char __user *buf);
+ size_t (*space_available)(struct iio_buffer *buffer);
int (*request_update)(struct iio_buffer *buffer);
@@ -80,6 +88,9 @@ struct iio_buffer {
/** @bytes_per_datum: Size of individual datum including timestamp. */
size_t bytes_per_datum;
+ /* @direction: Direction of the data stream (in/out). */
+ enum iio_buffer_direction direction;
+
/**
* @access: Buffer access functions associated with the
* implementation.
--
2.27.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-09-16 10:58 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-22 15:49 [PATCH v4 1/6] iio: Add output buffer support kernel test robot
2021-08-25 15:31 ` kernel test robot
2021-08-25 15:31 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-08-21 0:16 kernel test robot
2021-08-20 16:59 [PATCH v4 0/6] iio: Add output buffer support and DAC example Mihail Chindris
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
2021-08-21 0:24 ` kernel test robot
2021-08-21 0:24 ` kernel test robot
2021-08-23 2:23 ` kernel test robot
2021-08-23 2:23 ` kernel test robot
2021-08-23 13:50 ` Nuno Sá
2021-08-30 15:42 ` Jonathan Cameron
2021-09-01 8:50 ` Sa, Nuno
2021-09-05 9:54 ` Jonathan Cameron
2021-08-25 8:35 ` Alexandru Ardelean
2021-08-30 16:05 ` Jonathan Cameron
2021-09-01 8:54 ` Sa, Nuno
2021-09-05 9:55 ` Jonathan Cameron
2021-09-16 10:57 ` Chindris, Mihail
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.