lkml.org 
[lkml]   [2021]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH 1/7] remoteproc: core: Introduce virtio device add/remove functions
    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
    >

    \
     
     \ /
      Last update: 2021-10-14 19:50    [W:2.423 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site