Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure

[Paul Cercueil <paul@crapouillou.net>]

Hi Jonathan,

Le jeudi 21 décembre 2023 à 12:06 +0000, Jonathan Cameron a écrit :
> On Tue, 19 Dec 2023 18:50:06 +0100
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
> > Add the necessary infrastructure to the IIO core to support a new
> > optional DMABUF based interface.
> > 
> > With this new interface, DMABUF objects (externally created) can be
> > attached to a IIO buffer, and subsequently used for data transfer.
> > 
> > A userspace application can then use this interface to share DMABUF
> > objects between several interfaces, allowing it to transfer data in
> > a
> > zero-copy fashion, for instance between IIO and the USB stack.
> > 
> > The userspace application can also memory-map the DMABUF objects,
> > and
> > access the sample data directly. The advantage of doing this vs.
> > the
> > read() interface is that it avoids an extra copy of the data
> > between the
> > kernel and userspace. This is particularly userful for high-speed
> > devices which produce several megabytes or even gigabytes of data
> > per
> > second.
> > 
> > As part of the interface, 3 new IOCTLs have been added:
> > 
> > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
> >  Attach the DMABUF object identified by the given file descriptor
> > to the
> >  buffer.
> > 
> > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
> >  Detach the DMABUF object identified by the given file descriptor
> > from
> >  the buffer. Note that closing the IIO buffer's file descriptor
> > will
> >  automatically detach all previously attached DMABUF objects.
> > 
> > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
> >  Request a data transfer to/from the given DMABUF object. Its file
> >  descriptor, as well as the transfer size and flags are provided in
> > the
> >  "iio_dmabuf" structure.
> > 
> > These three IOCTLs have to be performed on the IIO buffer's file
> > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
> > 
> 
> Fair enough - so they don't apply to the 'legacy' buffer which
> simplifies
> things but in one place you assume that logic is used (given error
> return
> values).
> 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > 
> This is big and complex and I'm out of time for now, so I've made
> some
> comments but should revisit it.
> I'm also looking for review from those more familiar with dmabuf side
> of things than I am!
> 
> Jonathan
> 
> 
> >  
> > +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool
> > nonblock)
> > +{
> > +	int ret;
> > +
> > +	ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> > +	if (ret) {
> > +		if (ret != -EDEADLK)
> > +			goto out;
> > +		if (nonblock) {
> > +			ret = -EBUSY;
> > +			goto out;
> > +		}
> > +
> > +		ret = dma_resv_lock_slow_interruptible(dmabuf-
> > >resv, NULL);
> > +	}
> > +
> > +out:
> > +	return ret;
> 
> I'm not a fan gotos that do nothing.  Just return in appropriate
> places above.
> 
> > +}
> > 
> > +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair
> > *ib, int *user_req)
> > +{
> > +	struct dma_buf_attachment *attach;
> > +	struct iio_dmabuf_priv *priv;
> > +	struct dma_buf *dmabuf;
> > +	int dmabuf_fd, ret = 0;
> > +
> > +	if (copy_from_user(&dmabuf_fd, user_req,
> > sizeof(dmabuf_fd)))
> > +		return -EFAULT;
> > +
> > +	dmabuf = dma_buf_get(dmabuf_fd);
> > +	if (IS_ERR(dmabuf))
> > +		return PTR_ERR(dmabuf);
> > +
> > +	attach = iio_buffer_find_attachment(ib->indio_dev,
> > dmabuf);
> > +	if (IS_ERR(attach)) {
> > +		ret = PTR_ERR(attach);
> > +		goto out_dmabuf_put;
> > +	}
> > +
> > +	priv = attach->importer_priv;
> > +	list_del_init(&priv->entry);
> > +
> > +	/*
> > +	 * Unref twice to release the reference obtained with
> > +	 * iio_buffer_find_attachment() above, and the one
> > obtained in
> > +	 * iio_buffer_attach_dmabuf().
> > +	 */
> > +	iio_buffer_dmabuf_put(attach);
> > +	iio_buffer_dmabuf_put(attach);
> > +
> > +out_dmabuf_put:
> > +	dma_buf_put(dmabuf);
> As below. Feels like a __free(dma_buf_put) bit of magic would be a
> nice to have.

Whoa, never heard about this. That looks great!

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *
> > +iio_buffer_dma_fence_get_driver_name(struct dma_fence *fence)
> > +{
> > +	return "iio";
> > +}
> > +
> > +static void iio_buffer_dma_fence_release(struct dma_fence *fence)
> > +{
> > +	struct iio_dma_fence *iio_fence =
> > +		container_of(fence, struct iio_dma_fence, base);
> > +
> > +	kfree(iio_fence);
> > +}
> > +
> > +static const struct dma_fence_ops iio_buffer_dma_fence_ops = {
> > +	.get_driver_name	=
> > iio_buffer_dma_fence_get_driver_name,
> > +	.get_timeline_name	=
> > iio_buffer_dma_fence_get_driver_name,
> > +	.release		= iio_buffer_dma_fence_release,
> > +};
> > +
> > +static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair
> > *ib,
> > +				     struct iio_dmabuf __user
> > *iio_dmabuf_req,
> > +				     bool nonblock)
> > +{
> > +	struct iio_dev *indio_dev = ib->indio_dev;
> > +	struct iio_buffer *buffer = ib->buffer;
> > +	struct iio_dmabuf iio_dmabuf;
> > +	struct dma_buf_attachment *attach;
> > +	struct iio_dmabuf_priv *priv;
> > +	enum dma_data_direction dir;
> > +	struct iio_dma_fence *fence;
> > +	struct dma_buf *dmabuf;
> > +	struct sg_table *sgt;
> > +	unsigned long timeout;
> > +	bool dma_to_ram;
> > +	bool cyclic;
> > +	int ret;
> > +
> > +	if (copy_from_user(&iio_dmabuf, iio_dmabuf_req,
> > sizeof(iio_dmabuf)))
> > +		return -EFAULT;
> > +
> > +	if (iio_dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS)
> > +		return -EINVAL;
> > +
> > +	cyclic = iio_dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC;
> > +
> > +	/* Cyclic flag is only supported on output buffers */
> > +	if (cyclic && buffer->direction !=
> > IIO_BUFFER_DIRECTION_OUT)
> > +		return -EINVAL;
> > +
> > +	dmabuf = dma_buf_get(iio_dmabuf.fd);
> > +	if (IS_ERR(dmabuf))
> > +		return PTR_ERR(dmabuf);
> > +
> > +	if (!iio_dmabuf.bytes_used || iio_dmabuf.bytes_used >
> > dmabuf->size) {
> > +		ret = -EINVAL;
> > +		goto err_dmabuf_put;
> > +	}
> > +
> > +	attach = iio_buffer_find_attachment(indio_dev, dmabuf);
> > +	if (IS_ERR(attach)) {
> > +		ret = PTR_ERR(attach);
> > +		goto err_dmabuf_put;
> 
> Might be worth some cleanup.h magic given this put happens in all
> exit paths.
> 
> > +	}
> > +
> > +	priv = attach->importer_priv;
> > +
> > +	dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN;
> > +	dir = dma_to_ram ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> > +
> > +	sgt = dma_buf_map_attachment(attach, dir);
> > +	if (IS_ERR(sgt)) {
> > +		ret = PTR_ERR(sgt);
> > +		dev_err(&indio_dev->dev, "Unable to map
> > attachment: %d\n", ret);
> > +		goto err_attachment_put;
> > +	}
> > +
> > +	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> > +	if (!fence) {
> > +		ret = -ENOMEM;
> > +		goto err_unmap_attachment;
> > +	}
> > +
> > +	fence->priv = priv;
> > +	fence->sgt = sgt;
> > +	fence->dir = dir;
> > +	priv->fence = fence;
> > +
> > +	dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops,
> > +		       &priv->lock, priv->context, 0);
> > +
> > +	ret = iio_dma_resv_lock(dmabuf, nonblock);
> > +	if (ret)
> > +		goto err_fence_put;
> > +
> > +	timeout = nonblock ? 0 :
> > msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS);
> > +
> > +	/* Make sure we don't have writers */
> > +	ret = (int) dma_resv_wait_timeout(dmabuf->resv,
> > DMA_RESV_USAGE_WRITE,
> > +					  true, timeout);
> 
> I'd handle this and similar cases as long rather than adding the odd
> looking cast and making
> me think too much about it.
> 
> > +	if (ret == 0)
> > +		ret = -EBUSY;
> > +	if (ret < 0)
> > +		goto err_resv_unlock;
> > +
> > +	if (dma_to_ram) {
> > +		/*
> > +		 * If we're writing to the DMABUF, make sure we
> > don't have
> > +		 * readers
> > +		 */
> > +		ret = (int) dma_resv_wait_timeout(dmabuf->resv,
> > +						 
> > DMA_RESV_USAGE_READ, true,
> > +						  timeout);
> > +		if (ret == 0)
> > +			ret = -EBUSY;
> > +		if (ret < 0)
> > +			goto err_resv_unlock;
> > +	}
> > +
> > +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > +	if (ret)
> > +		goto err_resv_unlock;
> > +
> > +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> > +			   dma_resv_usage_rw(dma_to_ram));
> > +	dma_resv_unlock(dmabuf->resv);
> > +
> > +	ret = buffer->access->enqueue_dmabuf(buffer, priv->block,
> > sgt,
> > +					    
> > iio_dmabuf.bytes_used, cyclic);
> > +	if (ret)
> > +		iio_buffer_signal_dmabuf_done(attach, ret);
> 
> I'd like a comment on why using the 'successful' path cleanup makes
> sense in this
> error case.  It's possible to figure it out, but reviewers are lazy
> and generally
> we like the cleanup to be obvious and local on error paths.
> 
> > +
> > +	dma_buf_put(dmabuf);
> > +
> > +	return ret;
> > +
> > +err_resv_unlock:
> > +	dma_resv_unlock(dmabuf->resv);
> > +err_fence_put:
> > +	dma_fence_put(&fence->base);
> > +err_unmap_attachment:
> > +	dma_buf_unmap_attachment(attach, sgt, dir);
> > +err_attachment_put:
> > +	iio_buffer_dmabuf_put(attach);
> > +err_dmabuf_put:
> > +	dma_buf_put(dmabuf);
> > +
> > +	return ret;
> > +}
> > +
> > +void iio_buffer_signal_dmabuf_done(struct dma_buf_attachment
> > *attach, int ret)
> > +{
> > +	struct iio_dmabuf_priv *priv = attach->importer_priv;
> > +	struct iio_dma_fence *fence = priv->fence;
> > +	enum dma_data_direction dir = fence->dir;
> > +	struct sg_table *sgt = fence->sgt;
> > +
> > +	dma_fence_get(&fence->base);
> 
> I don't know much about dma_fence, but is it valid to access
> contents of it (sgt, etc) before getting a reference?
> Ultimately dma_fence_put() can result in a kfree_rcu() so seems
> unlikely to be safe and definitely fails the 'obviously correct'
> test.  Given those are I assume trivial accesses just do them 
> down here perhaps?

It is valid to access the fence before getting a reference - the fence
won't be freed before it is signaled down below. It would be illegal to
access it after the dma_fence_put() though, which I'm not doing here.

> 
> 
> > +	fence->base.error = ret;
> > +	dma_fence_signal(&fence->base);
> > +	dma_fence_put(&fence->base);
> > +
> > +	dma_buf_unmap_attachment(attach, sgt, dir);
> > +	iio_buffer_dmabuf_put(attach);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done);
> > +
> 
> > +static long iio_buffer_chrdev_ioctl(struct file *filp,
> > +				    unsigned int cmd, unsigned
> > long arg)
> > +{
> > +	struct iio_dev_buffer_pair *ib = filp->private_data;
> > +	void __user *_arg = (void __user *)arg;
> > +
> > +	switch (cmd) {
> > +	case IIO_BUFFER_DMABUF_ATTACH_IOCTL:
> > +		return iio_buffer_attach_dmabuf(ib, _arg);
> > +	case IIO_BUFFER_DMABUF_DETACH_IOCTL:
> > +		return iio_buffer_detach_dmabuf(ib, _arg);
> > +	case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:
> > +		return iio_buffer_enqueue_dmabuf(ib, _arg,
> > +						 filp->f_flags &
> > O_NONBLOCK);
> > +	default:
> > +		return IIO_IOCTL_UNHANDLED;
> 
> Given you aren't using the ioctl handling on the legacy buffer, I
> think this
> should simply return an error code, not the magic value we use to
> indicate
> 'all fine, but it's not mine'.
> Probably -EINVAL or similar.  Note that the wrapper around the legacy
> buffer
> ioctls translates this to -ENODEV; rather than returning from the
> ioctl.

ACK for this and the other comments.

Thanks for reviewing!

Cheers,
-Paul

> > +	}
> > +}
> > +
> >  static const struct file_operations iio_buffer_chrdev_fileops = {
> >  	.owner = THIS_MODULE,
> >  	.llseek = noop_llseek,
> >  	.read = iio_buffer_read,
> >  	.write = iio_buffer_write,
> > +	.unlocked_ioctl = iio_buffer_chrdev_ioctl,
> > +	.compat_ioctl = compat_ptr_ioctl,
> >  	.poll = iio_buffer_poll,
> >  	.release = iio_buffer_chrdev_release,
> >  };
>