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@xxxxxxxxxxxxx>
> ---
> 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()?
> + 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.
> +
> + 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?
> + 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.
Andrew
