| 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: <2a67cb8ee77cb96bc899d82348cb0eb0bf44925c.camel@gmail.com>
Date: Mon, 23 Aug 2021 15:50:14 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Mihail Chindris <mihail.chindris@...log.com>,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
nuno.sa@...log.com, dragos.bogdan@...log.com,
alexandru.ardelean@...log.com
Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> From: Lars-Peter Clausen <lars@...afoo.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@...afoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> Signed-off-by: Mihail Chindris <mihail.chindris@...log.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á
>
Powered by blists - more mailing lists