Looks good to me.
Tested-by: Fernando Guzman Lugo <fernando.lugo@xxxxxx>
Regards,
Fernando.
On Sun, May 20, 2012 at 7:00 AM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote:
>
> Dynamically allocate the vrings' DMA when the remote processor
> is about to be powered on (i.e. when ->find_vqs() is invoked),
> and release them as soon as it is powered off (i.e. when ->del_vqs()
> is invoked).
>
> The obvious and immediate benefit is better memory utilization, since
> memory for the vrings is now only allocated when the relevant remote
> processor is being used.
>
> Additionally, this approach also makes recovery of a (crashing)
> remote processor easier: one just needs to remove the relevant
> vdevs, and the entire vrings cleanup takes place automagically.
>
> Tested-by: Fernando Guzman Lugo <fernando.lugo@xxxxxx>
> Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> ---
> drivers/remoteproc/remoteproc_core.c | 109
> +++++++++++++++---------------
> drivers/remoteproc/remoteproc_internal.h | 2 +
> drivers/remoteproc/remoteproc_virtio.c | 13 +++-
> 3 files changed, 67 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index e756a0d..40e2b2d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -279,34 +279,17 @@ rproc_load_segments(struct rproc *rproc, const u8
> *elf_data, size_t len)
> return ret;
> }
>
> -static int
> -__rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc,
> int i)
> +int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
> {
> struct rproc *rproc = rvdev->rproc;
> struct device *dev = rproc->dev;
> - struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
> + struct rproc_vring *rvring = &rvdev->vring[i];
> dma_addr_t dma;
> void *va;
> int ret, size, notifyid;
>
> - dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
> - i, vring->da, vring->num, vring->align);
> -
> - /* make sure reserved bytes are zeroes */
> - if (vring->reserved) {
> - dev_err(dev, "vring rsc has non zero reserved bytes\n");
> - return -EINVAL;
> - }
> -
> - /* verify queue size and vring alignment are sane */
> - if (!vring->num || !vring->align) {
> - dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
> - vring->num, vring->align);
> - return -EINVAL;
> - }
> -
> /* actual size of vring (in bytes) */
> - size = PAGE_ALIGN(vring_size(vring->num, vring->align));
> + size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>
> if (!idr_pre_get(&rproc->notifyids, GFP_KERNEL)) {
> dev_err(dev, "idr_pre_get failed\n");
> @@ -316,6 +299,7 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct
> fw_rsc_vdev *rsc, int i)
> /*
> * Allocate non-cacheable memory for the vring. In the future
> * this call will also configure the IOMMU for us
> + * TODO: let the rproc know the da of this vring
> */
> va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
> if (!va) {
> @@ -323,44 +307,67 @@ __rproc_handle_vring(struct rproc_vdev *rvdev,
> struct fw_rsc_vdev *rsc, int i)
> return -EINVAL;
> }
>
> - /* assign an rproc-wide unique index for this vring */
> - /* TODO: assign a notifyid for rvdev updates as well */
> - ret = idr_get_new(&rproc->notifyids, &rvdev->vring[i], ¬ifyid);
> + /*
> + * Assign an rproc-wide unique index for this vring
> + * TODO: assign a notifyid for rvdev updates as well
> + * TODO: let the rproc know the notifyid of this vring
> + * TODO: support predefined notifyids (via resource table)
> + */
> + ret = idr_get_new(&rproc->notifyids, rvring, ¬ifyid);
> if (ret) {
> dev_err(dev, "idr_get_new failed: %d\n", ret);
> dma_free_coherent(dev, size, va, dma);
> return ret;
> }
>
> - /* let the rproc know the da and notifyid of this vring */
> - /* TODO: expose this to remote processor */
> - vring->da = dma;
> - vring->notifyid = notifyid;
> -
> dev_dbg(dev, "vring%d: va %p dma %x size %x idr %d\n", i, va,
> dma, size, notifyid);
>
> - rvdev->vring[i].len = vring->num;
> - rvdev->vring[i].align = vring->align;
> - rvdev->vring[i].va = va;
> - rvdev->vring[i].dma = dma;
> - rvdev->vring[i].notifyid = notifyid;
> - rvdev->vring[i].rvdev = rvdev;
> + rvring->va = va;
> + rvring->dma = dma;
> + rvring->notifyid = notifyid;
>
> return 0;
> }
>
> -static void __rproc_free_vrings(struct rproc_vdev *rvdev, int i)
> +static int
> +rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int
> i)
> {
> struct rproc *rproc = rvdev->rproc;
> + struct device *dev = rproc->dev;
> + struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
> + struct rproc_vring *rvring = &rvdev->vring[i];
>
> - for (i--; i >= 0; i--) {
> - struct rproc_vring *rvring = &rvdev->vring[i];
> - int size = PAGE_ALIGN(vring_size(rvring->len,
> rvring->align));
> + dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
> + i, vring->da, vring->num, vring->align);
> +
> + /* make sure reserved bytes are zeroes */
> + if (vring->reserved) {
> + dev_err(dev, "vring rsc has non zero reserved bytes\n");
> + return -EINVAL;
> + }
>
> - dma_free_coherent(rproc->dev, size, rvring->va,
> rvring->dma);
> - idr_remove(&rproc->notifyids, rvring->notifyid);
> + /* verify queue size and vring alignment are sane */
> + if (!vring->num || !vring->align) {
> + dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
> + vring->num, vring->align);
> + return -EINVAL;
> }
> +
> + rvring->len = vring->num;
> + rvring->align = vring->align;
> + rvring->rvdev = rvdev;
> +
> + return 0;
> +}
> +
> +void rproc_free_vring(struct rproc_vring *rvring)
> +{
> + int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
> + struct rproc *rproc = rvring->rvdev->rproc;
> +
> + dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
> + idr_remove(&rproc->notifyids, rvring->notifyid);
> }
>
> /**
> @@ -425,11 +432,11 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
>
> rvdev->rproc = rproc;
>
> - /* allocate the vrings */
> + /* parse the vrings */
> for (i = 0; i < rsc->num_of_vrings; i++) {
> - ret = __rproc_handle_vring(rvdev, rsc, i);
> + ret = rproc_parse_vring(rvdev, rsc, i);
> if (ret)
> - goto free_vrings;
> + goto free_rvdev;
> }
>
> /* remember the device features */
> @@ -440,12 +447,11 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> /* it is now safe to add the virtio device */
> ret = rproc_add_virtio_dev(rvdev, rsc->id);
> if (ret)
> - goto free_vrings;
> + goto free_rvdev;
>
> return 0;
>
> -free_vrings:
> - __rproc_free_vrings(rvdev, i);
> +free_rvdev:
> kfree(rvdev);
> return ret;
> }
> @@ -1265,18 +1271,11 @@ EXPORT_SYMBOL(rproc_shutdown);
> void rproc_release(struct kref *kref)
> {
> struct rproc *rproc = container_of(kref, struct rproc, refcount);
> - struct rproc_vdev *rvdev, *rvtmp;
>
> dev_info(rproc->dev, "removing %s\n", rproc->name);
>
> rproc_delete_debug_dir(rproc);
>
> - /* clean up remote vdev entries */
> - list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node) {
> - __rproc_free_vrings(rvdev, RVDEV_NUM_VRINGS);
> - list_del(&rvdev->node);
> - }
> -
> /*
> * At this point no one holds a reference to rproc anymore,
> * so we can directly unroll rproc_alloc()
> @@ -1547,7 +1546,7 @@ EXPORT_SYMBOL(rproc_free);
> */
> int rproc_unregister(struct rproc *rproc)
> {
> - struct rproc_vdev *rvdev;
> + struct rproc_vdev *rvdev, *tmp;
>
> if (!rproc)
> return -EINVAL;
> @@ -1556,7 +1555,7 @@ int rproc_unregister(struct rproc *rproc)
> wait_for_completion(&rproc->firmware_loading_complete);
>
> /* clean up remote vdev entries */
> - list_for_each_entry(rvdev, &rproc->rvdevs, node)
> + list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
> rproc_remove_virtio_dev(rvdev);
>
> /* the rproc is downref'ed as soon as it's removed from the klist
> */
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index 9f336d6..f4957cf 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -41,4 +41,6 @@ void rproc_create_debug_dir(struct rproc *rproc);
> void rproc_init_debugfs(void);
> void rproc_exit_debugfs(void);
>
> +void rproc_free_vring(struct rproc_vring *rvring);
> +int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> index ecf6121..26a7144 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -77,14 +77,17 @@ static struct virtqueue *rp_find_vq(struct
> virtio_device *vdev,
> struct rproc_vring *rvring;
> struct virtqueue *vq;
> void *addr;
> - int len, size;
> + int len, size, ret;
>
> /* we're temporarily limited to two virtqueues per rvdev */
> if (id >= ARRAY_SIZE(rvdev->vring))
> return ERR_PTR(-EINVAL);
>
> - rvring = &rvdev->vring[id];
> + ret = rproc_alloc_vring(rvdev, id);
> + if (ret)
> + return ERR_PTR(ret);
>
> + rvring = &rvdev->vring[id];
> addr = rvring->va;
> len = rvring->len;
>
> @@ -103,6 +106,7 @@ static struct virtqueue *rp_find_vq(struct
> virtio_device *vdev,
> rproc_virtio_notify, callback,
> name);
> if (!vq) {
> dev_err(rproc->dev, "vring_new_virtqueue %s failed\n",
> name);
> + rproc_free_vring(rvring);
> return ERR_PTR(-ENOMEM);
> }
>
> @@ -125,6 +129,7 @@ static void rproc_virtio_del_vqs(struct virtio_device
> *vdev)
> rvring = vq->priv;
> rvring->vq = NULL;
> vring_del_virtqueue(vq);
> + rproc_free_vring(rvring);
> }
> }
>
> @@ -228,8 +233,12 @@ static struct virtio_config_ops
> rproc_virtio_config_ops = {
> static void rproc_vdev_release(struct device *dev)
> {
> struct virtio_device *vdev = dev_to_virtio(dev);
> + struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> struct rproc *rproc = vdev_to_rproc(vdev);
>
> + list_del(&rvdev->node);
> + kfree(rvdev);
> +
> kref_put(&rproc->refcount, rproc_release);
> }
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
