lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 28 Mar 2022 20:16:13 +0100 From: Paul Cercueil <paul@...pouillou.net> To: Jonathan Cameron <jic23@...nel.org> Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, Christian König <christian.koenig@....com>, Sumit Semwal <sumit.semwal@...aro.org>, Jonathan Corbet <corbet@....net>, Alexandru Ardelean <ardeleanalex@...il.com>, dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org Subject: Re: [PATCH v2 07/12] iio: buffer-dma: Use DMABUFs instead of custom solution Hi Jonathan, Le lun., mars 28 2022 at 18:54:25 +0100, Jonathan Cameron <jic23@...nel.org> a écrit : > On Mon, 7 Feb 2022 12:59:28 +0000 > Paul Cercueil <paul@...pouillou.net> wrote: > >> Enhance the current fileio code by using DMABUF objects instead of >> custom buffers. >> >> This adds more code than it removes, but: >> - a lot of the complexity can be dropped, e.g. custom kref and >> iio_buffer_block_put_atomic() are not needed anymore; >> - it will be much easier to introduce an API to export these DMABUF >> objects to userspace in a following patch. >> >> Signed-off-by: Paul Cercueil <paul@...pouillou.net> > Hi Paul, > > I'm a bit rusty on dma mappings, but you seem to have > a mixture of streaming and coherent mappings going on in here. That's OK, so am I. What do you call "streaming mappings"? > Is it the case that the current code is using the coherent mappings > and a potential 'other user' of the dma buffer might need > streaming mappings? Something like that. There are two different things; on both cases, userspace needs to create a DMABUF with IIO_BUFFER_DMABUF_ALLOC_IOCTL, and the backing memory is allocated with dma_alloc_coherent(). - For the userspace interface, you then have a "cpu access" IOCTL (DMA_BUF_IOCTL_SYNC), that allows userspace to inform when it will start/finish to process the buffer in user-space (which will sync/invalidate the data cache if needed). A buffer can then be enqueued for DMA processing (TX or RX) with the new IIO_BUFFER_DMABUF_ENQUEUE_IOCTL. - When the DMABUF created via the IIO core is sent to another driver through the driver's custom DMABUF import function, this driver will call dma_buf_attach(), which will call iio_buffer_dma_buf_map(). Since it has to return a "struct sg_table *", this function then simply creates a sgtable with one entry that points to the backing memory. Note that I added the iio_buffer_dma_buf_map() / _unmap() functions because the dma-buf core would WARN() if these were not provided. But since this code doesn't yet support importing/exporting DMABUFs to other drivers, these are never called, and I should probably just make them return a ERR_PTR() unconditionally. Cheers, -Paul > Jonathan > >> --- >> drivers/iio/buffer/industrialio-buffer-dma.c | 192 >> ++++++++++++------- >> include/linux/iio/buffer-dma.h | 8 +- >> 2 files changed, 122 insertions(+), 78 deletions(-) >> >> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c >> b/drivers/iio/buffer/industrialio-buffer-dma.c >> index 15ea7bc3ac08..54e6000cd2ee 100644 >> --- a/drivers/iio/buffer/industrialio-buffer-dma.c >> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c >> @@ -14,6 +14,7 @@ >> #include <linux/poll.h> >> #include <linux/iio/buffer_impl.h> >> #include <linux/iio/buffer-dma.h> >> +#include <linux/dma-buf.h> >> #include <linux/dma-mapping.h> >> #include <linux/sizes.h> >> >> @@ -90,103 +91,145 @@ >> * callback is called from within the custom callback. >> */ >> >> -static void iio_buffer_block_release(struct kref *kref) >> -{ >> - struct iio_dma_buffer_block *block = container_of(kref, >> - struct iio_dma_buffer_block, kref); >> - >> - WARN_ON(block->state != IIO_BLOCK_STATE_DEAD); >> - >> - dma_free_coherent(block->queue->dev, PAGE_ALIGN(block->size), >> - block->vaddr, block->phys_addr); >> - >> - iio_buffer_put(&block->queue->buffer); >> - kfree(block); >> -} >> - >> -static void iio_buffer_block_get(struct iio_dma_buffer_block >> *block) >> -{ >> - kref_get(&block->kref); >> -} >> - >> -static void iio_buffer_block_put(struct iio_dma_buffer_block >> *block) >> -{ >> - kref_put(&block->kref, iio_buffer_block_release); >> -} >> - >> -/* >> - * dma_free_coherent can sleep, hence we need to take some special >> care to be >> - * able to drop a reference from an atomic context. >> - */ >> -static LIST_HEAD(iio_dma_buffer_dead_blocks); >> -static DEFINE_SPINLOCK(iio_dma_buffer_dead_blocks_lock); >> - >> -static void iio_dma_buffer_cleanup_worker(struct work_struct *work) >> -{ >> - struct iio_dma_buffer_block *block, *_block; >> - LIST_HEAD(block_list); >> - >> - spin_lock_irq(&iio_dma_buffer_dead_blocks_lock); >> - list_splice_tail_init(&iio_dma_buffer_dead_blocks, &block_list); >> - spin_unlock_irq(&iio_dma_buffer_dead_blocks_lock); >> - >> - list_for_each_entry_safe(block, _block, &block_list, head) >> - iio_buffer_block_release(&block->kref); >> -} >> -static DECLARE_WORK(iio_dma_buffer_cleanup_work, >> iio_dma_buffer_cleanup_worker); >> - >> -static void iio_buffer_block_release_atomic(struct kref *kref) >> -{ >> +struct iio_buffer_dma_buf_attachment { >> + struct scatterlist sgl; >> + struct sg_table sg_table; >> struct iio_dma_buffer_block *block; >> - unsigned long flags; >> - >> - block = container_of(kref, struct iio_dma_buffer_block, kref); >> - >> - spin_lock_irqsave(&iio_dma_buffer_dead_blocks_lock, flags); >> - list_add_tail(&block->head, &iio_dma_buffer_dead_blocks); >> - spin_unlock_irqrestore(&iio_dma_buffer_dead_blocks_lock, flags); >> - >> - schedule_work(&iio_dma_buffer_cleanup_work); >> -} >> - >> -/* >> - * Version of iio_buffer_block_put() that can be called from >> atomic context >> - */ >> -static void iio_buffer_block_put_atomic(struct >> iio_dma_buffer_block *block) >> -{ >> - kref_put(&block->kref, iio_buffer_block_release_atomic); >> -} >> +}; >> >> static struct iio_dma_buffer_queue *iio_buffer_to_queue(struct >> iio_buffer *buf) >> { >> return container_of(buf, struct iio_dma_buffer_queue, buffer); >> } >> >> +static struct iio_buffer_dma_buf_attachment * >> +to_iio_buffer_dma_buf_attachment(struct sg_table *table) >> +{ >> + return container_of(table, struct iio_buffer_dma_buf_attachment, >> sg_table); >> +} >> + >> +static void iio_buffer_block_get(struct iio_dma_buffer_block >> *block) >> +{ >> + get_dma_buf(block->dmabuf); >> +} >> + >> +static void iio_buffer_block_put(struct iio_dma_buffer_block >> *block) >> +{ >> + dma_buf_put(block->dmabuf); >> +} >> + >> +static int iio_buffer_dma_buf_attach(struct dma_buf *dbuf, >> + struct dma_buf_attachment *at) >> +{ >> + at->priv = dbuf->priv; >> + >> + return 0; >> +} >> + >> +static struct sg_table *iio_buffer_dma_buf_map(struct >> dma_buf_attachment *at, >> + enum dma_data_direction dma_dir) >> +{ >> + struct iio_dma_buffer_block *block = at->priv; >> + struct iio_buffer_dma_buf_attachment *dba; >> + int ret; >> + >> + dba = kzalloc(sizeof(*dba), GFP_KERNEL); >> + if (!dba) >> + return ERR_PTR(-ENOMEM); >> + >> + sg_init_one(&dba->sgl, block->vaddr, PAGE_ALIGN(block->size)); >> + dba->sg_table.sgl = &dba->sgl; >> + dba->sg_table.nents = 1; >> + dba->block = block; >> + >> + ret = dma_map_sgtable(at->dev, &dba->sg_table, dma_dir, 0); >> + if (ret) { >> + kfree(dba); >> + return ERR_PTR(ret); >> + } >> + >> + return &dba->sg_table; >> +} >> + >> +static void iio_buffer_dma_buf_unmap(struct dma_buf_attachment *at, >> + struct sg_table *sg_table, >> + enum dma_data_direction dma_dir) >> +{ >> + struct iio_buffer_dma_buf_attachment *dba = >> + to_iio_buffer_dma_buf_attachment(sg_table); >> + >> + dma_unmap_sgtable(at->dev, &dba->sg_table, dma_dir, 0); >> + kfree(dba); >> +} >> + >> +static void iio_buffer_dma_buf_release(struct dma_buf *dbuf) >> +{ >> + struct iio_dma_buffer_block *block = dbuf->priv; >> + struct iio_dma_buffer_queue *queue = block->queue; >> + >> + WARN_ON(block->state != IIO_BLOCK_STATE_DEAD); >> + >> + mutex_lock(&queue->lock); >> + >> + dma_free_coherent(queue->dev, PAGE_ALIGN(block->size), >> + block->vaddr, block->phys_addr); >> + kfree(block); >> + >> + mutex_unlock(&queue->lock); >> + iio_buffer_put(&queue->buffer); >> +} >> + >> +static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { >> + .attach = iio_buffer_dma_buf_attach, >> + .map_dma_buf = iio_buffer_dma_buf_map, >> + .unmap_dma_buf = iio_buffer_dma_buf_unmap, >> + .release = iio_buffer_dma_buf_release, >> +}; >> + >> static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( >> struct iio_dma_buffer_queue *queue, size_t size) >> { >> struct iio_dma_buffer_block *block; >> + DEFINE_DMA_BUF_EXPORT_INFO(einfo); >> + struct dma_buf *dmabuf; >> + int err = -ENOMEM; >> >> block = kzalloc(sizeof(*block), GFP_KERNEL); >> if (!block) >> - return NULL; >> + return ERR_PTR(err); >> >> block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size), >> &block->phys_addr, GFP_KERNEL); >> - if (!block->vaddr) { >> - kfree(block); >> - return NULL; >> + if (!block->vaddr) >> + goto err_free_block; >> + >> + einfo.ops = &iio_dma_buffer_dmabuf_ops; >> + einfo.size = PAGE_ALIGN(size); >> + einfo.priv = block; >> + einfo.flags = O_RDWR; >> + >> + dmabuf = dma_buf_export(&einfo); >> + if (IS_ERR(dmabuf)) { >> + err = PTR_ERR(dmabuf); >> + goto err_free_dma; >> } >> >> + block->dmabuf = dmabuf; >> block->size = size; >> block->state = IIO_BLOCK_STATE_DONE; >> block->queue = queue; >> INIT_LIST_HEAD(&block->head); >> - kref_init(&block->kref); >> >> iio_buffer_get(&queue->buffer); >> >> return block; >> + >> +err_free_dma: >> + dma_free_coherent(queue->dev, PAGE_ALIGN(size), >> + block->vaddr, block->phys_addr); >> +err_free_block: >> + kfree(block); >> + return ERR_PTR(err); >> } >> >> static void _iio_dma_buffer_block_done(struct iio_dma_buffer_block >> *block) >> @@ -223,7 +266,7 @@ void iio_dma_buffer_block_done(struct >> iio_dma_buffer_block *block) >> _iio_dma_buffer_block_done(block); >> spin_unlock_irqrestore(&queue->list_lock, flags); >> >> - iio_buffer_block_put_atomic(block); >> + iio_buffer_block_put(block); >> iio_dma_buffer_queue_wake(queue); >> } >> EXPORT_SYMBOL_GPL(iio_dma_buffer_block_done); >> @@ -249,7 +292,8 @@ void iio_dma_buffer_block_list_abort(struct >> iio_dma_buffer_queue *queue, >> list_del(&block->head); >> block->bytes_used = 0; >> _iio_dma_buffer_block_done(block); >> - iio_buffer_block_put_atomic(block); >> + >> + iio_buffer_block_put(block); >> } >> spin_unlock_irqrestore(&queue->list_lock, flags); >> >> @@ -340,8 +384,8 @@ int iio_dma_buffer_request_update(struct >> iio_buffer *buffer) >> >> if (!block) { >> block = iio_dma_buffer_alloc_block(queue, size); >> - if (!block) { >> - ret = -ENOMEM; >> + if (IS_ERR(block)) { >> + ret = PTR_ERR(block); >> goto out_unlock; >> } >> queue->fileio.blocks[i] = block; >> diff --git a/include/linux/iio/buffer-dma.h >> b/include/linux/iio/buffer-dma.h >> index 490b93f76fa8..6b3fa7d2124b 100644 >> --- a/include/linux/iio/buffer-dma.h >> +++ b/include/linux/iio/buffer-dma.h >> @@ -8,7 +8,6 @@ >> #define __INDUSTRIALIO_DMA_BUFFER_H__ >> >> #include <linux/list.h> >> -#include <linux/kref.h> >> #include <linux/spinlock.h> >> #include <linux/mutex.h> >> #include <linux/iio/buffer_impl.h> >> @@ -16,6 +15,7 @@ >> struct iio_dma_buffer_queue; >> struct iio_dma_buffer_ops; >> struct device; >> +struct dma_buf; >> >> /** >> * enum iio_block_state - State of a struct iio_dma_buffer_block >> @@ -39,8 +39,8 @@ enum iio_block_state { >> * @vaddr: Virutal address of the blocks memory >> * @phys_addr: Physical address of the blocks memory >> * @queue: Parent DMA buffer queue >> - * @kref: kref used to manage the lifetime of block >> * @state: Current state of the block >> + * @dmabuf: Underlying DMABUF object >> */ >> struct iio_dma_buffer_block { >> /* May only be accessed by the owner of the block */ >> @@ -56,13 +56,13 @@ struct iio_dma_buffer_block { >> size_t size; >> struct iio_dma_buffer_queue *queue; >> >> - /* Must not be accessed outside the core. */ >> - struct kref kref; >> /* >> * Must not be accessed outside the core. Access needs to hold >> * queue->list_lock if the block is not owned by the core. >> */ >> enum iio_block_state state; >> + >> + struct dma_buf *dmabuf; >> }; >> >> /** >
Powered by blists - more mailing lists