[PATCH v3 07/12] soc: aspeed: xdma: Add user interface
Eddie James
eajames at linux.ibm.com
Fri Dec 20 03:00:30 AEDT 2019
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 at 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
More information about the Linux-aspeed
mailing list