| Message ID | 20211222082349.30378-2-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: restructure the remoteproc VirtIO device | expand |
Good morning, On Wed, Dec 22, 2021 at 09:23:44AM +0100, Arnaud Pouliquen wrote: > In preparation of the migration of the management of rvdev in > remoteproc_virtio.c, 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.c. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > update vs previous revision: > - update according to the rebase from v15-rc1 to v16-rc1 > - split patch to introduce rproc_register_rvdev and rproc_unregister_rvdev > function in a separate patch > --- > drivers/remoteproc/remoteproc_core.c | 94 +++++++++++++++++----------- > 1 file changed, 57 insertions(+), 37 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 69f51acf235e..d1f1c5c25bd7 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -484,6 +484,61 @@ static int copy_dma_range_map(struct device *to, struct device *from) > return 0; > } > > +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; > + 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; > + } Registering the device here is a problem... If device_register() fails put_device() is called and we return, only to call device_unregister() on the same device in rproc_handle_vdev(). Moreover in rproc_handle_vdev(), device_unregister() is called in the error path but device_register() is called here in rproc_rvdev_add_device(). This introduces coupling between the two functions, making it hard to maintain from hereon. I suggest calling device_register() in rproc_handle_vdev() after rproc_rvdev_add_device() has returned successfully. More comments to come tomorrow. Thanks, Mathieu > + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > + if (ret) > + goto free_rvdev; > + > + /* 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... (%pe)\n", > + dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); > + } > + > + 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; > + > +free_rvdev: > + device_unregister(&rvdev->dev); > + return ret; > +} > + > +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) > +{ > + struct rproc *rproc = rvdev->rproc; > + > + rproc_remove_subdev(rproc, &rvdev->subdev); > + list_del(&rvdev->node); > + device_unregister(&rvdev->dev); > +} > + > /** > * rproc_handle_vdev() - handle a vdev fw resource > * @rproc: the remote processor > @@ -519,7 +574,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 > > @@ -553,34 +607,10 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > rvdev->rproc = rproc; > rvdev->index = rproc->nb_vdev++; > > - /* Initialise vdev subdevice */ > - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); > - rvdev->dev.parent = &rproc->dev; > - 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; > - } > - > - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > + ret = rproc_rvdev_add_device(rvdev); > if (ret) > goto free_rvdev; > > - /* 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... (%pe)\n", > - dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); > - } > - > /* parse the vrings */ > for (i = 0; i < rsc->num_of_vrings; i++) { > ret = rproc_parse_vring(rvdev, rsc, i); > @@ -598,13 +628,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: > @@ -619,7 +642,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++) { > @@ -627,9 +649,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 >
Hello Mathieu, On 1/4/22 8:08 PM, Mathieu Poirier wrote: > Good morning, > > On Wed, Dec 22, 2021 at 09:23:44AM +0100, Arnaud Pouliquen wrote: >> In preparation of the migration of the management of rvdev in >> remoteproc_virtio.c, 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.c. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> update vs previous revision: >> - update according to the rebase from v15-rc1 to v16-rc1 >> - split patch to introduce rproc_register_rvdev and rproc_unregister_rvdev >> function in a separate patch >> --- >> drivers/remoteproc/remoteproc_core.c | 94 +++++++++++++++++----------- >> 1 file changed, 57 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 69f51acf235e..d1f1c5c25bd7 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -484,6 +484,61 @@ static int copy_dma_range_map(struct device *to, struct device *from) >> return 0; >> } >> >> +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; >> + 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; >> + } > > Registering the device here is a problem... If device_register() fails > put_device() is called and we return, only to call device_unregister() on the > same device in rproc_handle_vdev(). > > Moreover in rproc_handle_vdev(), device_unregister() is called in the error > path but device_register() is called here in rproc_rvdev_add_device(). This > introduces coupling between the two functions, making it hard to maintain from > hereon. Very relevant, I need to rework the error management. > > I suggest calling device_register() in rproc_handle_vdev() after > rproc_rvdev_add_device() has returned successfully. One of the goals of this patchset is to move the device_register in remote_proc_virtio.c Doing this would not go in this direction. I need to test but following could be an alternative: - Call rproc_rvdev_remove_device in rproc_handle_vdev in case of error. - Remove the put_device in rproc_rvdev_add_device. => This would be aligned with patch [6/6] implementation with rproc_virtio_register_device/rproc_virtio_unregister_device... Thanks, Arnaud > > More comments to come tomorrow. > > Thanks, > Mathieu > >> + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); >> + if (ret) >> + goto free_rvdev; >> + >> + /* 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... (%pe)\n", >> + dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); >> + } >> + >> + 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; >> + >> +free_rvdev: >> + device_unregister(&rvdev->dev); >> + return ret; >> +} >> + >> +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) >> +{ >> + struct rproc *rproc = rvdev->rproc; >> + >> + rproc_remove_subdev(rproc, &rvdev->subdev); >> + list_del(&rvdev->node); >> + device_unregister(&rvdev->dev); >> +} >> + >> /** >> * rproc_handle_vdev() - handle a vdev fw resource >> * @rproc: the remote processor >> @@ -519,7 +574,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 > >> @@ -553,34 +607,10 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, >> rvdev->rproc = rproc; >> rvdev->index = rproc->nb_vdev++; >> >> - /* Initialise vdev subdevice */ >> - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); >> - rvdev->dev.parent = &rproc->dev; >> - 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; >> - } >> - >> - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); >> + ret = rproc_rvdev_add_device(rvdev); >> if (ret) >> goto free_rvdev; >> >> - /* 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... (%pe)\n", >> - dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); >> - } >> - >> /* parse the vrings */ >> for (i = 0; i < rsc->num_of_vrings; i++) { >> ret = rproc_parse_vring(rvdev, rsc, i); >> @@ -598,13 +628,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: >> @@ -619,7 +642,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++) { >> @@ -627,9 +649,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 >>
On Wed, Jan 05, 2022 at 09:05:21AM +0100, Arnaud POULIQUEN wrote: > Hello Mathieu, > > On 1/4/22 8:08 PM, Mathieu Poirier wrote: > > Good morning, > > > > On Wed, Dec 22, 2021 at 09:23:44AM +0100, Arnaud Pouliquen wrote: > > > In preparation of the migration of the management of rvdev in > > > remoteproc_virtio.c, 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.c. > > > > > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > > > --- > > > update vs previous revision: > > > - update according to the rebase from v15-rc1 to v16-rc1 > > > - split patch to introduce rproc_register_rvdev and rproc_unregister_rvdev > > > function in a separate patch > > > --- > > > drivers/remoteproc/remoteproc_core.c | 94 +++++++++++++++++----------- > > > 1 file changed, 57 insertions(+), 37 deletions(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index 69f51acf235e..d1f1c5c25bd7 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -484,6 +484,61 @@ static int copy_dma_range_map(struct device *to, struct device *from) > > > return 0; > > > } > > > +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; > > > + 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; > > > + } > > > > Registering the device here is a problem... If device_register() fails > > put_device() is called and we return, only to call device_unregister() on the > > same device in rproc_handle_vdev(). > > > > Moreover in rproc_handle_vdev(), device_unregister() is called in the error > > path but device_register() is called here in rproc_rvdev_add_device(). This > > introduces coupling between the two functions, making it hard to maintain from > > hereon. > > Very relevant, I need to rework the error management. > > > > > I suggest calling device_register() in rproc_handle_vdev() after > > rproc_rvdev_add_device() has returned successfully. > > One of the goals of this patchset is to move the device_register in > remote_proc_virtio.c > Doing this would not go in this direction. > > I need to test but following could be an alternative: > - Call rproc_rvdev_remove_device in rproc_handle_vdev in case of error. > - Remove the put_device in rproc_rvdev_add_device. > Right, some kind of rework is needed. I offered a simple solution but something more involved is likely required. > => This would be aligned with patch [6/6] implementation > with rproc_virtio_register_device/rproc_virtio_unregister_device... > > Thanks, > Arnaud > > > > > More comments to come tomorrow. > > > > Thanks, > > Mathieu > > > > > + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > > > + if (ret) > > > + goto free_rvdev; > > > + > > > + /* 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... (%pe)\n", > > > + dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); > > > + } > > > + > > > + 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; > > > + > > > +free_rvdev: > > > + device_unregister(&rvdev->dev); > > > + return ret; > > > +} > > > + > > > +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) > > > +{ > > > + struct rproc *rproc = rvdev->rproc; > > > + > > > + rproc_remove_subdev(rproc, &rvdev->subdev); > > > + list_del(&rvdev->node); > > > + device_unregister(&rvdev->dev); > > > +} > > > + > > > /** > > > * rproc_handle_vdev() - handle a vdev fw resource > > > * @rproc: the remote processor > > > @@ -519,7 +574,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 > > > > @@ -553,34 +607,10 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > > > rvdev->rproc = rproc; > > > rvdev->index = rproc->nb_vdev++; > > > - /* Initialise vdev subdevice */ > > > - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); > > > - rvdev->dev.parent = &rproc->dev; > > > - 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; > > > - } > > > - > > > - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); > > > + ret = rproc_rvdev_add_device(rvdev); > > > if (ret) > > > goto free_rvdev; > > > - /* 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... (%pe)\n", > > > - dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); > > > - } > > > - > > > /* parse the vrings */ > > > for (i = 0; i < rsc->num_of_vrings; i++) { > > > ret = rproc_parse_vring(rvdev, rsc, i); > > > @@ -598,13 +628,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: > > > @@ -619,7 +642,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++) { > > > @@ -627,9 +649,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 69f51acf235e..d1f1c5c25bd7 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -484,6 +484,61 @@ static int copy_dma_range_map(struct device *to, struct device *from) return 0; } +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; + 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; + } + ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); + if (ret) + goto free_rvdev; + + /* 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... (%pe)\n", + dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); + } + + 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; + +free_rvdev: + device_unregister(&rvdev->dev); + return ret; +} + +static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev) +{ + struct rproc *rproc = rvdev->rproc; + + rproc_remove_subdev(rproc, &rvdev->subdev); + list_del(&rvdev->node); + device_unregister(&rvdev->dev); +} + /** * rproc_handle_vdev() - handle a vdev fw resource * @rproc: the remote processor @@ -519,7 +574,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 > @@ -553,34 +607,10 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, rvdev->rproc = rproc; rvdev->index = rproc->nb_vdev++; - /* Initialise vdev subdevice */ - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); - rvdev->dev.parent = &rproc->dev; - 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; - } - - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent); + ret = rproc_rvdev_add_device(rvdev); if (ret) goto free_rvdev; - /* 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... (%pe)\n", - dma_get_mask(rproc->dev.parent), ERR_PTR(ret)); - } - /* parse the vrings */ for (i = 0; i < rsc->num_of_vrings; i++) { ret = rproc_parse_vring(rvdev, rsc, i); @@ -598,13 +628,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: @@ -619,7 +642,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++) { @@ -627,9 +649,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 remoteproc_virtio.c, 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.c. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- update vs previous revision: - update according to the rebase from v15-rc1 to v16-rc1 - split patch to introduce rproc_register_rvdev and rproc_unregister_rvdev function in a separate patch --- drivers/remoteproc/remoteproc_core.c | 94 +++++++++++++++++----------- 1 file changed, 57 insertions(+), 37 deletions(-)