| Message ID | 20211001101234.4247-2-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: restructure the remoteproc VirtIO device | expand |
Hi, I have started reviewing this set. Comments herein are related to code logic only. I will comment on the overall approach at a later time. On Fri, Oct 01, 2021 at 12:12:28PM +0200, Arnaud Pouliquen wrote: > In preparation of the migration of the management of rvdev in > rproc_virtio, this patch spins off new functions to manage the Are you referring to file remoteproc_virtio.c? If so please clearly state that it is the case by using the real name. Otherwise it is very confusing. > remoteproc virtio device. > > The rproc_rvdev_add_device and rproc_rvdev_remove_device will be > moved to remoteproc_virtio. Here too I have to guess that you mean remoteproc_virtio.c. Moreover two different nomenclatures are used in 3 lines. > > In addition the rproc_register_rvdev and rproc_unregister_rvdev is created > as it will be exported (used in rproc_rvdev_add_device > and rproc_rvdev_remove_device functions). > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > drivers/remoteproc/remoteproc_core.c | 102 ++++++++++++++++++--------- > 1 file changed, 67 insertions(+), 35 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 502b6604b757..7c783ca291a7 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -484,6 +484,69 @@ static int copy_dma_range_map(struct device *to, struct device *from) > return 0; > } > > +static void rproc_register_rvdev(struct rproc_vdev *rvdev) > +{ > + if (rvdev && rvdev->rproc) > + list_add_tail(&rvdev->node, &rvdev->rproc->rvdevs); > +} > + > +static void rproc_unregister_rvdev(struct rproc_vdev *rvdev) > +{ > + if (rvdev) > + list_del(&rvdev->node); > +} This file is a simple refactoring of the current code. Additions such as this one should be done in a separate patch. > + > +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev) > +{ > + struct rproc *rproc = rvdev->rproc; > + char name[16]; > + int ret; > + > + snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); > + rvdev->dev.parent = &rproc->dev; > + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > + if (ret) > + return ret; Memory is allocated for @rvdev in rproc_handle_vdev() using kzalloc(). If we return prematurely that memory will be leaked. Note that this problem is present in the current code base. I suggest sending a separate patch to fix it while this work is ongoing. > + > + rvdev->dev.release = rproc_rvdev_release; > + dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name); > + dev_set_drvdata(&rvdev->dev, rvdev); > + > + ret = device_register(&rvdev->dev); > + if (ret) { > + put_device(&rvdev->dev); > + return ret; > + } > + /* Make device dma capable by inheriting from parent's capabilities */ > + set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent)); > + > + ret = dma_coerce_mask_and_coherent(&rvdev->dev, > + dma_get_mask(rproc->dev.parent)); > + if (ret) { > + dev_warn(&rvdev->dev, > + "Failed to set DMA mask %llx. Trying to continue... %x\n", > + dma_get_mask(rproc->dev.parent), ret); > + } > + > + rproc_register_rvdev(rvdev); > + > + rvdev->subdev.start = rproc_vdev_do_start; > + rvdev->subdev.stop = rproc_vdev_do_stop; > + > + rproc_add_subdev(rproc, &rvdev->subdev); Please see comment above. > + > + return 0; > +} > + > +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) > +{ > + struct rproc *rproc = rvdev->rproc; > + > + rproc_remove_subdev(rproc, &rvdev->subdev); > + rproc_unregister_rvdev(rvdev); > + device_unregister(&rvdev->dev); > +} > + > /** > * rproc_handle_vdev() - handle a vdev fw resource > * @rproc: the remote processor > @@ -519,7 +582,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > struct device *dev = &rproc->dev; > struct rproc_vdev *rvdev; > int i, ret; > - char name[16]; > > /* make sure resource isn't truncated */ > if (struct_size(rsc, vring, rsc->num_of_vrings) + rsc->config_len > > @@ -551,33 +613,13 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > > rvdev->id = rsc->id; > rvdev->rproc = rproc; > - rvdev->index = rproc->nb_vdev++; > + rvdev->index = rproc->nb_vdev; This one may make sense in a later patch but for now it doesn't. Depending on the time I have more comments to come later, tomorrow or on Monday. Thanks, Mathieu > > - /* Initialise vdev subdevice */ > - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); > - rvdev->dev.parent = &rproc->dev; > - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > + ret = rproc_rvdev_add_device(rvdev); > if (ret) > return ret; > - rvdev->dev.release = rproc_rvdev_release; > - dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name); > - dev_set_drvdata(&rvdev->dev, rvdev); > > - ret = device_register(&rvdev->dev); > - if (ret) { > - put_device(&rvdev->dev); > - return ret; > - } > - /* Make device dma capable by inheriting from parent's capabilities */ > - set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent)); > - > - ret = dma_coerce_mask_and_coherent(&rvdev->dev, > - dma_get_mask(rproc->dev.parent)); > - if (ret) { > - dev_warn(dev, > - "Failed to set DMA mask %llx. Trying to continue... %x\n", > - dma_get_mask(rproc->dev.parent), ret); > - } > + rproc->nb_vdev++; > > /* parse the vrings */ > for (i = 0; i < rsc->num_of_vrings; i++) { > @@ -596,13 +638,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > goto unwind_vring_allocations; > } > > - list_add_tail(&rvdev->node, &rproc->rvdevs); > - > - rvdev->subdev.start = rproc_vdev_do_start; > - rvdev->subdev.stop = rproc_vdev_do_stop; > - > - rproc_add_subdev(rproc, &rvdev->subdev); > - > return 0; > > unwind_vring_allocations: > @@ -617,7 +652,6 @@ void rproc_vdev_release(struct kref *ref) > { > struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount); > struct rproc_vring *rvring; > - struct rproc *rproc = rvdev->rproc; > int id; > > for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) { > @@ -625,9 +659,7 @@ void rproc_vdev_release(struct kref *ref) > rproc_free_vring(rvring); > } > > - rproc_remove_subdev(rproc, &rvdev->subdev); > - list_del(&rvdev->node); > - device_unregister(&rvdev->dev); > + rproc_rvdev_remove_device(rvdev); > } > > /** > -- > 2.17.1 >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 502b6604b757..7c783ca291a7 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -484,6 +484,69 @@ static int copy_dma_range_map(struct device *to, struct device *from) return 0; } +static void rproc_register_rvdev(struct rproc_vdev *rvdev) +{ + if (rvdev && rvdev->rproc) + list_add_tail(&rvdev->node, &rvdev->rproc->rvdevs); +} + +static void rproc_unregister_rvdev(struct rproc_vdev *rvdev) +{ + if (rvdev) + list_del(&rvdev->node); +} + +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev) +{ + struct rproc *rproc = rvdev->rproc; + char name[16]; + int ret; + + snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); + rvdev->dev.parent = &rproc->dev; + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); + if (ret) + return ret; + + rvdev->dev.release = rproc_rvdev_release; + dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name); + dev_set_drvdata(&rvdev->dev, rvdev); + + ret = device_register(&rvdev->dev); + if (ret) { + put_device(&rvdev->dev); + return ret; + } + /* Make device dma capable by inheriting from parent's capabilities */ + set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent)); + + ret = dma_coerce_mask_and_coherent(&rvdev->dev, + dma_get_mask(rproc->dev.parent)); + if (ret) { + dev_warn(&rvdev->dev, + "Failed to set DMA mask %llx. Trying to continue... %x\n", + dma_get_mask(rproc->dev.parent), ret); + } + + rproc_register_rvdev(rvdev); + + rvdev->subdev.start = rproc_vdev_do_start; + rvdev->subdev.stop = rproc_vdev_do_stop; + + rproc_add_subdev(rproc, &rvdev->subdev); + + return 0; +} + +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) +{ + struct rproc *rproc = rvdev->rproc; + + rproc_remove_subdev(rproc, &rvdev->subdev); + rproc_unregister_rvdev(rvdev); + device_unregister(&rvdev->dev); +} + /** * rproc_handle_vdev() - handle a vdev fw resource * @rproc: the remote processor @@ -519,7 +582,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, struct device *dev = &rproc->dev; struct rproc_vdev *rvdev; int i, ret; - char name[16]; /* make sure resource isn't truncated */ if (struct_size(rsc, vring, rsc->num_of_vrings) + rsc->config_len > @@ -551,33 +613,13 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, rvdev->id = rsc->id; rvdev->rproc = rproc; - rvdev->index = rproc->nb_vdev++; + rvdev->index = rproc->nb_vdev; - /* Initialise vdev subdevice */ - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); - rvdev->dev.parent = &rproc->dev; - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); + ret = rproc_rvdev_add_device(rvdev); if (ret) return ret; - rvdev->dev.release = rproc_rvdev_release; - dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name); - dev_set_drvdata(&rvdev->dev, rvdev); - ret = device_register(&rvdev->dev); - if (ret) { - put_device(&rvdev->dev); - return ret; - } - /* Make device dma capable by inheriting from parent's capabilities */ - set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent)); - - ret = dma_coerce_mask_and_coherent(&rvdev->dev, - dma_get_mask(rproc->dev.parent)); - if (ret) { - dev_warn(dev, - "Failed to set DMA mask %llx. Trying to continue... %x\n", - dma_get_mask(rproc->dev.parent), ret); - } + rproc->nb_vdev++; /* parse the vrings */ for (i = 0; i < rsc->num_of_vrings; i++) { @@ -596,13 +638,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, goto unwind_vring_allocations; } - list_add_tail(&rvdev->node, &rproc->rvdevs); - - rvdev->subdev.start = rproc_vdev_do_start; - rvdev->subdev.stop = rproc_vdev_do_stop; - - rproc_add_subdev(rproc, &rvdev->subdev); - return 0; unwind_vring_allocations: @@ -617,7 +652,6 @@ void rproc_vdev_release(struct kref *ref) { struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount); struct rproc_vring *rvring; - struct rproc *rproc = rvdev->rproc; int id; for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) { @@ -625,9 +659,7 @@ void rproc_vdev_release(struct kref *ref) rproc_free_vring(rvring); } - rproc_remove_subdev(rproc, &rvdev->subdev); - list_del(&rvdev->node); - device_unregister(&rvdev->dev); + rproc_rvdev_remove_device(rvdev); } /**
In preparation of the migration of the management of rvdev in rproc_virtio, this patch spins off new functions to manage the remoteproc virtio device. The rproc_rvdev_add_device and rproc_rvdev_remove_device will be moved to remoteproc_virtio. In addition the rproc_register_rvdev and rproc_unregister_rvdev is created as it will be exported (used in rproc_rvdev_add_device and rproc_rvdev_remove_device functions). Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- drivers/remoteproc/remoteproc_core.c | 102 ++++++++++++++++++--------- 1 file changed, 67 insertions(+), 35 deletions(-)