On (Tue) 25 Sep 2012 [15:47:16], [email protected] wrote:
> +static DEFINE_SPINLOCK(dma_bufs_lock);
> +static LIST_HEAD(pending_free_dma_bufs);
> +
> static void free_buf(struct port_buffer *buf)
> {
> int i;
> + unsigned long flags;
>
> - kfree(buf->buf);
> + if (!buf->dev)
> + kfree(buf->buf);
Doesn't hurt to just kfree a NULL pointer; so check is not needed.
> - if (buf->sgpages)
> + if (buf->sgpages) {
> for (i = 0; i < buf->sgpages; i++) {
> struct page *page = sg_page(&buf->sg[i]);
> if (!page)
> break;
> put_page(page);
> }
> + return;
> + }
No kfree(buf)?
> + if (buf->dev && is_rproc_enabled) {
> +
> + /* dma_free_coherent requires interrupts to be enabled. */
> + if (irqs_disabled()) {
> + /* queue up dma-buffers to be freed later */
> + spin_lock_irqsave(&dma_bufs_lock, flags);
> + list_add_tail(&buf->list, &pending_free_dma_bufs);
> + spin_unlock_irqrestore(&dma_bufs_lock, flags);
> + return;
> + }
> + dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
> +
> + /* Release device refcnt and allow it to be freed */
> + might_sleep();
> + put_device(buf->dev);
> + }
>
> kfree(buf);
> }
>
> +static void reclaim_dma_bufs(void)
> +{
> + unsigned long flags;
> + struct port_buffer *buf, *tmp;
> + LIST_HEAD(tmp_list);
> +
> + WARN_ON(irqs_disabled());
> + if (list_empty(&pending_free_dma_bufs))
> + return;
> +
> + BUG_ON(!is_rproc_enabled);
> +
> + /* Create a copy of the pending_free_dma_bufs while holding the lock*/
> + spin_lock_irqsave(&dma_bufs_lock, flags);
> + list_cut_position(&tmp_list, &pending_free_dma_bufs,
> + pending_free_dma_bufs.prev);
> + spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +
> + /* Release the dma buffers, without irqs enabled */
> + list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
> + list_del(&buf->list);
> + free_buf(buf);
> + }
> +}
> +
> static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> int nrbufs)
> {
> struct port_buffer *buf;
> size_t alloc_size;
>
> + if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> + reclaim_dma_bufs();
> +
> /* Allocate buffer and the scatter list */
> alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> - buf = kmalloc(alloc_size, GFP_ATOMIC);
> + buf = kzalloc(alloc_size, GFP_ATOMIC);
Why change from kmalloc to kzalloc? It's better to split out such
changes into separate patch for easier review.
> if (!buf)
> goto fail;
>
> @@ -374,11 +444,30 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> if (nrbufs > 0)
> return buf;
>
> - buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> + if (is_rproc_serial(vq->vdev)) {
> + /*
> + * Allocate DMA memory from ancestor. When a virtio
> + * device is created by remoteproc, the DMA memory is
> + * associated with the grandparent device:
> + * vdev => rproc => platform-dev.
> + * The code here would have been less quirky if
> + * DMA_MEMORY_INCLUDES_CHILDREN had been supported
> + * in dma-coherent.c
> + */
> + if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> + goto free_buf;
> + buf->dev = vq->vdev->dev.parent->parent;
> +
> + /* Increase device refcnt to avoid freeing it*/
missing space before */
> + get_device(buf->dev);
> + buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
> + GFP_ATOMIC);
> + } else {
> + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> + }
> +
> if (!buf->buf)
> goto free_buf;
> - buf->len = 0;
> - buf->offset = 0;
As noted in a comment to previous patch, please use kmalloc and
explicit initialisation of variables.
> buf->size = buf_size;
>
> /* Prepare scatter buffer for sending */
> @@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> .u.data = &sgl,
> };
>
> + /* rproc_serial does not support splice */
> + if (is_rproc_serial(port->out_vq->vdev))
> + return -EINVAL;
Will something break if this is not done?
Amit