Messages in this thread | | | Subject | Re: [PATCH v3 07/12] soc: aspeed: xdma: Add user interface | From | Eddie James <> | Date | Thu, 19 Dec 2019 10:00:30 -0600 |
| |
On 12/18/19 7:19 PM, Andrew Jeffery wrote: > > On Thu, 19 Dec 2019, at 01:39, Eddie James wrote: >> This commits adds a miscdevice to provide a user interface to the XDMA >> engine. The interface provides the write operation to start DMA >> operations. The DMA parameters are passed as the data to the write call. >> The actual data to transfer is NOT passed through write. Note that both >> directions of DMA operation are accomplished through the write command; >> BMC to host and host to BMC. >> >> The XDMA driver reserves an area of physical memory for DMA operations, >> as the XDMA engine is restricted to accessing certain physical memory >> areas on some platforms. This memory forms a pool from which users can >> allocate pages for their usage with calls to mmap. The space allocated >> by a client will be the space used in the DMA operation. For an >> "upstream" (BMC to host) operation, the data in the client's area will >> be transferred to the host. For a "downstream" (host to BMC) operation, >> the host data will be placed in the client's memory area. >> >> Poll is also provided in order to determine when the DMA operation is >> complete for non-blocking IO. >> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> Changes since v2: >> - Rework commit message to talk about VGA memory less >> - Remove user reset functionality >> - Clean up sanity checks in aspeed_xdma_write() >> - Wait for transfer complete in the vm area close function >> >> drivers/soc/aspeed/aspeed-xdma.c | 205 ++++++++++++++++++++++++++++++- >> 1 file changed, 203 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/soc/aspeed/aspeed-xdma.c b/drivers/soc/aspeed/aspeed-xdma.c >> index cb94adf798b1..e844937dc925 100644 >> --- a/drivers/soc/aspeed/aspeed-xdma.c >> +++ b/drivers/soc/aspeed/aspeed-xdma.c >> @@ -13,6 +13,7 @@ >> #include <linux/io.h> >> #include <linux/jiffies.h> >> #include <linux/mfd/syscon.h> >> +#include <linux/miscdevice.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> #include <linux/of_device.h> >> @@ -201,6 +202,8 @@ struct aspeed_xdma { >> struct clk *clock; >> struct reset_control *reset; >> >> + /* file_lock serializes reads of current_client */ >> + struct mutex file_lock; > I wonder whether start_lock can serve this purpose. > >> /* client_lock protects error and in_progress of the client */ >> spinlock_t client_lock; >> struct aspeed_xdma_client *current_client; >> @@ -223,6 +226,8 @@ struct aspeed_xdma { >> void __iomem *mem_virt; >> dma_addr_t cmdq_phys; >> struct gen_pool *pool; >> + >> + struct miscdevice misc; >> }; >> >> struct aspeed_xdma_client { >> @@ -522,6 +527,185 @@ static irqreturn_t aspeed_xdma_pcie_irq(int irq, >> void *arg) >> return IRQ_HANDLED; >> } >> >> +static ssize_t aspeed_xdma_write(struct file *file, const char __user *buf, >> + size_t len, loff_t *offset) >> +{ >> + int rc; >> + struct aspeed_xdma_op op; >> + struct aspeed_xdma_client *client = file->private_data; >> + struct aspeed_xdma *ctx = client->ctx; >> + >> + if (len != sizeof(op)) >> + return -EINVAL; >> + >> + rc = copy_from_user(&op, buf, len); >> + if (rc) >> + return rc; >> + >> + if (!op.len || op.len > client->size || >> + op.direction > ASPEED_XDMA_DIRECTION_UPSTREAM) >> + return -EINVAL; >> + >> + if (file->f_flags & O_NONBLOCK) { >> + if (!mutex_trylock(&ctx->file_lock)) >> + return -EAGAIN; >> + >> + if (ctx->current_client) { > Should be tested under client_lock for consistency with the previous patch, > though perhaps you could use READ_ONCE()?
I think READ_ONCE will work.
> >> + mutex_unlock(&ctx->file_lock); >> + return -EBUSY; >> + } >> + } else { >> + mutex_lock(&ctx->file_lock); >> + >> + rc = wait_event_interruptible(ctx->wait, !ctx->current_client); >> + if (rc) { >> + mutex_unlock(&ctx->file_lock); >> + return -EINTR; >> + } >> + } >> + >> + aspeed_xdma_start(ctx, &op, client->phys, client); >> + >> + mutex_unlock(&ctx->file_lock); > Shouldn't we lift start_lock out of aspeed_xdma_start() use that here > instead of file_lock? I think that would mean that we could remove > file_lock.
That wouldn't work with the reset though. The reset should hold start_lock as well, but if a client is waiting here with start_lock, we'd never get to the reset if the transfer doesn't complete. I think file_lock is necessary.
> >> + >> + if (!(file->f_flags & O_NONBLOCK)) { >> + rc = wait_event_interruptible(ctx->wait, !client->in_progress); >> + if (rc) >> + return -EINTR; >> + >> + if (client->error) >> + return -EIO; >> + } >> + >> + return len; >> +} >> + >> +static __poll_t aspeed_xdma_poll(struct file *file, >> + struct poll_table_struct *wait) >> +{ >> + __poll_t mask = 0; >> + __poll_t req = poll_requested_events(wait); >> + struct aspeed_xdma_client *client = file->private_data; >> + struct aspeed_xdma *ctx = client->ctx; >> + >> + if (req & (EPOLLIN | EPOLLRDNORM)) { >> + if (client->in_progress) >> + poll_wait(file, &ctx->wait, wait); >> + >> + if (!client->in_progress) { >> + if (client->error) >> + mask |= EPOLLERR; >> + else >> + mask |= EPOLLIN | EPOLLRDNORM; >> + } >> + } >> + >> + if (req & (EPOLLOUT | EPOLLWRNORM)) { >> + if (ctx->current_client) >> + poll_wait(file, &ctx->wait, wait); >> + >> + if (!ctx->current_client) >> + mask |= EPOLLOUT | EPOLLWRNORM; >> + } >> + >> + return mask; >> +} >> + >> +static void aspeed_xdma_vma_close(struct vm_area_struct *vma) >> +{ >> + int rc; >> + struct aspeed_xdma_client *client = vma->vm_private_data; >> + >> + rc = wait_event_interruptible(client->ctx->wait, !client->in_progress); >> + if (rc) >> + return; >> + >> + gen_pool_free(client->ctx->pool, (unsigned long)client->virt, >> + client->size); >> + >> + client->virt = NULL; >> + client->phys = 0; >> + client->size = 0; >> +} >> + >> +static const struct vm_operations_struct aspeed_xdma_vm_ops = { >> + .close = aspeed_xdma_vma_close, >> +}; >> + >> +static int aspeed_xdma_mmap(struct file *file, struct vm_area_struct *vma) >> +{ >> + int rc; >> + struct aspeed_xdma_client *client = file->private_data; >> + struct aspeed_xdma *ctx = client->ctx; >> + >> + /* restrict file to one mapping */ >> + if (client->size) >> + return -EBUSY; >> + >> + client->size = vma->vm_end - vma->vm_start; >> + client->virt = gen_pool_dma_alloc(ctx->pool, client->size, >> + &client->phys); >> + if (!client->virt) { >> + client->phys = 0; >> + client->size = 0; >> + return -ENOMEM; >> + } >> + >> + vma->vm_pgoff = (client->phys - ctx->mem_phys) >> PAGE_SHIFT; >> + vma->vm_ops = &aspeed_xdma_vm_ops; >> + vma->vm_private_data = client; >> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> + >> + rc = io_remap_pfn_range(vma, vma->vm_start, client->phys >> PAGE_SHIFT, >> + client->size, vma->vm_page_prot); >> + if (rc) { > Probably worth a dev_warn() here so we know what happened?
Sure.
> >> + gen_pool_free(ctx->pool, (unsigned long)client->virt, >> + client->size); >> + >> + client->virt = NULL; >> + client->phys = 0; >> + client->size = 0; >> + return rc; >> + } >> + >> + dev_dbg(ctx->dev, "mmap: v[%08lx] to p[%08x], s[%08x]\n", >> + vma->vm_start, (u32)client->phys, client->size); >> + >> + return 0; >> +} >> + >> +static int aspeed_xdma_open(struct inode *inode, struct file *file) >> +{ >> + struct miscdevice *misc = file->private_data; >> + struct aspeed_xdma *ctx = container_of(misc, struct aspeed_xdma, misc); >> + struct aspeed_xdma_client *client = kzalloc(sizeof(*client), >> + GFP_KERNEL); >> + >> + if (!client) >> + return -ENOMEM; >> + >> + client->ctx = ctx; >> + file->private_data = client; >> + return 0; >> +} >> + >> +static int aspeed_xdma_release(struct inode *inode, struct file *file) >> +{ >> + struct aspeed_xdma_client *client = file->private_data; >> + >> + kfree(client); > I assume the vma gets torn down before release() gets invoked? I haven't > looked closely.
From what I've read, yes, the VMA has to be closed before release() can be called.
Thanks for the review!
Eddie
> > Andrew
| |