[PATCH 0/2] usb: gadget: functionfs: DMABUF import interface

[PATCH 0/2] usb: gadget: functionfs: DMABUF import interface

[Paul Cercueil]

Hi,

This small patchset adds three new IOCTLs that can be used to attach,
detach, or transfer from/to a DMABUF object.

This was surprisingly easy to add, as the functionfs code only uses
scatterlists for transfers and allows specifying the number of bytes to
transfer. The bulk of the code is then for general DMABUF accounting.

The patchset isn't tagged RFC but comments are very welcome, there are
some things I am not 100% sure about: ffs_dma_resv_lock (with no
ww_acquire_ctx), and I'm using pr_debug which feels wrong. Also, I
should probably add documentation? The current IOCTLs for functionfs
were not documented, as far as I can tell.

We use it with DMABUFs created with udmabuf, that we attach to the
functionfs interface and to IIO devices (with a DMABUF interface for
IIO, on its way to upstream too), to transfer samples from high-speed
transceivers to USB in a zero-copy fashion.

Cheers,
-Paul


Paul Cercueil (2):
  usb: gadget: Support already-mapped DMA SGs
  usb: gadget: functionfs: Add DMABUF import interface

 drivers/usb/gadget/function/f_fs.c  | 398 ++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c       |   7 +-
 include/linux/usb/gadget.h          |   2 +
 include/uapi/linux/usb/functionfs.h |  14 +-
 4 files changed, 419 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] usb: gadget: Support already-mapped DMA SGs

[Paul Cercueil]

Add a new 'sg_was_mapped' field to the struct usb_request. This field
can be used to indicate that the scatterlist associated to the USB
transfer has already been mapped into the DMA space, and it does not
have to be done internally.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/usb/gadget/udc/core.c | 7 ++++++-
 include/linux/usb/gadget.h    | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 23b0629a8774..5f2c4933769d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -829,6 +829,11 @@ int usb_gadget_map_request_by_dev(struct device *dev,
 	if (req->length == 0)
 		return 0;
 
+	if (req->sg_was_mapped) {
+		req->num_mapped_sgs = req->num_sgs;
+		return 0;
+	}
+
 	if (req->num_sgs) {
 		int     mapped;
 
@@ -874,7 +879,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_map_request);
 void usb_gadget_unmap_request_by_dev(struct device *dev,
 		struct usb_request *req, int is_in)
 {
-	if (req->length == 0)
+	if (req->length == 0 || req->sg_was_mapped)
 		return;
 
 	if (req->num_mapped_sgs) {
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 00750f7020f3..9dd829b8974a 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -52,6 +52,7 @@ struct usb_ep;
  * @short_not_ok: When reading data, makes short packets be
  *     treated as errors (queue stops advancing till cleanup).
  * @dma_mapped: Indicates if request has been mapped to DMA (internal)
+ * @sg_was_mapped: Set if the scatterlist has been mapped before the request
  * @complete: Function called when request completes, so this request and
  *	its buffer may be re-used.  The function will always be called with
  *	interrupts disabled, and it must not sleep.
@@ -111,6 +112,7 @@ struct usb_request {
 	unsigned		zero:1;
 	unsigned		short_not_ok:1;
 	unsigned		dma_mapped:1;
+	unsigned		sg_was_mapped:1;
 
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);
-- 
2.39.2

[PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[Paul Cercueil]

This patch introduces three new ioctls. They all should be called on a
data endpoint (ie. not ep0). They are:

- FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a DMABUF
  object to attach to the endpoint.

- FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
  DMABUF to detach from the endpoint. Note that closing the endpoint's
  file descriptor will automatically detach all attached DMABUFs.

- FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from / to
  the given DMABUF. Its argument is a structure that packs the DMABUF's
  file descriptor, the size in bytes to transfer (which should generally
  be set to the size of the DMABUF), and a 'flags' field which is unused
  for now.
  Before this ioctl can be used, the related DMABUF must be attached
  with FUNCTIONFS_DMABUF_ATTACH.

These three ioctls enable the FunctionFS code to transfer data between
the USB stack and a DMABUF object, which can be provided by a driver
from a completely different subsystem, in a zero-copy fashion.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/usb/gadget/function/f_fs.c  | 398 ++++++++++++++++++++++++++++
 include/uapi/linux/usb/functionfs.h |  14 +-
 2 files changed, 411 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ddfc537c7526..365fb716f0ad 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -15,6 +15,9 @@
 /* #define VERBOSE_DEBUG */
 
 #include <linux/blkdev.h>
+#include <linux/dma-buf.h>
+#include <linux/dma-fence.h>
+#include <linux/dma-resv.h>
 #include <linux/pagemap.h>
 #include <linux/export.h>
 #include <linux/fs_parser.h>
@@ -124,6 +127,26 @@ struct ffs_ep {
 	u8				num;
 };
 
+struct ffs_dmabuf_priv {
+	struct list_head entry;
+	struct kref ref;
+	struct dma_buf_attachment *attach;
+	spinlock_t lock;
+	u64 context;
+};
+
+struct ffs_dma_fence {
+	struct dma_fence base;
+	struct ffs_dmabuf_priv *priv;
+	struct sg_table *sgt;
+	enum dma_data_direction dir;
+};
+
+static inline struct ffs_dma_fence *to_ffs_dma_fence(struct dma_fence *fence)
+{
+	return container_of(fence, struct ffs_dma_fence, base);
+}
+
 struct ffs_epfile {
 	/* Protects ep->ep and ep->req. */
 	struct mutex			mutex;
@@ -197,6 +220,8 @@ struct ffs_epfile {
 	unsigned char			isoc;	/* P: ffs->eps_lock */
 
 	unsigned char			_pad;
+
+	struct list_head		dmabufs;
 };
 
 struct ffs_buffer {
@@ -1279,19 +1304,354 @@ static ssize_t ffs_epfile_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 	return res;
 }
 
+static void ffs_dmabuf_release(struct kref *ref)
+{
+	struct ffs_dmabuf_priv *priv = container_of(ref, struct ffs_dmabuf_priv, ref);
+	struct dma_buf_attachment *attach = priv->attach;
+	struct dma_buf *dmabuf = attach->dmabuf;
+
+	pr_debug("FFS DMABUF release\n");
+	dma_buf_detach(attach->dmabuf, attach);
+	dma_buf_put(dmabuf);
+
+	list_del(&priv->entry);
+	kfree(priv);
+}
+
+static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
+{
+	struct ffs_dmabuf_priv *priv = attach->importer_priv;
+
+	kref_get(&priv->ref);
+}
+
+static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
+{
+	struct ffs_dmabuf_priv *priv = attach->importer_priv;
+
+	kref_put(&priv->ref, ffs_dmabuf_release);
+}
+
 static int
 ffs_epfile_release(struct inode *inode, struct file *file)
 {
 	struct ffs_epfile *epfile = inode->i_private;
+	struct ffs_dmabuf_priv *priv, *tmp;
 
 	ENTER();
 
+	/* Close all attached DMABUFs */
+	list_for_each_entry_safe(priv, tmp, &epfile->dmabufs, entry) {
+		ffs_dmabuf_put(priv->attach);
+	}
+
 	__ffs_epfile_read_buffer_free(epfile);
 	ffs_data_closed(epfile->ffs);
 
 	return 0;
 }
 
+static void ffs_dmabuf_signal_done(struct ffs_dma_fence *dma_fence, int ret)
+{
+	struct ffs_dmabuf_priv *priv = dma_fence->priv;
+	struct dma_fence *fence = &dma_fence->base;
+
+	dma_fence_get(fence);
+	fence->error = ret;
+	dma_fence_signal(fence);
+
+	dma_buf_unmap_attachment(priv->attach, dma_fence->sgt, dma_fence->dir);
+	dma_fence_put(fence);
+	ffs_dmabuf_put(priv->attach);
+}
+
+static void ffs_epfile_dmabuf_io_complete(struct usb_ep *ep,
+					  struct usb_request *req)
+{
+	ENTER();
+
+	pr_debug("FFS: DMABUF transfer complete, status=%d\n", req->status);
+	ffs_dmabuf_signal_done(req->context, req->status);
+	usb_ep_free_request(ep, req);
+}
+
+static const char *ffs_dmabuf_get_driver_name(struct dma_fence *fence)
+{
+	return "functionfs";
+}
+
+static const char *ffs_dmabuf_get_timeline_name(struct dma_fence *fence)
+{
+	return "";
+}
+
+static void ffs_dmabuf_fence_release(struct dma_fence *fence)
+{
+	struct ffs_dma_fence *dma_fence = to_ffs_dma_fence(fence);
+
+	kfree(dma_fence);
+}
+
+static const struct dma_fence_ops ffs_dmabuf_fence_ops = {
+	.get_driver_name	= ffs_dmabuf_get_driver_name,
+	.get_timeline_name	= ffs_dmabuf_get_timeline_name,
+	.release		= ffs_dmabuf_fence_release,
+};
+
+int ffs_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;
+}
+
+static struct dma_buf_attachment *
+ffs_dmabuf_find_attachment(struct device *dev, struct dma_buf *dmabuf,
+			   bool nonblock)
+{
+	struct dma_buf_attachment *elm, *attach = NULL;
+	int ret;
+
+	ret = ffs_dma_resv_lock(dmabuf, nonblock);
+	if (ret)
+		return ERR_PTR(ret);
+
+	list_for_each_entry(elm, &dmabuf->attachments, node) {
+		if (elm->dev == dev) {
+			attach = elm;
+			break;
+		}
+	}
+
+	if (attach)
+		ffs_dmabuf_get(elm);
+
+	dma_resv_unlock(dmabuf->resv);
+
+	return attach ?: ERR_PTR(-EPERM);
+}
+
+static int ffs_dmabuf_attach(struct file *file, int fd)
+{
+	struct ffs_epfile *epfile = file->private_data;
+	struct usb_gadget *gadget = epfile->ffs->gadget;
+	struct dma_buf_attachment *attach;
+	struct ffs_dmabuf_priv *priv;
+	struct dma_buf *dmabuf;
+	int err;
+
+	if (!gadget || !gadget->sg_supported)
+		return -EPERM;
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	attach = dma_buf_attach(dmabuf, gadget->dev.parent);
+	if (IS_ERR(attach)) {
+		err = PTR_ERR(attach);
+		goto err_dmabuf_put;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		err = -ENOMEM;
+		goto err_dmabuf_detach;
+	}
+
+	attach->importer_priv = priv;
+
+	priv->attach = attach;
+	spin_lock_init(&priv->lock);
+	kref_init(&priv->ref);
+	priv->context = dma_fence_context_alloc(1);
+
+	list_add(&priv->entry, &epfile->dmabufs);
+
+	return 0;
+
+err_dmabuf_detach:
+	dma_buf_detach(dmabuf, attach);
+err_dmabuf_put:
+	dma_buf_put(dmabuf);
+
+	return err;
+}
+
+static int ffs_dmabuf_detach(struct file *file, int fd)
+{
+	struct ffs_epfile *epfile = file->private_data;
+	struct usb_gadget *gadget = epfile->ffs->gadget;
+	bool nonblock = file->f_flags & O_NONBLOCK;
+	struct dma_buf_attachment *attach;
+	struct dma_buf *dmabuf;
+	int ret = 0;
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
+					    dmabuf, nonblock);
+	if (IS_ERR(attach)) {
+		ret = PTR_ERR(attach);
+		goto out_dmabuf_put;
+	}
+
+	ffs_dmabuf_put(attach);
+	ffs_dmabuf_put(attach);
+
+out_dmabuf_put:
+	dma_buf_put(dmabuf);
+	return ret;
+}
+
+static int ffs_dmabuf_transfer(struct file *file,
+			       const struct usb_ffs_dmabuf_transfer_req *req)
+{
+	bool dma_to_ram, nonblock = file->f_flags & O_NONBLOCK;
+	struct ffs_epfile *epfile = file->private_data;
+	struct usb_gadget *gadget = epfile->ffs->gadget;
+	struct dma_buf_attachment *attach;
+	struct ffs_dmabuf_priv *priv;
+	enum dma_data_direction dir;
+	struct ffs_dma_fence *fence;
+	struct usb_request *usb_req;
+	struct sg_table *sg_table;
+	struct dma_buf *dmabuf;
+	struct ffs_ep *ep = epfile->ep;
+	int ret;
+
+	if (req->flags & ~USB_FFS_DMABUF_TRANSFER_MASK)
+		return -EINVAL;
+
+	dmabuf = dma_buf_get(req->fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	if (req->length > dmabuf->size || req->length == 0) {
+		ret = -EINVAL;
+		goto err_dmabuf_put;
+	}
+
+	attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
+					    dmabuf, nonblock);
+	if (IS_ERR(attach)) {
+		ret = PTR_ERR(attach);
+		goto err_dmabuf_put;
+	}
+
+	priv = attach->importer_priv;
+
+	if (epfile->in)
+		dir = DMA_FROM_DEVICE;
+	else
+		dir = DMA_TO_DEVICE;
+
+	sg_table = dma_buf_map_attachment(attach, dir);
+	if (IS_ERR(sg_table)) {
+		ret = PTR_ERR(sg_table);
+		goto err_attachment_put;
+	}
+
+	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence) {
+		ret = -ENOMEM;
+		goto err_unmap_attachment;
+	}
+
+	fence->sgt = sg_table;
+	fence->dir = dir;
+	fence->priv = priv;
+
+	dma_fence_init(&fence->base, &ffs_dmabuf_fence_ops,
+		       &priv->lock, priv->context, 0);
+
+	usb_req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
+	if (!usb_req) {
+		ret = -ENOMEM;
+		goto err_fence_put;
+	}
+
+	ret = ffs_dma_resv_lock(dmabuf, nonblock);
+	if (ret)
+		goto err_free_request;
+
+	/* Make sure we don't have writers */
+	if (!dma_resv_test_signaled(dmabuf->resv, DMA_RESV_USAGE_WRITE)) {
+		pr_debug("FFS WRITE fence is not signaled\n");
+		ret = -EBUSY;
+		goto err_resv_unlock;
+	}
+
+	dma_to_ram = dir == DMA_FROM_DEVICE;
+
+	/* If we're writing to the DMABUF, make sure we don't have readers */
+	if (dma_to_ram &&
+	    !dma_resv_test_signaled(dmabuf->resv, DMA_RESV_USAGE_READ)) {
+		pr_debug("FFS READ fence is not signaled\n");
+		ret = -EBUSY;
+		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);
+
+	/* Now that the dma_fence is in place, queue the transfer. */
+
+	usb_req->length = req->length;
+	usb_req->buf = NULL;
+	usb_req->sg = sg_table->sgl;
+	usb_req->num_sgs = sg_nents_for_len(sg_table->sgl, req->length);
+	usb_req->sg_was_mapped = true;
+	usb_req->context  = fence;
+	usb_req->complete = ffs_epfile_dmabuf_io_complete;
+
+	ret = usb_ep_queue(ep->ep, usb_req, GFP_ATOMIC);
+	if (ret) {
+		pr_warn("FFS: Failed to queue DMABUF: %d\n", ret);
+		ffs_dmabuf_signal_done(fence, ret);
+		usb_ep_free_request(ep->ep, usb_req);
+	}
+
+	dma_buf_put(dmabuf);
+
+	return ret;
+
+err_resv_unlock:
+	dma_resv_unlock(dmabuf->resv);
+err_free_request:
+	usb_ep_free_request(ep->ep, usb_req);
+err_fence_put:
+	dma_fence_put(&fence->base);
+err_unmap_attachment:
+	dma_buf_unmap_attachment(attach, sg_table, dir);
+err_attachment_put:
+	ffs_dmabuf_put(attach);
+err_dmabuf_put:
+	dma_buf_put(dmabuf);
+
+	return ret;
+}
+
 static long ffs_epfile_ioctl(struct file *file, unsigned code,
 			     unsigned long value)
 {
@@ -1364,8 +1724,45 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 			ret = -EFAULT;
 		return ret;
 	}
+	case FUNCTIONFS_DMABUF_ATTACH:
+	{
+		int fd;
+
+		if (copy_from_user(&fd, (void __user *)value, sizeof(fd))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = ffs_dmabuf_attach(file, fd);
+		break;
+	}
+	case FUNCTIONFS_DMABUF_DETACH:
+	{
+		int fd;
+
+		if (copy_from_user(&fd, (void __user *)value, sizeof(fd))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = ffs_dmabuf_detach(file, fd);
+		break;
+	}
+	case FUNCTIONFS_DMABUF_TRANSFER:
+	{
+		struct usb_ffs_dmabuf_transfer_req req;
+
+		if (copy_from_user(&req, (void __user *)value, sizeof(req))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = ffs_dmabuf_transfer(file, &req);
+		break;
+	}
 	default:
 		ret = -ENOTTY;
+		break;
 	}
 	spin_unlock_irq(&epfile->ffs->eps_lock);
 
@@ -1925,6 +2322,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
 	for (i = 1; i <= count; ++i, ++epfile) {
 		epfile->ffs = ffs;
 		mutex_init(&epfile->mutex);
+		INIT_LIST_HEAD(&epfile->dmabufs);
 		if (ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
 			sprintf(epfile->name, "ep%02x", ffs->eps_addrmap[i]);
 		else
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index d77ee6b65328..1412ab9f8ccc 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -84,6 +84,15 @@ struct usb_ext_prop_desc {
 	__le16	wPropertyNameLength;
 } __attribute__((packed));
 
+/* Flags for usb_ffs_dmabuf_transfer_req->flags (none for now) */
+#define USB_FFS_DMABUF_TRANSFER_MASK	0x0
+
+struct usb_ffs_dmabuf_transfer_req {
+	int fd;
+	__u32 flags;
+	__u64 length;
+} __attribute__((packed));
+
 #ifndef __KERNEL__
 
 /*
@@ -288,6 +297,9 @@ struct usb_functionfs_event {
 #define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
 					     struct usb_endpoint_descriptor)
 
-
+#define FUNCTIONFS_DMABUF_ATTACH	_IOW('g', 131, int)
+#define FUNCTIONFS_DMABUF_DETACH	_IOW('g', 132, int)
+#define FUNCTIONFS_DMABUF_TRANSFER	_IOW('g', 133, \
+					     struct usb_ffs_dmabuf_transfer_req)
 
 #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
-- 
2.39.2

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[Christian König]

Am 14.03.23 um 11:52 schrieb Paul Cercueil:
> This patch introduces three new ioctls. They all should be called on a
> data endpoint (ie. not ep0). They are:
>
> - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a DMABUF
>    object to attach to the endpoint.
>
> - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
>    DMABUF to detach from the endpoint. Note that closing the endpoint's
>    file descriptor will automatically detach all attached DMABUFs.
>
> - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from / to
>    the given DMABUF. Its argument is a structure that packs the DMABUF's
>    file descriptor, the size in bytes to transfer (which should generally
>    be set to the size of the DMABUF), and a 'flags' field which is unused
>    for now.
>    Before this ioctl can be used, the related DMABUF must be attached
>    with FUNCTIONFS_DMABUF_ATTACH.
>
> These three ioctls enable the FunctionFS code to transfer data between
> the USB stack and a DMABUF object, which can be provided by a driver
> from a completely different subsystem, in a zero-copy fashion.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>   drivers/usb/gadget/function/f_fs.c  | 398 ++++++++++++++++++++++++++++
>   include/uapi/linux/usb/functionfs.h |  14 +-
>   2 files changed, 411 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index ddfc537c7526..365fb716f0ad 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -15,6 +15,9 @@
>   /* #define VERBOSE_DEBUG */
>   
>   #include <linux/blkdev.h>
> +#include <linux/dma-buf.h>
> +#include <linux/dma-fence.h>
> +#include <linux/dma-resv.h>
>   #include <linux/pagemap.h>
>   #include <linux/export.h>
>   #include <linux/fs_parser.h>
> @@ -124,6 +127,26 @@ struct ffs_ep {
>   	u8				num;
>   };
>   
> +struct ffs_dmabuf_priv {
> +	struct list_head entry;
> +	struct kref ref;
> +	struct dma_buf_attachment *attach;
> +	spinlock_t lock;
> +	u64 context;
> +};
> +
> +struct ffs_dma_fence {
> +	struct dma_fence base;
> +	struct ffs_dmabuf_priv *priv;
> +	struct sg_table *sgt;
> +	enum dma_data_direction dir;
> +};
> +
> +static inline struct ffs_dma_fence *to_ffs_dma_fence(struct dma_fence *fence)
> +{
> +	return container_of(fence, struct ffs_dma_fence, base);
> +}
> +
>   struct ffs_epfile {
>   	/* Protects ep->ep and ep->req. */
>   	struct mutex			mutex;
> @@ -197,6 +220,8 @@ struct ffs_epfile {
>   	unsigned char			isoc;	/* P: ffs->eps_lock */
>   
>   	unsigned char			_pad;
> +
> +	struct list_head		dmabufs;
>   };
>   
>   struct ffs_buffer {
> @@ -1279,19 +1304,354 @@ static ssize_t ffs_epfile_read_iter(struct kiocb *kiocb, struct iov_iter *to)
>   	return res;
>   }
>   
> +static void ffs_dmabuf_release(struct kref *ref)
> +{
> +	struct ffs_dmabuf_priv *priv = container_of(ref, struct ffs_dmabuf_priv, ref);
> +	struct dma_buf_attachment *attach = priv->attach;
> +	struct dma_buf *dmabuf = attach->dmabuf;
> +
> +	pr_debug("FFS DMABUF release\n");
> +	dma_buf_detach(attach->dmabuf, attach);
> +	dma_buf_put(dmabuf);
> +
> +	list_del(&priv->entry);
> +	kfree(priv);
> +}
> +
> +static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
> +{
> +	struct ffs_dmabuf_priv *priv = attach->importer_priv;
> +
> +	kref_get(&priv->ref);
> +}
> +
> +static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
> +{
> +	struct ffs_dmabuf_priv *priv = attach->importer_priv;
> +
> +	kref_put(&priv->ref, ffs_dmabuf_release);
> +}
> +
>   static int
>   ffs_epfile_release(struct inode *inode, struct file *file)
>   {
>   	struct ffs_epfile *epfile = inode->i_private;
> +	struct ffs_dmabuf_priv *priv, *tmp;
>   
>   	ENTER();
>   
> +	/* Close all attached DMABUFs */
> +	list_for_each_entry_safe(priv, tmp, &epfile->dmabufs, entry) {
> +		ffs_dmabuf_put(priv->attach);
> +	}
> +
>   	__ffs_epfile_read_buffer_free(epfile);
>   	ffs_data_closed(epfile->ffs);
>   
>   	return 0;
>   }
>   
> +static void ffs_dmabuf_signal_done(struct ffs_dma_fence *dma_fence, int ret)
> +{
> +	struct ffs_dmabuf_priv *priv = dma_fence->priv;
> +	struct dma_fence *fence = &dma_fence->base;
> +
> +	dma_fence_get(fence);
> +	fence->error = ret;
> +	dma_fence_signal(fence);
> +
> +	dma_buf_unmap_attachment(priv->attach, dma_fence->sgt, dma_fence->dir);
> +	dma_fence_put(fence);
> +	ffs_dmabuf_put(priv->attach);
> +}
> +
> +static void ffs_epfile_dmabuf_io_complete(struct usb_ep *ep,
> +					  struct usb_request *req)
> +{
> +	ENTER();
> +
> +	pr_debug("FFS: DMABUF transfer complete, status=%d\n", req->status);
> +	ffs_dmabuf_signal_done(req->context, req->status);
> +	usb_ep_free_request(ep, req);
> +}
> +
> +static const char *ffs_dmabuf_get_driver_name(struct dma_fence *fence)
> +{
> +	return "functionfs";
> +}
> +
> +static const char *ffs_dmabuf_get_timeline_name(struct dma_fence *fence)
> +{
> +	return "";
> +}
> +
> +static void ffs_dmabuf_fence_release(struct dma_fence *fence)
> +{
> +	struct ffs_dma_fence *dma_fence = to_ffs_dma_fence(fence);
> +
> +	kfree(dma_fence);
> +}
> +
> +static const struct dma_fence_ops ffs_dmabuf_fence_ops = {
> +	.get_driver_name	= ffs_dmabuf_get_driver_name,
> +	.get_timeline_name	= ffs_dmabuf_get_timeline_name,
> +	.release		= ffs_dmabuf_fence_release,
> +};
> +
> +int ffs_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;
> +}
> +
> +static struct dma_buf_attachment *
> +ffs_dmabuf_find_attachment(struct device *dev, struct dma_buf *dmabuf,
> +			   bool nonblock)
> +{
> +	struct dma_buf_attachment *elm, *attach = NULL;
> +	int ret;
> +
> +	ret = ffs_dma_resv_lock(dmabuf, nonblock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	list_for_each_entry(elm, &dmabuf->attachments, node) {
> +		if (elm->dev == dev) {
> +			attach = elm;
> +			break;
> +		}
> +	}
> +
> +	if (attach)
> +		ffs_dmabuf_get(elm);
> +
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	return attach ?: ERR_PTR(-EPERM);
> +}
> +
> +static int ffs_dmabuf_attach(struct file *file, int fd)
> +{
> +	struct ffs_epfile *epfile = file->private_data;
> +	struct usb_gadget *gadget = epfile->ffs->gadget;
> +	struct dma_buf_attachment *attach;
> +	struct ffs_dmabuf_priv *priv;
> +	struct dma_buf *dmabuf;
> +	int err;
> +
> +	if (!gadget || !gadget->sg_supported)
> +		return -EPERM;
> +
> +	dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	attach = dma_buf_attach(dmabuf, gadget->dev.parent);
> +	if (IS_ERR(attach)) {
> +		err = PTR_ERR(attach);
> +		goto err_dmabuf_put;
> +	}
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		err = -ENOMEM;
> +		goto err_dmabuf_detach;
> +	}
> +
> +	attach->importer_priv = priv;
> +
> +	priv->attach = attach;
> +	spin_lock_init(&priv->lock);
> +	kref_init(&priv->ref);
> +	priv->context = dma_fence_context_alloc(1);
> +
> +	list_add(&priv->entry, &epfile->dmabufs);
> +
> +	return 0;
> +
> +err_dmabuf_detach:
> +	dma_buf_detach(dmabuf, attach);
> +err_dmabuf_put:
> +	dma_buf_put(dmabuf);
> +
> +	return err;
> +}
> +
> +static int ffs_dmabuf_detach(struct file *file, int fd)
> +{
> +	struct ffs_epfile *epfile = file->private_data;
> +	struct usb_gadget *gadget = epfile->ffs->gadget;
> +	bool nonblock = file->f_flags & O_NONBLOCK;
> +	struct dma_buf_attachment *attach;
> +	struct dma_buf *dmabuf;
> +	int ret = 0;
> +
> +	dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
> +					    dmabuf, nonblock);
> +	if (IS_ERR(attach)) {
> +		ret = PTR_ERR(attach);
> +		goto out_dmabuf_put;
> +	}
> +
> +	ffs_dmabuf_put(attach);
> +	ffs_dmabuf_put(attach);
> +
> +out_dmabuf_put:
> +	dma_buf_put(dmabuf);
> +	return ret;
> +}
> +
> +static int ffs_dmabuf_transfer(struct file *file,
> +			       const struct usb_ffs_dmabuf_transfer_req *req)
> +{
> +	bool dma_to_ram, nonblock = file->f_flags & O_NONBLOCK;
> +	struct ffs_epfile *epfile = file->private_data;
> +	struct usb_gadget *gadget = epfile->ffs->gadget;
> +	struct dma_buf_attachment *attach;
> +	struct ffs_dmabuf_priv *priv;
> +	enum dma_data_direction dir;
> +	struct ffs_dma_fence *fence;
> +	struct usb_request *usb_req;
> +	struct sg_table *sg_table;
> +	struct dma_buf *dmabuf;
> +	struct ffs_ep *ep = epfile->ep;
> +	int ret;
> +
> +	if (req->flags & ~USB_FFS_DMABUF_TRANSFER_MASK)
> +		return -EINVAL;
> +
> +	dmabuf = dma_buf_get(req->fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	if (req->length > dmabuf->size || req->length == 0) {
> +		ret = -EINVAL;
> +		goto err_dmabuf_put;
> +	}
> +
> +	attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
> +					    dmabuf, nonblock);
> +	if (IS_ERR(attach)) {
> +		ret = PTR_ERR(attach);
> +		goto err_dmabuf_put;
> +	}
> +
> +	priv = attach->importer_priv;
> +
> +	if (epfile->in)
> +		dir = DMA_FROM_DEVICE;
> +	else
> +		dir = DMA_TO_DEVICE;
> +
> +	sg_table = dma_buf_map_attachment(attach, dir);
> +	if (IS_ERR(sg_table)) {
> +		ret = PTR_ERR(sg_table);
> +		goto err_attachment_put;
> +	}
> +
> +	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence) {
> +		ret = -ENOMEM;
> +		goto err_unmap_attachment;
> +	}
> +
> +	fence->sgt = sg_table;
> +	fence->dir = dir;
> +	fence->priv = priv;
> +
> +	dma_fence_init(&fence->base, &ffs_dmabuf_fence_ops,
> +		       &priv->lock, priv->context, 0);

Sequence numbers must be unique, so constantly using 0 here is quite 
questionable.

If you set the use_64bit_seqno flag in your fence ops structure you can 
simply use a 64bit counter.

> +
> +	usb_req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
> +	if (!usb_req) {
> +		ret = -ENOMEM;
> +		goto err_fence_put;
> +	}
> +
> +	ret = ffs_dma_resv_lock(dmabuf, nonblock);
> +	if (ret)
> +		goto err_free_request;
> +
> +	/* Make sure we don't have writers */
> +	if (!dma_resv_test_signaled(dmabuf->resv, DMA_RESV_USAGE_WRITE)) {
> +		pr_debug("FFS WRITE fence is not signaled\n");
> +		ret = -EBUSY;
> +		goto err_resv_unlock;
> +	}
> +
> +	dma_to_ram = dir == DMA_FROM_DEVICE;
> +
> +	/* If we're writing to the DMABUF, make sure we don't have readers */
> +	if (dma_to_ram &&
> +	    !dma_resv_test_signaled(dmabuf->resv, DMA_RESV_USAGE_READ)) {
> +		pr_debug("FFS READ fence is not signaled\n");
> +		ret = -EBUSY;
> +		goto err_resv_unlock;
> +	}
> +
> +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> +	if (ret)
> +		goto err_resv_unlock;

A dma_fence should only be published if nothing can go wrong with the 
request any more.

You are doing that correctly here, but it is good practice that you also 
don't allocate and initialize the fence before you publish it.

In other words reorder this and move the allocation and call to 
dma_fence_init() here if possible.

> +
> +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> +			   dma_resv_usage_rw(dma_to_ram));
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	/* Now that the dma_fence is in place, queue the transfer. */
> +
> +	usb_req->length = req->length;
> +	usb_req->buf = NULL;
> +	usb_req->sg = sg_table->sgl;
> +	usb_req->num_sgs = sg_nents_for_len(sg_table->sgl, req->length);
> +	usb_req->sg_was_mapped = true;
> +	usb_req->context  = fence;
> +	usb_req->complete = ffs_epfile_dmabuf_io_complete;
> +
> +	ret = usb_ep_queue(ep->ep, usb_req, GFP_ATOMIC);

Already using GFP_ATOMIC gets you a lot of bonus points for this patch 
:) Background is that as soon as you have published a fence you can't 
use GFP_KERNEL that easily any more because that memory allocation might 
wait for the fence.

But even better would be to try to avoid memory allocation in the 
critical code path altogether. Would it be possible to call this 
function with the dma_resv_lock held?

Regards,
Christian.


> +	if (ret) {
> +		pr_warn("FFS: Failed to queue DMABUF: %d\n", ret);
> +		ffs_dmabuf_signal_done(fence, ret);
> +		usb_ep_free_request(ep->ep, usb_req);
> +	}
> +
> +	dma_buf_put(dmabuf);
> +
> +	return ret;
> +
> +err_resv_unlock:
> +	dma_resv_unlock(dmabuf->resv);
> +err_free_request:
> +	usb_ep_free_request(ep->ep, usb_req);
> +err_fence_put:
> +	dma_fence_put(&fence->base);
> +err_unmap_attachment:
> +	dma_buf_unmap_attachment(attach, sg_table, dir);
> +err_attachment_put:
> +	ffs_dmabuf_put(attach);
> +err_dmabuf_put:
> +	dma_buf_put(dmabuf);
> +
> +	return ret;
> +}
> +
>   static long ffs_epfile_ioctl(struct file *file, unsigned code,
>   			     unsigned long value)
>   {
> @@ -1364,8 +1724,45 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
>   			ret = -EFAULT;
>   		return ret;
>   	}
> +	case FUNCTIONFS_DMABUF_ATTACH:
> +	{
> +		int fd;
> +
> +		if (copy_from_user(&fd, (void __user *)value, sizeof(fd))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		ret = ffs_dmabuf_attach(file, fd);
> +		break;
> +	}
> +	case FUNCTIONFS_DMABUF_DETACH:
> +	{
> +		int fd;
> +
> +		if (copy_from_user(&fd, (void __user *)value, sizeof(fd))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		ret = ffs_dmabuf_detach(file, fd);
> +		break;
> +	}
> +	case FUNCTIONFS_DMABUF_TRANSFER:
> +	{
> +		struct usb_ffs_dmabuf_transfer_req req;
> +
> +		if (copy_from_user(&req, (void __user *)value, sizeof(req))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		ret = ffs_dmabuf_transfer(file, &req);
> +		break;
> +	}
>   	default:
>   		ret = -ENOTTY;
> +		break;
>   	}
>   	spin_unlock_irq(&epfile->ffs->eps_lock);
>   
> @@ -1925,6 +2322,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
>   	for (i = 1; i <= count; ++i, ++epfile) {
>   		epfile->ffs = ffs;
>   		mutex_init(&epfile->mutex);
> +		INIT_LIST_HEAD(&epfile->dmabufs);
>   		if (ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
>   			sprintf(epfile->name, "ep%02x", ffs->eps_addrmap[i]);
>   		else
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index d77ee6b65328..1412ab9f8ccc 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -84,6 +84,15 @@ struct usb_ext_prop_desc {
>   	__le16	wPropertyNameLength;
>   } __attribute__((packed));
>   
> +/* Flags for usb_ffs_dmabuf_transfer_req->flags (none for now) */
> +#define USB_FFS_DMABUF_TRANSFER_MASK	0x0
> +
> +struct usb_ffs_dmabuf_transfer_req {
> +	int fd;
> +	__u32 flags;
> +	__u64 length;
> +} __attribute__((packed));
> +
>   #ifndef __KERNEL__
>   
>   /*
> @@ -288,6 +297,9 @@ struct usb_functionfs_event {
>   #define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
>   					     struct usb_endpoint_descriptor)
>   
> -
> +#define FUNCTIONFS_DMABUF_ATTACH	_IOW('g', 131, int)
> +#define FUNCTIONFS_DMABUF_DETACH	_IOW('g', 132, int)
> +#define FUNCTIONFS_DMABUF_TRANSFER	_IOW('g', 133, \
> +					     struct usb_ffs_dmabuf_transfer_req)
>   
>   #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[Paul Cercueil]

Hi Christian,

Le mardi 14 mars 2023 à 12:59 +0100, Christian König a écrit :
> > Am 14.03.23 um 11:52 schrieb Paul Cercueil:
> > > > This patch introduces three new ioctls. They all should be
> > > > called
> > > > on a
> > > > data endpoint (ie. not ep0). They are:
> > > > 
> > > > - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of
> > > > a
> > > > DMABUF
> > > >    object to attach to the endpoint.
> > > > 
> > > > - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of
> > > > the
> > > >    DMABUF to detach from the endpoint. Note that closing the
> > > > endpoint's
> > > >    file descriptor will automatically detach all attached
> > > > DMABUFs.
> > > > 
> > > > - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer
> > > > from /
> > > > to
> > > >    the given DMABUF. Its argument is a structure that packs the
> > > > DMABUF's
> > > >    file descriptor, the size in bytes to transfer (which should
> > > > generally
> > > >    be set to the size of the DMABUF), and a 'flags' field which
> > > > is
> > > > unused
> > > >    for now.
> > > >    Before this ioctl can be used, the related DMABUF must be
> > > > attached
> > > >    with FUNCTIONFS_DMABUF_ATTACH.
> > > > 
> > > > These three ioctls enable the FunctionFS code to transfer data
> > > > between
> > > > the USB stack and a DMABUF object, which can be provided by a
> > > > driver
> > > > from a completely different subsystem, in a zero-copy fashion.
> > > > 
> > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > ---
> > > >   drivers/usb/gadget/function/f_fs.c  | 398
> > > > ++++++++++++++++++++++++++++
> > > >   include/uapi/linux/usb/functionfs.h |  14 +-
> > > >   2 files changed, 411 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/gadget/function/f_fs.c
> > > > b/drivers/usb/gadget/function/f_fs.c
> > > > index ddfc537c7526..365fb716f0ad 100644
> > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > @@ -15,6 +15,9 @@
> > > >   /* #define VERBOSE_DEBUG */
> > > >   
> > > >   #include <linux/blkdev.h>
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/dma-fence.h>
> > > > +#include <linux/dma-resv.h>
> > > >   #include <linux/pagemap.h>
> > > >   #include <linux/export.h>
> > > >   #include <linux/fs_parser.h>
> > > > @@ -124,6 +127,26 @@ struct ffs_ep {
> > > >         u8                              num;
> > > >   };
> > > >   
> > > > +struct ffs_dmabuf_priv {
> > > > +       struct list_head entry;
> > > > +       struct kref ref;
> > > > +       struct dma_buf_attachment *attach;
> > > > +       spinlock_t lock;
> > > > +       u64 context;
> > > > +};
> > > > +
> > > > +struct ffs_dma_fence {
> > > > +       struct dma_fence base;
> > > > +       struct ffs_dmabuf_priv *priv;
> > > > +       struct sg_table *sgt;
> > > > +       enum dma_data_direction dir;
> > > > +};
> > > > +
> > > > +static inline struct ffs_dma_fence *to_ffs_dma_fence(struct
> > > > dma_fence *fence)
> > > > +{
> > > > +       return container_of(fence, struct ffs_dma_fence, base);
> > > > +}
> > > > +
> > > >   struct ffs_epfile {
> > > >         /* Protects ep->ep and ep->req. */
> > > >         struct mutex                    mutex;
> > > > @@ -197,6 +220,8 @@ struct ffs_epfile {
> > > >         unsigned char                   isoc;   /* P: ffs-
> > > > >eps_lock
> > > > */
> > > >   
> > > >         unsigned char                   _pad;
> > > > +
> > > > +       struct list_head                dmabufs;
> > > >   };
> > > >   
> > > >   struct ffs_buffer {
> > > > @@ -1279,19 +1304,354 @@ static ssize_t
> > > > ffs_epfile_read_iter(struct
> > > > kiocb *kiocb, struct iov_iter *to)
> > > >         return res;
> > > >   }
> > > >   
> > > > +static void ffs_dmabuf_release(struct kref *ref)
> > > > +{
> > > > +       struct ffs_dmabuf_priv *priv = container_of(ref, struct
> > > > ffs_dmabuf_priv, ref);
> > > > +       struct dma_buf_attachment *attach = priv->attach;
> > > > +       struct dma_buf *dmabuf = attach->dmabuf;
> > > > +
> > > > +       pr_debug("FFS DMABUF release\n");
> > > > +       dma_buf_detach(attach->dmabuf, attach);
> > > > +       dma_buf_put(dmabuf);
> > > > +
> > > > +       list_del(&priv->entry);
> > > > +       kfree(priv);
> > > > +}
> > > > +
> > > > +static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
> > > > +{
> > > > +       struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > > > +
> > > > +       kref_get(&priv->ref);
> > > > +}
> > > > +
> > > > +static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
> > > > +{
> > > > +       struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > > > +
> > > > +       kref_put(&priv->ref, ffs_dmabuf_release);
> > > > +}
> > > > +
> > > >   static int
> > > >   ffs_epfile_release(struct inode *inode, struct file *file)
> > > >   {
> > > >         struct ffs_epfile *epfile = inode->i_private;
> > > > +       struct ffs_dmabuf_priv *priv, *tmp;
> > > >   
> > > >         ENTER();
> > > >   
> > > > +       /* Close all attached DMABUFs */
> > > > +       list_for_each_entry_safe(priv, tmp, &epfile->dmabufs,
> > > > entry) {
> > > > +               ffs_dmabuf_put(priv->attach);
> > > > +       }
> > > > +
> > > >         __ffs_epfile_read_buffer_free(epfile);
> > > >         ffs_data_closed(epfile->ffs);
> > > >   
> > > >         return 0;
> > > >   }
> > > >   
> > > > +static void ffs_dmabuf_signal_done(struct ffs_dma_fence
> > > > *dma_fence, int ret)
> > > > +{
> > > > +       struct ffs_dmabuf_priv *priv = dma_fence->priv;
> > > > +       struct dma_fence *fence = &dma_fence->base;
> > > > +
> > > > +       dma_fence_get(fence);
> > > > +       fence->error = ret;
> > > > +       dma_fence_signal(fence);
> > > > +
> > > > +       dma_buf_unmap_attachment(priv->attach, dma_fence->sgt,
> > > > dma_fence->dir);
> > > > +       dma_fence_put(fence);
> > > > +       ffs_dmabuf_put(priv->attach);
> > > > +}
> > > > +
> > > > +static void ffs_epfile_dmabuf_io_complete(struct usb_ep *ep,
> > > > +                                         struct usb_request
> > > > *req)
> > > > +{
> > > > +       ENTER();
> > > > +
> > > > +       pr_debug("FFS: DMABUF transfer complete, status=%d\n",
> > > > req->status);
> > > > +       ffs_dmabuf_signal_done(req->context, req->status);
> > > > +       usb_ep_free_request(ep, req);
> > > > +}
> > > > +
> > > > +static const char *ffs_dmabuf_get_driver_name(struct dma_fence
> > > > *fence)
> > > > +{
> > > > +       return "functionfs";
> > > > +}
> > > > +
> > > > +static const char *ffs_dmabuf_get_timeline_name(struct
> > > > dma_fence
> > > > *fence)
> > > > +{
> > > > +       return "";
> > > > +}
> > > > +
> > > > +static void ffs_dmabuf_fence_release(struct dma_fence *fence)
> > > > +{
> > > > +       struct ffs_dma_fence *dma_fence =
> > > > to_ffs_dma_fence(fence);
> > > > +
> > > > +       kfree(dma_fence);
> > > > +}
> > > > +
> > > > +static const struct dma_fence_ops ffs_dmabuf_fence_ops = {
> > > > +       .get_driver_name        = ffs_dmabuf_get_driver_name,
> > > > +       .get_timeline_name      = ffs_dmabuf_get_timeline_name,
> > > > +       .release                = ffs_dmabuf_fence_release,
> > > > +};
> > > > +
> > > > +int ffs_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;
> > > > +}
> > > > +
> > > > +static struct dma_buf_attachment *
> > > > +ffs_dmabuf_find_attachment(struct device *dev, struct dma_buf
> > > > *dmabuf,
> > > > +                          bool nonblock)
> > > > +{
> > > > +       struct dma_buf_attachment *elm, *attach = NULL;
> > > > +       int ret;
> > > > +
> > > > +       ret = ffs_dma_resv_lock(dmabuf, nonblock);
> > > > +       if (ret)
> > > > +               return ERR_PTR(ret);
> > > > +
> > > > +       list_for_each_entry(elm, &dmabuf->attachments, node) {
> > > > +               if (elm->dev == dev) {
> > > > +                       attach = elm;
> > > > +                       break;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       if (attach)
> > > > +               ffs_dmabuf_get(elm);
> > > > +
> > > > +       dma_resv_unlock(dmabuf->resv);
> > > > +
> > > > +       return attach ?: ERR_PTR(-EPERM);
> > > > +}
> > > > +
> > > > +static int ffs_dmabuf_attach(struct file *file, int fd)
> > > > +{
> > > > +       struct ffs_epfile *epfile = file->private_data;
> > > > +       struct usb_gadget *gadget = epfile->ffs->gadget;
> > > > +       struct dma_buf_attachment *attach;
> > > > +       struct ffs_dmabuf_priv *priv;
> > > > +       struct dma_buf *dmabuf;
> > > > +       int err;
> > > > +
> > > > +       if (!gadget || !gadget->sg_supported)
> > > > +               return -EPERM;
> > > > +
> > > > +       dmabuf = dma_buf_get(fd);
> > > > +       if (IS_ERR(dmabuf))
> > > > +               return PTR_ERR(dmabuf);
> > > > +
> > > > +       attach = dma_buf_attach(dmabuf, gadget->dev.parent);
> > > > +       if (IS_ERR(attach)) {
> > > > +               err = PTR_ERR(attach);
> > > > +               goto err_dmabuf_put;
> > > > +       }
> > > > +
> > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > +       if (!priv) {
> > > > +               err = -ENOMEM;
> > > > +               goto err_dmabuf_detach;
> > > > +       }
> > > > +
> > > > +       attach->importer_priv = priv;
> > > > +
> > > > +       priv->attach = attach;
> > > > +       spin_lock_init(&priv->lock);
> > > > +       kref_init(&priv->ref);
> > > > +       priv->context = dma_fence_context_alloc(1);
> > > > +
> > > > +       list_add(&priv->entry, &epfile->dmabufs);
> > > > +
> > > > +       return 0;
> > > > +
> > > > +err_dmabuf_detach:
> > > > +       dma_buf_detach(dmabuf, attach);
> > > > +err_dmabuf_put:
> > > > +       dma_buf_put(dmabuf);
> > > > +
> > > > +       return err;
> > > > +}
> > > > +
> > > > +static int ffs_dmabuf_detach(struct file *file, int fd)
> > > > +{
> > > > +       struct ffs_epfile *epfile = file->private_data;
> > > > +       struct usb_gadget *gadget = epfile->ffs->gadget;
> > > > +       bool nonblock = file->f_flags & O_NONBLOCK;
> > > > +       struct dma_buf_attachment *attach;
> > > > +       struct dma_buf *dmabuf;
> > > > +       int ret = 0;
> > > > +
> > > > +       dmabuf = dma_buf_get(fd);
> > > > +       if (IS_ERR(dmabuf))
> > > > +               return PTR_ERR(dmabuf);
> > > > +
> > > > +       attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
> > > > +                                           dmabuf, nonblock);
> > > > +       if (IS_ERR(attach)) {
> > > > +               ret = PTR_ERR(attach);
> > > > +               goto out_dmabuf_put;
> > > > +       }
> > > > +
> > > > +       ffs_dmabuf_put(attach);
> > > > +       ffs_dmabuf_put(attach);
> > > > +
> > > > +out_dmabuf_put:
> > > > +       dma_buf_put(dmabuf);
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static int ffs_dmabuf_transfer(struct file *file,
> > > > +                              const struct
> > > > usb_ffs_dmabuf_transfer_req *req)
> > > > +{
> > > > +       bool dma_to_ram, nonblock = file->f_flags & O_NONBLOCK;
> > > > +       struct ffs_epfile *epfile = file->private_data;
> > > > +       struct usb_gadget *gadget = epfile->ffs->gadget;
> > > > +       struct dma_buf_attachment *attach;
> > > > +       struct ffs_dmabuf_priv *priv;
> > > > +       enum dma_data_direction dir;
> > > > +       struct ffs_dma_fence *fence;
> > > > +       struct usb_request *usb_req;
> > > > +       struct sg_table *sg_table;
> > > > +       struct dma_buf *dmabuf;
> > > > +       struct ffs_ep *ep = epfile->ep;
> > > > +       int ret;
> > > > +
> > > > +       if (req->flags & ~USB_FFS_DMABUF_TRANSFER_MASK)
> > > > +               return -EINVAL;
> > > > +
> > > > +       dmabuf = dma_buf_get(req->fd);
> > > > +       if (IS_ERR(dmabuf))
> > > > +               return PTR_ERR(dmabuf);
> > > > +
> > > > +       if (req->length > dmabuf->size || req->length == 0) {
> > > > +               ret = -EINVAL;
> > > > +               goto err_dmabuf_put;
> > > > +       }
> > > > +
> > > > +       attach = ffs_dmabuf_find_attachment(gadget->dev.parent,
> > > > +                                           dmabuf, nonblock);
> > > > +       if (IS_ERR(attach)) {
> > > > +               ret = PTR_ERR(attach);
> > > > +               goto err_dmabuf_put;
> > > > +       }
> > > > +
> > > > +       priv = attach->importer_priv;
> > > > +
> > > > +       if (epfile->in)
> > > > +               dir = DMA_FROM_DEVICE;
> > > > +       else
> > > > +               dir = DMA_TO_DEVICE;
> > > > +
> > > > +       sg_table = dma_buf_map_attachment(attach, dir);
> > > > +       if (IS_ERR(sg_table)) {
> > > > +               ret = PTR_ERR(sg_table);
> > > > +               goto err_attachment_put;
> > > > +       }
> > > > +
> > > > +       fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> > > > +       if (!fence) {
> > > > +               ret = -ENOMEM;
> > > > +               goto err_unmap_attachment;
> > > > +       }
> > > > +
> > > > +       fence->sgt = sg_table;
> > > > +       fence->dir = dir;
> > > > +       fence->priv = priv;
> > > > +
> > > > +       dma_fence_init(&fence->base, &ffs_dmabuf_fence_ops,
> > > > +                      &priv->lock, priv->context, 0);
> > 
> > Sequence numbers must be unique, so constantly using 0 here is
> > quite 
> > questionable.
> > 
> > If you set the use_64bit_seqno flag in your fence ops structure you
> > can 
> > simply use a 64bit counter.
> > 
> > > > +
> > > > +       usb_req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
> > > > +       if (!usb_req) {
> > > > +               ret = -ENOMEM;
> > > > +               goto err_fence_put;
> > > > +       }
> > > > +
> > > > +       ret = ffs_dma_resv_lock(dmabuf, nonblock);
> > > > +       if (ret)
> > > > +               goto err_free_request;
> > > > +
> > > > +       /* Make sure we don't have writers */
> > > > +       if (!dma_resv_test_signaled(dmabuf->resv,
> > > > DMA_RESV_USAGE_WRITE)) {
> > > > +               pr_debug("FFS WRITE fence is not signaled\n");
> > > > +               ret = -EBUSY;
> > > > +               goto err_resv_unlock;
> > > > +       }
> > > > +
> > > > +       dma_to_ram = dir == DMA_FROM_DEVICE;
> > > > +
> > > > +       /* If we're writing to the DMABUF, make sure we don't
> > > > have
> > > > readers */
> > > > +       if (dma_to_ram &&
> > > > +           !dma_resv_test_signaled(dmabuf->resv,
> > > > DMA_RESV_USAGE_READ)) {
> > > > +               pr_debug("FFS READ fence is not signaled\n");
> > > > +               ret = -EBUSY;
> > > > +               goto err_resv_unlock;
> > > > +       }
> > > > +
> > > > +       ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > > > +       if (ret)
> > > > +               goto err_resv_unlock;
> > 
> > A dma_fence should only be published if nothing can go wrong with
> > the
> > request any more.
> > 
> > You are doing that correctly here, but it is good practice that you
> > also 
> > don't allocate and initialize the fence before you publish it.
> > 
> > In other words reorder this and move the allocation and call to 
> > dma_fence_init() here if possible.
> > 
> > > > +
> > > > +       dma_resv_add_fence(dmabuf->resv, &fence->base,
> > > > +                          dma_resv_usage_rw(dma_to_ram));
> > > > +       dma_resv_unlock(dmabuf->resv);
> > > > +
> > > > +       /* Now that the dma_fence is in place, queue the
> > > > transfer.
> > > > */
> > > > +
> > > > +       usb_req->length = req->length;
> > > > +       usb_req->buf = NULL;
> > > > +       usb_req->sg = sg_table->sgl;
> > > > +       usb_req->num_sgs = sg_nents_for_len(sg_table->sgl,
> > > > req->length);
> > > > +       usb_req->sg_was_mapped = true;
> > > > +       usb_req->context  = fence;
> > > > +       usb_req->complete = ffs_epfile_dmabuf_io_complete;
> > > > +
> > > > +       ret = usb_ep_queue(ep->ep, usb_req, GFP_ATOMIC);
> > 
> > Already using GFP_ATOMIC gets you a lot of bonus points for this
> > patch 
> > :) Background is that as soon as you have published a fence you
> > can't
> > use GFP_KERNEL that easily any more because that memory allocation
> > might 
> > wait for the fence.

I actually did not know this and that's not the reason why I'm using
GFP_ATOMIC here. The caller function calls a spin_lock_irq() to prevent
the USB endpoint from being disabled or changed under us. So
usb_ep_alloc_request() and usb_ep_queue() must use GFP_ATOMIC.

Now, I totally forgot about this spinlock being locked in the caller
function, so I can't just lock the resv here (nevermind use GFP_KERNEL
to alloc the fence). I'll have to rework that.

Cheers,
-Paul

> > But even better would be to try to avoid memory allocation in the 
> > critical code path altogether. Would it be possible to call this 
> > function with the dma_resv_lock held?
> > 
> > Regards,
> > Christian.
> > 
> > 
> > > > +       if (ret) {
> > > > +               pr_warn("FFS: Failed to queue DMABUF: %d\n",
> > > > ret);
> > > > +               ffs_dmabuf_signal_done(fence, ret);
> > > > +               usb_ep_free_request(ep->ep, usb_req);
> > > > +       }
> > > > +
> > > > +       dma_buf_put(dmabuf);
> > > > +
> > > > +       return ret;
> > > > +
> > > > +err_resv_unlock:
> > > > +       dma_resv_unlock(dmabuf->resv);
> > > > +err_free_request:
> > > > +       usb_ep_free_request(ep->ep, usb_req);
> > > > +err_fence_put:
> > > > +       dma_fence_put(&fence->base);
> > > > +err_unmap_attachment:
> > > > +       dma_buf_unmap_attachment(attach, sg_table, dir);
> > > > +err_attachment_put:
> > > > +       ffs_dmabuf_put(attach);
> > > > +err_dmabuf_put:
> > > > +       dma_buf_put(dmabuf);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > >   static long ffs_epfile_ioctl(struct file *file, unsigned
> > > > code,
> > > >                              unsigned long value)
> > > >   {
> > > > @@ -1364,8 +1724,45 @@ static long ffs_epfile_ioctl(struct file
> > > > *file, unsigned code,
> > > >                         ret = -EFAULT;
> > > >                 return ret;
> > > >         }
> > > > +       case FUNCTIONFS_DMABUF_ATTACH:
> > > > +       {
> > > > +               int fd;
> > > > +
> > > > +               if (copy_from_user(&fd, (void __user *)value,
> > > > sizeof(fd))) {
> > > > +                       ret = -EFAULT;
> > > > +                       break;
> > > > +               }
> > > > +
> > > > +               ret = ffs_dmabuf_attach(file, fd);
> > > > +               break;
> > > > +       }
> > > > +       case FUNCTIONFS_DMABUF_DETACH:
> > > > +       {
> > > > +               int fd;
> > > > +
> > > > +               if (copy_from_user(&fd, (void __user *)value,
> > > > sizeof(fd))) {
> > > > +                       ret = -EFAULT;
> > > > +                       break;
> > > > +               }
> > > > +
> > > > +               ret = ffs_dmabuf_detach(file, fd);
> > > > +               break;
> > > > +       }
> > > > +       case FUNCTIONFS_DMABUF_TRANSFER:
> > > > +       {
> > > > +               struct usb_ffs_dmabuf_transfer_req req;
> > > > +
> > > > +               if (copy_from_user(&req, (void __user *)value,
> > > > sizeof(req))) {
> > > > +                       ret = -EFAULT;
> > > > +                       break;
> > > > +               }
> > > > +
> > > > +               ret = ffs_dmabuf_transfer(file, &req);
> > > > +               break;
> > > > +       }
> > > >         default:
> > > >                 ret = -ENOTTY;
> > > > +               break;
> > > >         }
> > > >         spin_unlock_irq(&epfile->ffs->eps_lock);
> > > >   
> > > > @@ -1925,6 +2322,7 @@ static int ffs_epfiles_create(struct
> > > > ffs_data
> > > > *ffs)
> > > >         for (i = 1; i <= count; ++i, ++epfile) {
> > > >                 epfile->ffs = ffs;
> > > >                 mutex_init(&epfile->mutex);
> > > > +               INIT_LIST_HEAD(&epfile->dmabufs);
> > > >                 if (ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
> > > >                         sprintf(epfile->name, "ep%02x",
> > > > ffs->eps_addrmap[i]);
> > > >                 else
> > > > diff --git a/include/uapi/linux/usb/functionfs.h
> > > > b/include/uapi/linux/usb/functionfs.h
> > > > index d77ee6b65328..1412ab9f8ccc 100644
> > > > --- a/include/uapi/linux/usb/functionfs.h
> > > > +++ b/include/uapi/linux/usb/functionfs.h
> > > > @@ -84,6 +84,15 @@ struct usb_ext_prop_desc {
> > > >         __le16  wPropertyNameLength;
> > > >   } __attribute__((packed));
> > > >   
> > > > +/* Flags for usb_ffs_dmabuf_transfer_req->flags (none for now)
> > > > */
> > > > +#define USB_FFS_DMABUF_TRANSFER_MASK   0x0
> > > > +
> > > > +struct usb_ffs_dmabuf_transfer_req {
> > > > +       int fd;
> > > > +       __u32 flags;
> > > > +       __u64 length;
> > > > +} __attribute__((packed));
> > > > +
> > > >   #ifndef __KERNEL__
> > > >   
> > > >   /*
> > > > @@ -288,6 +297,9 @@ struct usb_functionfs_event {
> > > >   #define       FUNCTIONFS_ENDPOINT_DESC        _IOR('g', 130,
> > > > \
> > > >                                              struct
> > > > usb_endpoint_descriptor)
> > > >   
> > > > -
> > > > +#define FUNCTIONFS_DMABUF_ATTACH       _IOW('g', 131, int)
> > > > +#define FUNCTIONFS_DMABUF_DETACH       _IOW('g', 132, int)
> > > > +#define FUNCTIONFS_DMABUF_TRANSFER     _IOW('g', 133, \
> > > > +                                            struct
> > > > usb_ffs_dmabuf_transfer_req)
> > > >   
> > > >   #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
> > 

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[kernel test robot]

Hi Paul,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.3-rc2 next-20230314]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230314105257.17345-3-paul%40crapouillou.net
patch subject: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230314/202303142140.ZQsw4C4V-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
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/intel-lab-lkp/linux/commit/4ee364ed5d112c4550344fd037f4e1ef7cc41878
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
        git checkout 4ee364ed5d112c4550344fd037f4e1ef7cc41878
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/usb/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303142140.ZQsw4C4V-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/usb/gadget/function/f_fs.c:1401:5: warning: no previous prototype for 'ffs_dma_resv_lock' [-Wmissing-prototypes]
    1401 | int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
         |     ^~~~~~~~~~~~~~~~~


vim +/ffs_dma_resv_lock +1401 drivers/usb/gadget/function/f_fs.c

  1400	
> 1401	int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
  1402	{
  1403		int ret;
  1404	
  1405		ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
  1406		if (ret) {
  1407			if (ret != -EDEADLK)
  1408				goto out;
  1409			if (nonblock) {
  1410				ret = -EBUSY;
  1411				goto out;
  1412			}
  1413	
  1414			ret = dma_resv_lock_slow_interruptible(dmabuf->resv, NULL);
  1415		}
  1416	
  1417	out:
  1418		return ret;
  1419	}
  1420	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[Paul Cercueil]

Le mardi 14 mars 2023 à 21:41 +0800, kernel test robot a écrit :
> Hi Paul,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on usb/usb-next usb/usb-linus linus/master
> v6.3-rc2 next-20230314]
> [If your patch is applied to the wrong git tree, kindly drop us a
> note.
> And when submitting patch, we suggest to use '--base' as documented
> in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
> base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-
> testing
> patch link:   
> https://lore.kernel.org/r/20230314105257.17345-3-paul%40crapouillou.net
> patch subject: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import
> interface
> config: m68k-allyesconfig
> (https://download.01.org/0day-ci/archive/20230314/202303142140.ZQsw4C
> 4V-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 12.1.0
> 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/intel-lab-lkp/linux/commit/4ee364ed5d112c4550344fd037f4e1ef7cc41878
>         git remote add linux-review
> https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Paul-Cercueil/usb-gadget-
> Support-already-mapped-DMA-SGs/20230314-185522
>         git checkout 4ee364ed5d112c4550344fd037f4e1ef7cc41878
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> make.cross W=1 O=build_dir ARCH=m68k olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/usb/
> 
> If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > Link:
> > https://lore.kernel.org/oe-kbuild-all/202303142140.ZQsw4C4V-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> > > drivers/usb/gadget/function/f_fs.c:1401:5: warning: no previous
> > > prototype for 'ffs_dma_resv_lock' [-Wmissing-prototypes]
>     1401 | int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool
> nonblock)
>          |     ^~~~~~~~~~~~~~~~~

Indeed, missing a "static" here. Thank you Robot.

Cheers,
-Paul

> 
> 
> vim +/ffs_dma_resv_lock +1401 drivers/usb/gadget/function/f_fs.c
> 
>   1400  
> > 1401    int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool
> > nonblock)
>   1402  {
>   1403          int ret;
>   1404  
>   1405          ret = dma_resv_lock_interruptible(dmabuf->resv,
> NULL);
>   1406          if (ret) {
>   1407                  if (ret != -EDEADLK)
>   1408                          goto out;
>   1409                  if (nonblock) {
>   1410                          ret = -EBUSY;
>   1411                          goto out;
>   1412                  }
>   1413  
>   1414                  ret =
> dma_resv_lock_slow_interruptible(dmabuf->resv, NULL);
>   1415          }
>   1416  
>   1417  out:
>   1418          return ret;
>   1419  }
>   1420  
> 

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[kernel test robot]

Hi Paul,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.3-rc2 next-20230314]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230314105257.17345-3-paul%40crapouillou.net
patch subject: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface
config: arm-defconfig (https://download.01.org/0day-ci/archive/20230315/202303151116.lxbyxGM6-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
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/intel-lab-lkp/linux/commit/4ee364ed5d112c4550344fd037f4e1ef7cc41878
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
        git checkout 4ee364ed5d112c4550344fd037f4e1ef7cc41878
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303151116.lxbyxGM6-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_attach from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_map_attachment from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_unmap_attachment from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_get from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_put from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_detach from namespace DMA_BUF, but does not import it.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[kernel test robot]

Hi Paul,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.3-rc2 next-20230315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230314105257.17345-3-paul%40crapouillou.net
patch subject: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface
config: s390-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230315/202303151639.4xUPfzJD-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/4ee364ed5d112c4550344fd037f4e1ef7cc41878
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
        git checkout 4ee364ed5d112c4550344fd037f4e1ef7cc41878
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/media/platform/mediatek/jpeg/ drivers/usb/gadget/function/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303151639.4xUPfzJD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/usb/gadget/function/f_fs.c:18:
   In file included from include/linux/dma-buf.h:16:
   In file included from include/linux/iosys-map.h:10:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/usb/gadget/function/f_fs.c:18:
   In file included from include/linux/dma-buf.h:16:
   In file included from include/linux/iosys-map.h:10:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/usb/gadget/function/f_fs.c:18:
   In file included from include/linux/dma-buf.h:16:
   In file included from include/linux/iosys-map.h:10:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/usb/gadget/function/f_fs.c:1401:5: warning: no previous prototype for function 'ffs_dma_resv_lock' [-Wmissing-prototypes]
   int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
       ^
   drivers/usb/gadget/function/f_fs.c:1401:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
   ^
   static 
   13 warnings generated.


vim +/ffs_dma_resv_lock +1401 drivers/usb/gadget/function/f_fs.c

  1400	
> 1401	int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
  1402	{
  1403		int ret;
  1404	
  1405		ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
  1406		if (ret) {
  1407			if (ret != -EDEADLK)
  1408				goto out;
  1409			if (nonblock) {
  1410				ret = -EBUSY;
  1411				goto out;
  1412			}
  1413	
  1414			ret = dma_resv_lock_slow_interruptible(dmabuf->resv, NULL);
  1415		}
  1416	
  1417	out:
  1418		return ret;
  1419	}
  1420	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface

[kernel test robot]

Hi Paul,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.3-rc2 next-20230315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230314105257.17345-3-paul%40crapouillou.net
patch subject: [PATCH 2/2] usb: gadget: functionfs: Add DMABUF import interface
config: i386-randconfig-a014-20230313 (https://download.01.org/0day-ci/archive/20230316/202303160514.ZHer8I6t-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
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/intel-lab-lkp/linux/commit/4ee364ed5d112c4550344fd037f4e1ef7cc41878
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paul-Cercueil/usb-gadget-Support-already-mapped-DMA-SGs/20230314-185522
        git checkout 4ee364ed5d112c4550344fd037f4e1ef7cc41878
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303160514.ZHer8I6t-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_get from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_attach from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_detach from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_put from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_map_attachment from namespace DMA_BUF, but does not import it.
>> ERROR: modpost: module usb_f_fs uses symbol dma_buf_unmap_attachment from namespace DMA_BUF, but does not import it.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 0/2] usb: gadget: functionfs: DMABUF import interface

[Christian König]

Am 14.03.23 um 11:52 schrieb Paul Cercueil:
> Hi,
>
> This small patchset adds three new IOCTLs that can be used to attach,
> detach, or transfer from/to a DMABUF object.
>
> This was surprisingly easy to add, as the functionfs code only uses
> scatterlists for transfers and allows specifying the number of bytes to
> transfer. The bulk of the code is then for general DMABUF accounting.
>
> The patchset isn't tagged RFC but comments are very welcome, there are
> some things I am not 100% sure about: ffs_dma_resv_lock (with no
> ww_acquire_ctx),

That is perfectly fine as long as you only want to lock one dma_resv object.

I've made a few notes what should be fixed and could potentially be 
improved, but from the DMA-buf side that looks like it should mostly work.

Regards,
Christian.

>   and I'm using pr_debug which feels wrong. Also, I
> should probably add documentation? The current IOCTLs for functionfs
> were not documented, as far as I can tell.
>
> We use it with DMABUFs created with udmabuf, that we attach to the
> functionfs interface and to IIO devices (with a DMABUF interface for
> IIO, on its way to upstream too), to transfer samples from high-speed
> transceivers to USB in a zero-copy fashion.
>
> Cheers,
> -Paul
>
>
> Paul Cercueil (2):
>    usb: gadget: Support already-mapped DMA SGs
>    usb: gadget: functionfs: Add DMABUF import interface
>
>   drivers/usb/gadget/function/f_fs.c  | 398 ++++++++++++++++++++++++++++
>   drivers/usb/gadget/udc/core.c       |   7 +-
>   include/linux/usb/gadget.h          |   2 +
>   include/uapi/linux/usb/functionfs.h |  14 +-
>   4 files changed, 419 insertions(+), 2 deletions(-)
>