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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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__ */
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(-)
>
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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
> [email protected]/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 <[email protected]>
> > Link:
> > https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> 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
>
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> > > > ---
> > > > 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__ */
> >