| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+U=Dspbu-CY9HM5ZhmXXzJP4HpQaXdN1YA1Mv+cyL3g3gBw-Q@mail.gmail.com>
Date: Mon, 15 Feb 2021 13:39:28 +0200
From: Alexandru Ardelean <ardeleanalex@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alexandru Ardelean <alexandru.ardelean@...log.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
"Hennerich, Michael" <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
"Bogdan, Dragos" <dragos.bogdan@...log.com>
Subject: Re: [PATCH v2 3/3] tools: iio: add example for high-speed buffer support
On Sun, Feb 14, 2021 at 5:58 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Fri, 12 Feb 2021 12:11:43 +0200
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
>
> > Following a recent update to the IIO buffer infrastructure, this change
> > adds a basic example on how to access an IIO buffer via the new mmap()
> > interface.
> >
> > The ioctl() for the high-speed mode needs to be enabled right from the
> > start, before setting any parameters via sysfs (length, enable, etc), to
> > make sure that the mmap mode is used and not the fileio mode.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
>
> Just one small question on error handling. Otherwise this looks fine to me.
>
> thanks,
>
> Jonathan
>
> > ---
> > tools/iio/iio_generic_buffer.c | 183 +++++++++++++++++++++++++++++++--
> > 1 file changed, 177 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> > index fdd08514d556..675a7e6047e0 100644
> > --- a/tools/iio/iio_generic_buffer.c
> > +++ b/tools/iio/iio_generic_buffer.c
> > @@ -31,6 +31,7 @@
> > #include <stdbool.h>
> > #include <signal.h>
> > #include <sys/ioctl.h>
> > +#include <sys/mman.h>
> > #include <linux/iio/buffer.h>
> > #include "iio_utils.h"
> >
> > @@ -239,6 +240,132 @@ static int enable_disable_all_channels(char *dev_dir_name, int buffer_idx, int e
> > return 0;
> > }
> >
> > +struct mmap_block {
> > + struct iio_buffer_block block;
> > + void *addr;
> > +};
> > +
> > +static struct mmap_block *enable_high_speed(int buf_fd, unsigned int block_size,
> > + int nblocks)
> > +{
> > + struct iio_buffer_block_alloc_req req = { 0 };
> > + struct mmap_block *mmaps = NULL;
> > + int mmaps_cnt = 0;
> > + int i, ret;
> > +
> > + /**
> > + * Validate we can do high-speed by issuing BLOCK_FREE ioctl.
> > + * If using just BLOCK_ALLOC it's distinguish between ENOSYS
> > + * and other error types.
> > + */
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_FREE_IOCTL, 0);
> > + if (ret < 0) {
> > + errno = ENOSYS;
> > + return NULL;
> > + }
> > +
> > + /* for now, this */
> > + req.id = 0;
> > + req.type = 0;
> > + req.size = block_size;
> > + req.count = nblocks;
> > +
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_ALLOC_IOCTL, &req);
> > + if (ret < 0)
> > + return NULL;
> > +
> > + if (req.count == 0) {
> > + errno = ENOMEM;
> > + return NULL;
> > + }
> > +
> > + if (req.count < nblocks) {
> > + fprintf(stderr, "Requested %d blocks, got %d\n",
> > + nblocks, req.count);
> > + errno = ENOMEM;
> > + return NULL;
> > + }
> > +
> > + mmaps = calloc(req.count, sizeof(*mmaps));
> > + if (!mmaps) {
> > + errno = ENOMEM;
> > + return NULL;
> > + }
> > +
> > + for (i = 0; i < req.count; i++) {
> > + mmaps[i].block.id = i;
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_QUERY_IOCTL, &mmaps[i].block);
> > + if (ret < 0)
> > + goto error;
> > +
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_ENQUEUE_IOCTL, &mmaps[i].block);
> > + if (ret < 0)
> > + goto error;
>
> > +
> > + mmaps[i].addr = mmap(0, mmaps[i].block.size,
> > + PROT_READ | PROT_WRITE, MAP_SHARED,
> > + buf_fd, mmaps[i].block.data.offset);
> > +
> > + if (mmaps[i].addr == MAP_FAILED)
> > + goto error;
> > +
> > + mmaps_cnt++;
> > + }
> > +
> > + return mmaps;
> > +
> > +error:
> > + for (i = 0; i < mmaps_cnt; i++)
> > + munmap(mmaps[i].addr, mmaps[i].block.size);
> > + free(mmaps);
>
> No free of the blocks? We have unmapped them but I'd imagine we'd also
> need to free them from the driver side.
Good catch.
>
> > + return NULL;
> > +}
> > +
> > +static int read_high_speed(int buf_fd, char *data, unsigned int block_size,
> > + struct mmap_block *mmaps, unsigned int mmaps_cnt)
> > +{
> > + struct iio_buffer_block block;
> > + int ret;
> > +
> > + /**
> > + * This is where some buffer-pool management can do wonders,
> > + * but for the sake of this sample-code, we're just going to
> > + * copy the data and re-enqueue it back
> > + */
> > + memset(&block, 0, sizeof(block));
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_DEQUEUE_IOCTL, &block);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* check for weird conditions */
> > + if (block.bytes_used > block_size) {
> > + fprintf(stderr,
> > + "Got a bigger block (%u) than expected (%u)\n",
> > + block.bytes_used, block_size);
> > + return -EFBIG;
> > + }
> > +
> > + if (block.bytes_used < block_size) {
> > + /**
> > + * This can be normal, with some real-world data
> > + * terminating abruptly. But log it.
> > + */
> > + fprintf(stderr,
> > + "Got a smaller block (%u) than expected (%u)\n",
> > + block.bytes_used, block_size);
> > + }
> > +
> > + /* memcpy() the data, we lose some more performance here :p */
> > + memcpy(data, mmaps[block.id].addr, block.bytes_used);
> > +
> > + /* and re-queue this back */
> > + ret = ioctl(buf_fd, IIO_BUFFER_BLOCK_ENQUEUE_IOCTL, &mmaps[block.id].block);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return block.bytes_used;
> > +}
> > +
> > static void print_usage(void)
> > {
> > fprintf(stderr, "Usage: generic_buffer [options]...\n"
> > @@ -249,6 +376,7 @@ static void print_usage(void)
> > " -c <n> Do n conversions, or loop forever if n < 0\n"
> > " -e Disable wait for event (new data)\n"
> > " -g Use trigger-less mode\n"
> > + " -h Use high-speed buffer access\n"
> > " -l <n> Set buffer length to n samples\n"
> > " --device-name -n <name>\n"
> > " --device-num -N <num>\n"
> > @@ -356,9 +484,15 @@ int main(int argc, char **argv)
> >
> > struct iio_channel_info *channels = NULL;
> >
> > + static bool use_high_speed = false;
> > + unsigned int block_size;
> > + int nblocks = 16; /* default */
> > + int mmaps_cnt = 0;
> > + struct mmap_block *mmaps = NULL;
> > +
> > register_cleanup();
> >
> > - while ((c = getopt_long(argc, argv, "aAb:c:egl:n:N:t:T:w:?", longopts,
> > + while ((c = getopt_long(argc, argv, "aAb:c:eghl:n:N:t:T:w:?", longopts,
> > NULL)) != -1) {
> > switch (c) {
> > case 'a':
> > @@ -396,6 +530,9 @@ int main(int argc, char **argv)
> > case 'g':
> > notrigger = 1;
> > break;
> > + case 'h':
> > + use_high_speed = true;
> > + break;
> > case 'l':
> > errno = 0;
> > buf_len = strtoul(optarg, &dummy, 10);
> > @@ -661,6 +798,29 @@ int main(int argc, char **argv)
> > goto error;
> > }
> >
> > + scan_size = size_from_channelarray(channels, num_channels);
> > + block_size = scan_size * buf_len;
> > + /**
> > + * Need to enable high-speed before configuring length/enable.
> > + * Otherwise, the DMA buffer will work in fileio mode,
> > + * and mmap won't work.
> > + */
> > + if (use_high_speed) {
> > + /**
> > + * The block_size for one block is the same as 'data', but it
> > + * doesn't need to be the same size. It is easier for the sake
> > + * of this example.
> > + */
> > + mmaps = enable_high_speed(buf_fd, block_size, nblocks);
> > + if (!mmaps) {
> > + fprintf(stderr, "Could not enable high-speed mode\n");
> > + ret = -errno;
> > + goto error;
> > + }
> > + mmaps_cnt = nblocks;
> > + printf("Using high-speed mode\n");
> > + }
> > +
> > /* Setup ring buffer parameters */
> > ret = write_sysfs_int("length", buf_dir_name, buf_len);
> > if (ret < 0)
> > @@ -675,8 +835,7 @@ int main(int argc, char **argv)
> > goto error;
> > }
> >
> > - scan_size = size_from_channelarray(channels, num_channels);
> > - data = malloc(scan_size * buf_len);
> > + data = malloc(block_size);
> > if (!data) {
> > ret = -ENOMEM;
> > goto error;
> > @@ -719,7 +878,13 @@ int main(int argc, char **argv)
> > toread = 64;
> > }
> >
> > - read_size = read(buf_fd, data, toread * scan_size);
> > + if (use_high_speed) {
> > + read_size = read_high_speed(buf_fd, data, block_size,
> > + mmaps, mmaps_cnt);
> > + } else {
> > + read_size = read(buf_fd, data, toread * scan_size);
> > + }
> > +
> > if (read_size < 0) {
> > if (errno == EAGAIN) {
> > fprintf(stderr, "nothing available\n");
> > @@ -738,8 +903,14 @@ int main(int argc, char **argv)
> >
> > if (fd >= 0 && close(fd) == -1)
> > perror("Failed to close character device");
> > - if (buf_fd >= 0 && close(buf_fd) == -1)
> > - perror("Failed to close buffer");
> > + for (i = 0; i < mmaps_cnt; i++)
> > + munmap(mmaps[i].addr, mmaps[i].block.size);
> > + free(mmaps);
> > + if (buf_fd >= 0) {
> > + ioctl(buf_fd, IIO_BUFFER_BLOCK_FREE_IOCTL, 0);
> > + if (close(buf_fd) == -1)
> > + perror("Failed to close buffer");
> > + }
> > free(buffer_access);
> > free(data);
> > free(buf_dir_name);
>
Powered by blists - more mailing lists