[RFC PATCH 0/7] remoteproc: restructure the remoteproc VirtIO device

[RFC PATCH 0/7] remoteproc: restructure the remoteproc VirtIO device

[Arnaud Pouliquen]

This series is a part of the work initiate a long time ago in 
the series "remoteproc: Decorelate virtio from core"[1]


Objective of the work:
- Update the remoteproc VirtIO device creation (use platform device)
- Allow to declare remoteproc VirtIO device in DT
    - declare resources associated to a remote proc VirtIO
    - declare a list of VirtIO supported by the platform.
- Prepare the enhancement to more VirtIO devices (e.g audio, video, ...)
- Keep the legacy working!
- Try to improve the picture about concerns reported by Christoph Hellwing [2][3]

[1] https://lkml.org/lkml/2020/4/16/1817
[2] https://lkml.org/lkml/2021/6/23/607
[3] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/

In term of device tree this would result in such hiearchy (stm32mp1 example with 2 virtio RPMSG):

	m4_rproc: m4@10000000 {
		compatible = "st,stm32mp1-m4";
		reg = <0x10000000 0x40000>,
		      <0x30000000 0x40000>,
		      <0x38000000 0x10000>;
        memory-region = <&retram>, <&mcuram>,<&mcuram2>;
        mboxes = <&ipcc 2>, <&ipcc 3>;
        mbox-names = "shutdown", "detach";
        status = "okay";

        #address-cells = <1>;
        #size-cells = <0>;
        
        vdev@0 {
		compatible = "rproc-virtio";
		reg = <0>;
		virtio,id = <7>;  /* RPMSG */
		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
		mboxes = <&ipcc 0>, <&ipcc 1>;
		mbox-names = "vq0", "vq1";
		status = "okay";
        };

        vdev@1 {
		compatible = "rproc-virtio";
		reg = <1>;
		virtio,id = <7>;  /*RPMSG */
		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
		mboxes = <&ipcc 4>, <&ipcc 5>;
		mbox-names = "vq0", "vq1";
		status = "okay";
        };
};

I have divided the work in 4 steps to simplify the review, This series implements only
the step 1:
step 1:  redefine the remoteproc VirtIO device as a platform device
  - migrate rvdev management in remoteproc virtio.c,
  - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
step 2: add possibility to declare and prob a VirtIO sub node
  - VirtIO bindings declaration,
  - multi DT VirtIO devices support,
  - introduction of a remote proc virtio bind device mechanism ,
=> https://github.com/arnopo/linux/commits/step2-virtio-in-DT
step 3: Add memory declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step3-virtio-memories
step 4: Add mailbox declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step4-virtio-mailboxes

Arnaud Pouliquen (7):
  remoteproc: core: Introduce virtio device add/remove functions
  remoteproc: Move rvdev management in rproc_virtio
  remoteproc: Remove vdev_to_rvdev and vdev_to_rproc from remoteproc API
  remoteproc: create the REMOTEPROC_VIRTIO config
  remoteproc: virtio: Create platform device for the remoteproc_virtio
  remoteproc: virtio: Add helper to create platform device
  remoteproc: Instantiate the new remoteproc virtio platform device

 drivers/remoteproc/Kconfig               |  11 +-
 drivers/remoteproc/Makefile              |   2 +-
 drivers/remoteproc/remoteproc_core.c     | 142 +++-------------
 drivers/remoteproc/remoteproc_internal.h |  52 +++++-
 drivers/remoteproc/remoteproc_virtio.c   | 207 +++++++++++++++++++++--
 include/linux/remoteproc.h               |  18 +-
 6 files changed, 282 insertions(+), 150 deletions(-)

-- 
2.17.1

[RFC PATCH 1/7] remoteproc: core: Introduce virtio device add/remove functions

[Arnaud Pouliquen]

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(-)

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);
 }
 
 /**
-- 
2.17.1

Re: [RFC PATCH 1/7] remoteproc: core: Introduce virtio device add/remove functions

[Mathieu Poirier]

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
> 

[RFC PATCH 2/7] remoteproc: Move rvdev management in rproc_virtio

[Arnaud Pouliquen]

Move functions related to the management of the rproc_vdev
structure in the remoteproc virtio.
The aim is to decorrelate as possible the virtio management form
the core part.

Due to the strong correlation between the vrings and the resource table
the vrings management is kept in the remoteproc core.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c     | 127 -----------------------
 drivers/remoteproc/remoteproc_internal.h |  19 +++-
 drivers/remoteproc/remoteproc_virtio.c   | 121 ++++++++++++++++++++-
 3 files changed, 134 insertions(+), 133 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7c783ca291a7..67ccd088db8f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -434,119 +434,6 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	}
 }
 
-static int rproc_vdev_do_start(struct rproc_subdev *subdev)
-{
-	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
-
-	return rproc_add_virtio_dev(rvdev, rvdev->id);
-}
-
-static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
-{
-	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
-	int ret;
-
-	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
-	if (ret)
-		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
-}
-
-/**
- * rproc_rvdev_release() - release the existence of a rvdev
- *
- * @dev: the subdevice's dev
- */
-static void rproc_rvdev_release(struct device *dev)
-{
-	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
-
-	of_reserved_mem_device_release(dev);
-
-	kfree(rvdev);
-}
-
-static int copy_dma_range_map(struct device *to, struct device *from)
-{
-	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
-	int num_ranges = 0;
-
-	if (!map)
-		return 0;
-
-	for (r = map; r->size; r++)
-		num_ranges++;
-
-	new_map = kmemdup(map, array_size(num_ranges + 1, sizeof(*map)),
-			  GFP_KERNEL);
-	if (!new_map)
-		return -ENOMEM;
-	to->dma_range_map = new_map;
-	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
@@ -648,20 +535,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
 	return ret;
 }
 
-void rproc_vdev_release(struct kref *ref)
-{
-	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
-	struct rproc_vring *rvring;
-	int id;
-
-	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
-		rvring = &rvdev->vring[id];
-		rproc_free_vring(rvring);
-	}
-
-	rproc_rvdev_remove_device(rvdev);
-}
-
 /**
  * rproc_handle_trace() - handle a shared trace buffer resource
  * @rproc: the remote processor
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index a328e634b1de..152fe2e8668a 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -26,14 +26,13 @@ struct rproc_debug_trace {
 
 /* from remoteproc_core.c */
 void rproc_release(struct kref *kref);
-irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
-void rproc_vdev_release(struct kref *ref);
 int rproc_of_parse_firmware(struct device *dev, int index,
 			    const char **fw_name);
 
 /* from remoteproc_virtio.c */
-int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
-int rproc_remove_virtio_dev(struct device *dev, void *data);
+int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
+irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
+void rproc_vdev_release(struct kref *ref);
 
 /* from remoteproc_debugfs.c */
 void rproc_remove_trace_file(struct dentry *tfile);
@@ -196,4 +195,16 @@ bool rproc_u64_fit_in_size_t(u64 val)
 	return (val <= (size_t) -1);
 }
 
+static inline void rproc_register_rvdev(struct rproc_vdev *rvdev)
+{
+	if (rvdev && rvdev->rproc)
+		list_add_tail(&rvdev->node, &rvdev->rproc->rvdevs);
+}
+
+static inline void rproc_unregister_rvdev(struct rproc_vdev *rvdev)
+{
+	if (rvdev)
+		list_del(&rvdev->node);
+}
+
 #endif /* REMOTEPROC_INTERNAL_H */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index cf4d54e98e6a..5e5a78b3243f 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -9,7 +9,9 @@
  * Brian Swetland <swetland@google.com>
  */
 
+#include <linux/dma-direct.h>
 #include <linux/dma-map-ops.h>
+#include <linux/dma-mapping.h>
 #include <linux/export.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/remoteproc.h>
@@ -23,6 +25,25 @@
 
 #include "remoteproc_internal.h"
 
+static int copy_dma_range_map(struct device *to, struct device *from)
+{
+	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
+	int num_ranges = 0;
+
+	if (!map)
+		return 0;
+
+	for (r = map; r->size; r++)
+		num_ranges++;
+
+	new_map = kmemdup(map, array_size(num_ranges + 1, sizeof(*map)),
+			  GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+	to->dma_range_map = new_map;
+	return 0;
+}
+
 /* kick the remote processor, and let it know which virtqueue to poke at */
 static bool rproc_virtio_notify(struct virtqueue *vq)
 {
@@ -327,7 +348,7 @@ static void rproc_virtio_dev_release(struct device *dev)
  *
  * Return: 0 on success or an appropriate error value otherwise
  */
-int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
+static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = &rvdev->dev;
@@ -435,10 +456,106 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
  *
  * Return: 0
  */
-int rproc_remove_virtio_dev(struct device *dev, void *data)
+static int rproc_remove_virtio_dev(struct device *dev, void *data)
 {
 	struct virtio_device *vdev = dev_to_virtio(dev);
 
 	unregister_virtio_device(vdev);
 	return 0;
 }
+
+static int rproc_vdev_do_start(struct rproc_subdev *subdev)
+{
+	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
+
+	return rproc_add_virtio_dev(rvdev, rvdev->id);
+}
+
+static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
+	int ret;
+
+	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
+	if (ret)
+		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
+}
+
+/**
+ * rproc_rvdev_release() - release the existence of a rvdev
+ *
+ * @dev: the subdevice's dev
+ */
+static void rproc_rvdev_release(struct device *dev)
+{
+	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
+
+	of_reserved_mem_device_release(dev);
+
+	kfree(rvdev);
+}
+
+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);
+}
+
+void rproc_vdev_release(struct kref *ref)
+{
+	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
+	struct rproc_vring *rvring;
+	int id;
+
+	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
+		rvring = &rvdev->vring[id];
+		rproc_free_vring(rvring);
+	}
+
+	rproc_rvdev_remove_device(rvdev);
+}
-- 
2.17.1

Re: [RFC PATCH 2/7] remoteproc: Move rvdev management in rproc_virtio

[Mathieu Poirier]

More comments...

The title should probably be:

"remoteproc: Move rproc_vdev management to remoteproc_virtio.c"


On Fri, Oct 01, 2021 at 12:12:29PM +0200, Arnaud Pouliquen wrote:
> Move functions related to the management of the rproc_vdev
> structure in the remoteproc virtio.
> The aim is to decorrelate as possible the virtio management form

s/form/from

> the core part.
> 
> Due to the strong correlation between the vrings and the resource table
> the vrings management is kept in the remoteproc core.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 127 -----------------------
>  drivers/remoteproc/remoteproc_internal.h |  19 +++-
>  drivers/remoteproc/remoteproc_virtio.c   | 121 ++++++++++++++++++++-
>  3 files changed, 134 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7c783ca291a7..67ccd088db8f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -434,119 +434,6 @@ void rproc_free_vring(struct rproc_vring *rvring)
>  	}
>  }
>  
> -static int rproc_vdev_do_start(struct rproc_subdev *subdev)
> -{
> -	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> -
> -	return rproc_add_virtio_dev(rvdev, rvdev->id);
> -}
> -
> -static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
> -{
> -	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> -	int ret;
> -
> -	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
> -	if (ret)
> -		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
> -}
> -
> -/**
> - * rproc_rvdev_release() - release the existence of a rvdev
> - *
> - * @dev: the subdevice's dev
> - */
> -static void rproc_rvdev_release(struct device *dev)
> -{
> -	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> -
> -	of_reserved_mem_device_release(dev);
> -
> -	kfree(rvdev);
> -}
> -
> -static int copy_dma_range_map(struct device *to, struct device *from)
> -{
> -	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
> -	int num_ranges = 0;
> -
> -	if (!map)
> -		return 0;
> -
> -	for (r = map; r->size; r++)
> -		num_ranges++;
> -
> -	new_map = kmemdup(map, array_size(num_ranges + 1, sizeof(*map)),
> -			  GFP_KERNEL);
> -	if (!new_map)
> -		return -ENOMEM;
> -	to->dma_range_map = new_map;
> -	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
> @@ -648,20 +535,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	return ret;
>  }
>  
> -void rproc_vdev_release(struct kref *ref)
> -{
> -	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
> -	struct rproc_vring *rvring;
> -	int id;
> -
> -	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> -		rvring = &rvdev->vring[id];
> -		rproc_free_vring(rvring);
> -	}
> -
> -	rproc_rvdev_remove_device(rvdev);
> -}
> -
>  /**
>   * rproc_handle_trace() - handle a shared trace buffer resource
>   * @rproc: the remote processor
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index a328e634b1de..152fe2e8668a 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -26,14 +26,13 @@ struct rproc_debug_trace {
>  
>  /* from remoteproc_core.c */
>  void rproc_release(struct kref *kref);
> -irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> -void rproc_vdev_release(struct kref *ref);
>  int rproc_of_parse_firmware(struct device *dev, int index,
>  			    const char **fw_name);
>  
>  /* from remoteproc_virtio.c */
> -int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
> -int rproc_remove_virtio_dev(struct device *dev, void *data);
> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
> +irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> +void rproc_vdev_release(struct kref *ref);
>  
>  /* from remoteproc_debugfs.c */
>  void rproc_remove_trace_file(struct dentry *tfile);
> @@ -196,4 +195,16 @@ bool rproc_u64_fit_in_size_t(u64 val)
>  	return (val <= (size_t) -1);
>  }
>  
> +static inline void rproc_register_rvdev(struct rproc_vdev *rvdev)
> +{
> +	if (rvdev && rvdev->rproc)
> +		list_add_tail(&rvdev->node, &rvdev->rproc->rvdevs);
> +}
> +
> +static inline void rproc_unregister_rvdev(struct rproc_vdev *rvdev)
> +{
> +	if (rvdev)
> +		list_del(&rvdev->node);
> +}
> +
>  #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index cf4d54e98e6a..5e5a78b3243f 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -9,7 +9,9 @@
>   * Brian Swetland <swetland@google.com>
>   */
>  
> +#include <linux/dma-direct.h>
>  #include <linux/dma-map-ops.h>
> +#include <linux/dma-mapping.h>

Please see if either dma-direct.h and dma-mapping.h can be removed from
remoteproc_core.c

>  #include <linux/export.h>
>  #include <linux/of_reserved_mem.h>
>  #include <linux/remoteproc.h>
> @@ -23,6 +25,25 @@
>  
>  #include "remoteproc_internal.h"
>  
> +static int copy_dma_range_map(struct device *to, struct device *from)
> +{
> +	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
> +	int num_ranges = 0;
> +
> +	if (!map)
> +		return 0;
> +
> +	for (r = map; r->size; r++)
> +		num_ranges++;
> +
> +	new_map = kmemdup(map, array_size(num_ranges + 1, sizeof(*map)),
> +			  GFP_KERNEL);
> +	if (!new_map)
> +		return -ENOMEM;
> +	to->dma_range_map = new_map;
> +	return 0;
> +}
> +
>  /* kick the remote processor, and let it know which virtqueue to poke at */
>  static bool rproc_virtio_notify(struct virtqueue *vq)
>  {
> @@ -327,7 +348,7 @@ static void rproc_virtio_dev_release(struct device *dev)
>   *
>   * Return: 0 on success or an appropriate error value otherwise
>   */
> -int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> +static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
>  	struct device *dev = &rvdev->dev;
> @@ -435,10 +456,106 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>   *
>   * Return: 0
>   */
> -int rproc_remove_virtio_dev(struct device *dev, void *data)
> +static int rproc_remove_virtio_dev(struct device *dev, void *data)
>  {
>  	struct virtio_device *vdev = dev_to_virtio(dev);
>  
>  	unregister_virtio_device(vdev);
>  	return 0;
>  }
> +
> +static int rproc_vdev_do_start(struct rproc_subdev *subdev)
> +{
> +	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> +
> +	return rproc_add_virtio_dev(rvdev, rvdev->id);
> +}
> +
> +static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
> +{
> +	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> +	int ret;
> +
> +	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
> +	if (ret)
> +		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
> +}
> +
> +/**
> + * rproc_rvdev_release() - release the existence of a rvdev
> + *
> + * @dev: the subdevice's dev
> + */
> +static void rproc_rvdev_release(struct device *dev)
> +{
> +	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> +
> +	of_reserved_mem_device_release(dev);
> +
> +	kfree(rvdev);
> +}
> +
> +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);
> +}
> +
> +void rproc_vdev_release(struct kref *ref)
> +{
> +	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
> +	struct rproc_vring *rvring;
> +	int id;
> +
> +	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> +		rvring = &rvdev->vring[id];
> +		rproc_free_vring(rvring);
> +	}
> +
> +	rproc_rvdev_remove_device(rvdev);
> +}
> -- 
> 2.17.1
> 

[RFC PATCH 3/7] remoteproc: Remove vdev_to_rvdev and vdev_to_rproc from remoteproc API

[Arnaud Pouliquen]

These both functions are only used by the remoteproc_virtio.
There is no reason to expose them in the API.
Move the functions in remoteproc_virtio.c

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 12 ++++++++++++
 include/linux/remoteproc.h             | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 5e5a78b3243f..c9eecd2f9fb2 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -25,6 +25,18 @@
 
 #include "remoteproc_internal.h"
 
+static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
+{
+	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
+}
+
+static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+
+	return rvdev->rproc;
+}
+
 static int copy_dma_range_map(struct device *to, struct device *from)
 {
 	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 83c09ac36b13..e0600e1e5c17 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -684,18 +684,6 @@ int rproc_coredump_add_custom_segment(struct rproc *rproc,
 				      void *priv);
 int rproc_coredump_set_elf_info(struct rproc *rproc, u8 class, u16 machine);
 
-static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
-{
-	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
-}
-
-static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
-{
-	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
-
-	return rvdev->rproc;
-}
-
 void rproc_add_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
 
 void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
-- 
2.17.1

Re: (subset) [RFC PATCH 3/7] remoteproc: Remove vdev_to_rvdev and vdev_to_rproc from remoteproc API

[Bjorn Andersson]

On Fri, 1 Oct 2021 12:12:30 +0200, Arnaud Pouliquen wrote:
> These both functions are only used by the remoteproc_virtio.
> There is no reason to expose them in the API.
> Move the functions in remoteproc_virtio.c
> 
> 

Applied, thanks!

[3/7] remoteproc: Remove vdev_to_rvdev and vdev_to_rproc from remoteproc API
      commit: 9955548919c47a6987b40d90a30fd56bbc043e7b

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

Re: [RFC PATCH 3/7] remoteproc: Remove vdev_to_rvdev and vdev_to_rproc from remoteproc API

[Mathieu Poirier]

Good morning,

On Fri, Oct 01, 2021 at 12:12:30PM +0200, Arnaud Pouliquen wrote:
> These both functions are only used by the remoteproc_virtio.

s/"These both functions"/"Both of these functions"

> There is no reason to expose them in the API.
> Move the functions in remoteproc_virtio.c
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_virtio.c | 12 ++++++++++++
>  include/linux/remoteproc.h             | 12 ------------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 5e5a78b3243f..c9eecd2f9fb2 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -25,6 +25,18 @@
>  
>  #include "remoteproc_internal.h"
>  
> +static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
> +{
> +	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
> +}
> +
> +static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> +{
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +
> +	return rvdev->rproc;
> +}
> +
>  static int copy_dma_range_map(struct device *to, struct device *from)
>  {
>  	const struct bus_dma_region *map = from->dma_range_map, *new_map, *r;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 83c09ac36b13..e0600e1e5c17 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -684,18 +684,6 @@ int rproc_coredump_add_custom_segment(struct rproc *rproc,
>  				      void *priv);
>  int rproc_coredump_set_elf_info(struct rproc *rproc, u8 class, u16 machine);
>  
> -static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
> -{
> -	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
> -}
> -
> -static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> -{
> -	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> -
> -	return rvdev->rproc;
> -}
> -
>  void rproc_add_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
>  
>  void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
> -- 
> 2.17.1
> 

[RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config

[Arnaud Pouliquen]

Create the config to associate to the remoteproc virtio.

Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
is that it defines API that is used by the built-in remote proc core.
Functions such are rproc_add_virtio_dev can be called during the
Linux boot phase.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/Kconfig               | 11 +++++++++-
 drivers/remoteproc/Makefile              |  2 +-
 drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 9a6eedc3994a..f271552c0d84 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -6,7 +6,7 @@ config REMOTEPROC
 	depends on HAS_DMA
 	select CRC32
 	select FW_LOADER
-	select VIRTIO
+	select REMOTEPROC_VIRTIO
 	select WANT_DEV_COREDUMP
 	help
 	  Support for remote processors (such as DSP coprocessors). These
@@ -14,6 +14,15 @@ config REMOTEPROC
 
 if REMOTEPROC
 
+config REMOTEPROC_VIRTIO
+	bool "Remoteproc virtio device "
+	select VIRTIO
+	help
+	  Say y here to have a virtio device support for the remoteproc
+	  communication.
+
+	  It's safe to say N if you don't use the virtio for the IPC.
+
 config REMOTEPROC_CDEV
 	bool "Remoteproc character device interface"
 	help
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index bb26c9e4ef9c..73d2384a76aa 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
 remoteproc-y				+= remoteproc_coredump.o
 remoteproc-y				+= remoteproc_debugfs.o
 remoteproc-y				+= remoteproc_sysfs.o
-remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
 obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 152fe2e8668a..4ce012c353c0 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
 			    const char **fw_name);
 
 /* from remoteproc_virtio.c */
+#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
+
 int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
 void rproc_vdev_release(struct kref *ref);
 
+#else
+
+int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return -ENXIO;
+}
+
+static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return IRQ_NONE;
+}
+
+static inline void rproc_vdev_release(struct kref *ref)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+}
+
+#endif
+
 /* from remoteproc_debugfs.c */
 void rproc_remove_trace_file(struct dentry *tfile);
 struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
-- 
2.17.1

Re: [RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config

[Bjorn Andersson]

On Fri 01 Oct 05:12 CDT 2021, Arnaud Pouliquen wrote:

> Create the config to associate to the remoteproc virtio.
> 
> Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
> is that it defines API that is used by the built-in remote proc core.
> Functions such are rproc_add_virtio_dev can be called during the
> Linux boot phase.
> 

Please don't introduce new Kconfig options for everything. Consider that
the expectation should be that everyone runs the default defconfig on
their boards - and if someone actually needs this level of control, they
are welcome to present patches with numbers showing the benefit of the
savings.

Thanks,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/Kconfig               | 11 +++++++++-
>  drivers/remoteproc/Makefile              |  2 +-
>  drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 9a6eedc3994a..f271552c0d84 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -6,7 +6,7 @@ config REMOTEPROC
>  	depends on HAS_DMA
>  	select CRC32
>  	select FW_LOADER
> -	select VIRTIO
> +	select REMOTEPROC_VIRTIO
>  	select WANT_DEV_COREDUMP
>  	help
>  	  Support for remote processors (such as DSP coprocessors). These
> @@ -14,6 +14,15 @@ config REMOTEPROC
>  
>  if REMOTEPROC
>  
> +config REMOTEPROC_VIRTIO
> +	bool "Remoteproc virtio device "
> +	select VIRTIO
> +	help
> +	  Say y here to have a virtio device support for the remoteproc
> +	  communication.
> +
> +	  It's safe to say N if you don't use the virtio for the IPC.
> +
>  config REMOTEPROC_CDEV
>  	bool "Remoteproc character device interface"
>  	help
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index bb26c9e4ef9c..73d2384a76aa 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
>  remoteproc-y				+= remoteproc_coredump.o
>  remoteproc-y				+= remoteproc_debugfs.o
>  remoteproc-y				+= remoteproc_sysfs.o
> -remoteproc-y				+= remoteproc_virtio.o
>  remoteproc-y				+= remoteproc_elf_loader.o
> +obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 152fe2e8668a..4ce012c353c0 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>  			    const char **fw_name);
>  
>  /* from remoteproc_virtio.c */
> +#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
> +
>  int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>  void rproc_vdev_release(struct kref *ref);
>  
> +#else
> +
> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return -ENXIO;
> +}
> +
> +static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return IRQ_NONE;
> +}
> +
> +static inline void rproc_vdev_release(struct kref *ref)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +}
> +
> +#endif
> +
>  /* from remoteproc_debugfs.c */
>  void rproc_remove_trace_file(struct dentry *tfile);
>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> -- 
> 2.17.1
> 

Re: [RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config

[Arnaud POULIQUEN]

On 10/9/21 5:36 AM, Bjorn Andersson wrote:
> On Fri 01 Oct 05:12 CDT 2021, Arnaud Pouliquen wrote:
> 
>> Create the config to associate to the remoteproc virtio.
>>
>> Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
>> is that it defines API that is used by the built-in remote proc core.
>> Functions such are rproc_add_virtio_dev can be called during the
>> Linux boot phase.
>>
> 
> Please don't introduce new Kconfig options for everything. Consider that
> the expectation should be that everyone runs the default defconfig on
> their boards - and if someone actually needs this level of control, they
> are welcome to present patches with numbers showing the benefit of the
> savings.

My goal here was to decorrelate the remote virtio from the remote proc,
so that platforms based on a non-virtio solution do not embed the code.
By reading your commentary it jumps out at me that that's stupid. The
REMOTEPROC_VIRTIO config is useless as the remoteproc_virtio must be kept
built-in for legacy compatibility.

Thanks,
Arnaud

> 
> Thanks,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/Kconfig               | 11 +++++++++-
>>  drivers/remoteproc/Makefile              |  2 +-
>>  drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
>>  3 files changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 9a6eedc3994a..f271552c0d84 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -6,7 +6,7 @@ config REMOTEPROC
>>  	depends on HAS_DMA
>>  	select CRC32
>>  	select FW_LOADER
>> -	select VIRTIO
>> +	select REMOTEPROC_VIRTIO
>>  	select WANT_DEV_COREDUMP
>>  	help
>>  	  Support for remote processors (such as DSP coprocessors). These
>> @@ -14,6 +14,15 @@ config REMOTEPROC
>>  
>>  if REMOTEPROC
>>  
>> +config REMOTEPROC_VIRTIO
>> +	bool "Remoteproc virtio device "
>> +	select VIRTIO
>> +	help
>> +	  Say y here to have a virtio device support for the remoteproc
>> +	  communication.
>> +
>> +	  It's safe to say N if you don't use the virtio for the IPC.
>> +
>>  config REMOTEPROC_CDEV
>>  	bool "Remoteproc character device interface"
>>  	help
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index bb26c9e4ef9c..73d2384a76aa 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
>>  remoteproc-y				+= remoteproc_coredump.o
>>  remoteproc-y				+= remoteproc_debugfs.o
>>  remoteproc-y				+= remoteproc_sysfs.o
>> -remoteproc-y				+= remoteproc_virtio.o
>>  remoteproc-y				+= remoteproc_elf_loader.o
>> +obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
>>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
>>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>> index 152fe2e8668a..4ce012c353c0 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>>  			    const char **fw_name);
>>  
>>  /* from remoteproc_virtio.c */
>> +#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
>> +
>>  int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>>  void rproc_vdev_release(struct kref *ref);
>>  
>> +#else
>> +
>> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return -ENXIO;
>> +}
>> +
>> +static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return IRQ_NONE;
>> +}
>> +
>> +static inline void rproc_vdev_release(struct kref *ref)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +}
>> +
>> +#endif
>> +
>>  /* from remoteproc_debugfs.c */
>>  void rproc_remove_trace_file(struct dentry *tfile);
>>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>> -- 
>> 2.17.1
>>

Re: [RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config

[Bjorn Andersson]

On Mon 11 Oct 08:58 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/9/21 5:36 AM, Bjorn Andersson wrote:
> > On Fri 01 Oct 05:12 CDT 2021, Arnaud Pouliquen wrote:
> > 
> >> Create the config to associate to the remoteproc virtio.
> >>
> >> Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
> >> is that it defines API that is used by the built-in remote proc core.
> >> Functions such are rproc_add_virtio_dev can be called during the
> >> Linux boot phase.
> >>
> > 
> > Please don't introduce new Kconfig options for everything. Consider that
> > the expectation should be that everyone runs the default defconfig on
> > their boards - and if someone actually needs this level of control, they
> > are welcome to present patches with numbers showing the benefit of the
> > savings.
> 
> My goal here was to decorrelate the remote virtio from the remote proc,
> so that platforms based on a non-virtio solution do not embed the code.
> By reading your commentary it jumps out at me that that's stupid.

I definitely don't think it's stupid. In a resource constraint
environment that want to use remoteproc but won't use virtio this makes
total sense.

But the added Kconfig creates complexity and people run into issues
because the default defconfig is incomplete, they got their defconfig
"wrong" or "make olddefconfig" misses the new config.

As such, I simply would like us to postpone the introduction of
configurability until there's someone that can show it's worth the
complexity.

> The REMOTEPROC_VIRTIO config is useless as the remoteproc_virtio must
> be kept built-in for legacy compatibility.
> 

Right, so we would have to make sure that in all "standard"
configurations REMOTEPROC_VIRTIO is included.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Bjorn
> > 
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/remoteproc/Kconfig               | 11 +++++++++-
> >>  drivers/remoteproc/Makefile              |  2 +-
> >>  drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
> >>  3 files changed, 39 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index 9a6eedc3994a..f271552c0d84 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -6,7 +6,7 @@ config REMOTEPROC
> >>  	depends on HAS_DMA
> >>  	select CRC32
> >>  	select FW_LOADER
> >> -	select VIRTIO
> >> +	select REMOTEPROC_VIRTIO
> >>  	select WANT_DEV_COREDUMP
> >>  	help
> >>  	  Support for remote processors (such as DSP coprocessors). These
> >> @@ -14,6 +14,15 @@ config REMOTEPROC
> >>  
> >>  if REMOTEPROC
> >>  
> >> +config REMOTEPROC_VIRTIO
> >> +	bool "Remoteproc virtio device "
> >> +	select VIRTIO
> >> +	help
> >> +	  Say y here to have a virtio device support for the remoteproc
> >> +	  communication.
> >> +
> >> +	  It's safe to say N if you don't use the virtio for the IPC.
> >> +
> >>  config REMOTEPROC_CDEV
> >>  	bool "Remoteproc character device interface"
> >>  	help
> >> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >> index bb26c9e4ef9c..73d2384a76aa 100644
> >> --- a/drivers/remoteproc/Makefile
> >> +++ b/drivers/remoteproc/Makefile
> >> @@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
> >>  remoteproc-y				+= remoteproc_coredump.o
> >>  remoteproc-y				+= remoteproc_debugfs.o
> >>  remoteproc-y				+= remoteproc_sysfs.o
> >> -remoteproc-y				+= remoteproc_virtio.o
> >>  remoteproc-y				+= remoteproc_elf_loader.o
> >> +obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
> >>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
> >>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
> >>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> >> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> >> index 152fe2e8668a..4ce012c353c0 100644
> >> --- a/drivers/remoteproc/remoteproc_internal.h
> >> +++ b/drivers/remoteproc/remoteproc_internal.h
> >> @@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
> >>  			    const char **fw_name);
> >>  
> >>  /* from remoteproc_virtio.c */
> >> +#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
> >> +
> >>  int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
> >>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> >>  void rproc_vdev_release(struct kref *ref);
> >>  
> >> +#else
> >> +
> >> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return -ENXIO;
> >> +}
> >> +
> >> +static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return IRQ_NONE;
> >> +}
> >> +
> >> +static inline void rproc_vdev_release(struct kref *ref)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +}
> >> +
> >> +#endif
> >> +
> >>  /* from remoteproc_debugfs.c */
> >>  void rproc_remove_trace_file(struct dentry *tfile);
> >>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> >> -- 
> >> 2.17.1
> >>

Re: [RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config

[Mathieu Poirier]

On Fri, Oct 01, 2021 at 12:12:31PM +0200, Arnaud Pouliquen wrote:
> Create the config to associate to the remoteproc virtio.
> 
> Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
> is that it defines API that is used by the built-in remote proc core.
> Functions such are rproc_add_virtio_dev can be called during the
> Linux boot phase.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/Kconfig               | 11 +++++++++-
>  drivers/remoteproc/Makefile              |  2 +-
>  drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 9a6eedc3994a..f271552c0d84 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -6,7 +6,7 @@ config REMOTEPROC
>  	depends on HAS_DMA
>  	select CRC32
>  	select FW_LOADER
> -	select VIRTIO
> +	select REMOTEPROC_VIRTIO
>  	select WANT_DEV_COREDUMP
>  	help
>  	  Support for remote processors (such as DSP coprocessors). These
> @@ -14,6 +14,15 @@ config REMOTEPROC
>  
>  if REMOTEPROC
>  
> +config REMOTEPROC_VIRTIO
> +	bool "Remoteproc virtio device "
> +	select VIRTIO
> +	help
> +	  Say y here to have a virtio device support for the remoteproc
> +	  communication.
> +
> +	  It's safe to say N if you don't use the virtio for the IPC.

This one is weird but there is no need to comment further after reading your
conversation with Bjorn.

> +
>  config REMOTEPROC_CDEV
>  	bool "Remoteproc character device interface"
>  	help
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index bb26c9e4ef9c..73d2384a76aa 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
>  remoteproc-y				+= remoteproc_coredump.o
>  remoteproc-y				+= remoteproc_debugfs.o
>  remoteproc-y				+= remoteproc_sysfs.o
> -remoteproc-y				+= remoteproc_virtio.o
>  remoteproc-y				+= remoteproc_elf_loader.o
> +obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 152fe2e8668a..4ce012c353c0 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>  			    const char **fw_name);
>  
>  /* from remoteproc_virtio.c */
> +#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
> +
>  int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>  void rproc_vdev_release(struct kref *ref);
>  
> +#else
> +
> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return -ENXIO;
> +}
> +
> +static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return IRQ_NONE;
> +}
> +
> +static inline void rproc_vdev_release(struct kref *ref)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +}
> +
> +#endif
> +
>  /* from remoteproc_debugfs.c */
>  void rproc_remove_trace_file(struct dentry *tfile);
>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> -- 
> 2.17.1
> 

[RFC PATCH 5/7] remoteproc: virtio: Create platform device for the remoteproc_virtio

[Arnaud Pouliquen]

Define a platform device for the remoteproc virtio to prepare the
management of the remoteproc virtio as a platform device.

The platform device allows to pass rproc_vdev_data platform data to
specify properties that are stored in the rproc_vdev structure.

Such approach will allow to preserve legacy remoteproc virtio device
creation but also to probe the device using device tree mechanism.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_internal.h |  6 +++
 drivers/remoteproc/remoteproc_virtio.c   | 65 ++++++++++++++++++++++++
 include/linux/remoteproc.h               |  2 +
 3 files changed, 73 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 4ce012c353c0..1b963a8912ed 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,12 @@ struct rproc_debug_trace {
 	struct rproc_mem_entry trace_mem;
 };
 
+struct rproc_vdev_data {
+	u32 rsc_offset;
+	unsigned int id;
+	unsigned int index;
+};
+
 /* from remoteproc_core.c */
 void rproc_release(struct kref *kref);
 int rproc_of_parse_firmware(struct device *dev, int index,
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index c9eecd2f9fb2..9b2ab79e4c4c 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2011 Texas Instruments, Inc.
  * Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2021 STMicroelectronics
  *
  * Ohad Ben-Cohen <ohad@wizery.com>
  * Brian Swetland <swetland@google.com>
@@ -13,6 +14,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/dma-mapping.h>
 #include <linux/export.h>
+#include <linux/of_platform.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/remoteproc.h>
 #include <linux/virtio.h>
@@ -571,3 +573,66 @@ void rproc_vdev_release(struct kref *ref)
 
 	rproc_rvdev_remove_device(rvdev);
 }
+
+static int rproc_virtio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rproc_vdev_data *vdev_data = dev->platform_data;
+	struct rproc_vdev *rvdev;
+	struct rproc *rproc;
+
+	if (!vdev_data)
+		return -EINVAL;
+
+	rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
+	if (!rvdev)
+		return -ENOMEM;
+
+	rproc = container_of(dev->parent, struct rproc, dev);
+
+	rvdev->rsc_offset = vdev_data->rsc_offset;
+	rvdev->id = vdev_data->id;
+	rvdev->index = vdev_data->index;
+
+	rvdev->pdev = pdev;
+	rvdev->rproc = rproc;
+
+	platform_set_drvdata(pdev, rvdev);
+
+	return rproc_rvdev_add_device(rvdev);
+}
+
+static int rproc_virtio_remove(struct platform_device *pdev)
+{
+	struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
+	struct rproc *rproc = rvdev->rproc;
+	struct rproc_vring *rvring;
+	int id;
+
+	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
+		rvring = &rvdev->vring[id];
+		rproc_free_vring(rvring);
+	}
+
+	rproc_remove_subdev(rproc, &rvdev->subdev);
+	rproc_unregister_rvdev(rvdev);
+	dev_dbg(&pdev->dev, "virtio dev %d removed\n",  rvdev->index);
+
+	return 0;
+}
+
+/* Platform driver */
+static const struct of_device_id rproc_virtio_match[] = {
+	{ .compatible = "rproc-virtio", },
+	{},
+};
+
+static struct platform_driver rproc_virtio_driver = {
+	.probe		= rproc_virtio_probe,
+	.remove		= rproc_virtio_remove,
+	.driver		= {
+		.name	= "rproc-virtio",
+		.of_match_table	= rproc_virtio_match,
+	},
+};
+builtin_platform_driver(rproc_virtio_driver);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e0600e1e5c17..542a3d4664f2 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -616,6 +616,7 @@ struct rproc_vring {
  * struct rproc_vdev - remoteproc state for a supported virtio device
  * @refcount: reference counter for the vdev and vring allocations
  * @subdev: handle for registering the vdev as a rproc subdevice
+ * @pdev: remoteproc virtio platform device
  * @dev: device struct used for reference count semantics
  * @id: virtio device id (as in virtio_ids.h)
  * @node: list node
@@ -628,6 +629,7 @@ struct rproc_vdev {
 	struct kref refcount;
 
 	struct rproc_subdev subdev;
+	struct platform_device *pdev;
 	struct device dev;
 
 	unsigned int id;
-- 
2.17.1

Re: [RFC PATCH 5/7] remoteproc: virtio: Create platform device for the remoteproc_virtio

[Mathieu Poirier]

The title mentions the creation of a "platform device" but this patch adds a
platform driver interface.

On Fri, Oct 01, 2021 at 12:12:32PM +0200, Arnaud Pouliquen wrote:
> Define a platform device for the remoteproc virtio to prepare the
> management of the remoteproc virtio as a platform device.

The above should be:

"Define a platform driver to prepare for the managemnt of remoteproc virtio
devices as platform devices."

> 
> The platform device allows to pass rproc_vdev_data platform data to
> specify properties that are stored in the rproc_vdev structure.
> 
> Such approach will allow to preserve legacy remoteproc virtio device
> creation but also to probe the device using device tree mechanism.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_internal.h |  6 +++
>  drivers/remoteproc/remoteproc_virtio.c   | 65 ++++++++++++++++++++++++
>  include/linux/remoteproc.h               |  2 +
>  3 files changed, 73 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ce012c353c0..1b963a8912ed 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,12 @@ struct rproc_debug_trace {
>  	struct rproc_mem_entry trace_mem;
>  };
>  
> +struct rproc_vdev_data {
> +	u32 rsc_offset;
> +	unsigned int id;
> +	unsigned int index;
> +};
> +
>  /* from remoteproc_core.c */
>  void rproc_release(struct kref *kref);
>  int rproc_of_parse_firmware(struct device *dev, int index,
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index c9eecd2f9fb2..9b2ab79e4c4c 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2011 Texas Instruments, Inc.
>   * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2021 STMicroelectronics
>   *
>   * Ohad Ben-Cohen <ohad@wizery.com>
>   * Brian Swetland <swetland@google.com>
> @@ -13,6 +14,7 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_reserved_mem.h>
>  #include <linux/remoteproc.h>
>  #include <linux/virtio.h>
> @@ -571,3 +573,66 @@ void rproc_vdev_release(struct kref *ref)
>  
>  	rproc_rvdev_remove_device(rvdev);
>  }
> +
> +static int rproc_virtio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rproc_vdev_data *vdev_data = dev->platform_data;
> +	struct rproc_vdev *rvdev;
> +	struct rproc *rproc;
> +
> +	if (!vdev_data)
> +		return -EINVAL;
> +
> +	rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
> +	if (!rvdev)
> +		return -ENOMEM;
> +
> +	rproc = container_of(dev->parent, struct rproc, dev);
> +
> +	rvdev->rsc_offset = vdev_data->rsc_offset;
> +	rvdev->id = vdev_data->id;
> +	rvdev->index = vdev_data->index;
> +
> +	rvdev->pdev = pdev;
> +	rvdev->rproc = rproc;
> +
> +	platform_set_drvdata(pdev, rvdev);
> +
> +	return rproc_rvdev_add_device(rvdev);
> +}
> +
> +static int rproc_virtio_remove(struct platform_device *pdev)
> +{
> +	struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
> +	struct rproc *rproc = rvdev->rproc;
> +	struct rproc_vring *rvring;
> +	int id;
> +
> +	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> +		rvring = &rvdev->vring[id];
> +		rproc_free_vring(rvring);
> +	}
> +
> +	rproc_remove_subdev(rproc, &rvdev->subdev);
> +	rproc_unregister_rvdev(rvdev);
> +	dev_dbg(&pdev->dev, "virtio dev %d removed\n",  rvdev->index);
> +
> +	return 0;
> +}
> +
> +/* Platform driver */
> +static const struct of_device_id rproc_virtio_match[] = {
> +	{ .compatible = "rproc-virtio", },
> +	{},
> +};
> +
> +static struct platform_driver rproc_virtio_driver = {
> +	.probe		= rproc_virtio_probe,
> +	.remove		= rproc_virtio_remove,
> +	.driver		= {
> +		.name	= "rproc-virtio",
> +		.of_match_table	= rproc_virtio_match,
> +	},
> +};
> +builtin_platform_driver(rproc_virtio_driver);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..542a3d4664f2 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -616,6 +616,7 @@ struct rproc_vring {
>   * struct rproc_vdev - remoteproc state for a supported virtio device
>   * @refcount: reference counter for the vdev and vring allocations
>   * @subdev: handle for registering the vdev as a rproc subdevice
> + * @pdev: remoteproc virtio platform device
>   * @dev: device struct used for reference count semantics
>   * @id: virtio device id (as in virtio_ids.h)
>   * @node: list node
> @@ -628,6 +629,7 @@ struct rproc_vdev {
>  	struct kref refcount;
>  
>  	struct rproc_subdev subdev;
> +	struct platform_device *pdev;
>  	struct device dev;
>  
>  	unsigned int id;
> -- 
> 2.17.1
> 

Re: [RFC PATCH 5/7] remoteproc: virtio: Create platform device for the remoteproc_virtio

[Mathieu Poirier]

On Fri, Oct 01, 2021 at 12:12:32PM +0200, Arnaud Pouliquen wrote:
> Define a platform device for the remoteproc virtio to prepare the
> management of the remoteproc virtio as a platform device.
> 
> The platform device allows to pass rproc_vdev_data platform data to
> specify properties that are stored in the rproc_vdev structure.
> 
> Such approach will allow to preserve legacy remoteproc virtio device
> creation but also to probe the device using device tree mechanism.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_internal.h |  6 +++
>  drivers/remoteproc/remoteproc_virtio.c   | 65 ++++++++++++++++++++++++
>  include/linux/remoteproc.h               |  2 +
>  3 files changed, 73 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ce012c353c0..1b963a8912ed 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,12 @@ struct rproc_debug_trace {
>  	struct rproc_mem_entry trace_mem;
>  };
>  
> +struct rproc_vdev_data {

s/rproc_vdev_data/rproc_vdev_pdata

> +	u32 rsc_offset;
> +	unsigned int id;
> +	unsigned int index;
> +};
> +
>  /* from remoteproc_core.c */
>  void rproc_release(struct kref *kref);
>  int rproc_of_parse_firmware(struct device *dev, int index,
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index c9eecd2f9fb2..9b2ab79e4c4c 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2011 Texas Instruments, Inc.
>   * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2021 STMicroelectronics
>   *
>   * Ohad Ben-Cohen <ohad@wizery.com>
>   * Brian Swetland <swetland@google.com>
> @@ -13,6 +14,7 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_reserved_mem.h>
>  #include <linux/remoteproc.h>
>  #include <linux/virtio.h>
> @@ -571,3 +573,66 @@ void rproc_vdev_release(struct kref *ref)
>  
>  	rproc_rvdev_remove_device(rvdev);
>  }
> +
> +static int rproc_virtio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rproc_vdev_data *vdev_data = dev->platform_data;
> +	struct rproc_vdev *rvdev;
> +	struct rproc *rproc;
> +
> +	if (!vdev_data)
> +		return -EINVAL;
> +
> +	rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
> +	if (!rvdev)
> +		return -ENOMEM;
> +
> +	rproc = container_of(dev->parent, struct rproc, dev);
> +
> +	rvdev->rsc_offset = vdev_data->rsc_offset;
> +	rvdev->id = vdev_data->id;
> +	rvdev->index = vdev_data->index;
> +
> +	rvdev->pdev = pdev;
> +	rvdev->rproc = rproc;
> +
> +	platform_set_drvdata(pdev, rvdev);
> +
> +	return rproc_rvdev_add_device(rvdev);
> +}
> +
> +static int rproc_virtio_remove(struct platform_device *pdev)
> +{
> +	struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
> +	struct rproc *rproc = rvdev->rproc;
> +	struct rproc_vring *rvring;
> +	int id;
> +
> +	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> +		rvring = &rvdev->vring[id];
> +		rproc_free_vring(rvring);
> +	}
> +
> +	rproc_remove_subdev(rproc, &rvdev->subdev);
> +	rproc_unregister_rvdev(rvdev);
> +	dev_dbg(&pdev->dev, "virtio dev %d removed\n",  rvdev->index);
> +
> +	return 0;
> +}
> +
> +/* Platform driver */
> +static const struct of_device_id rproc_virtio_match[] = {
> +	{ .compatible = "rproc-virtio", },
> +	{},
> +};
> +
> +static struct platform_driver rproc_virtio_driver = {
> +	.probe		= rproc_virtio_probe,
> +	.remove		= rproc_virtio_remove,
> +	.driver		= {
> +		.name	= "rproc-virtio",
> +		.of_match_table	= rproc_virtio_match,
> +	},
> +};
> +builtin_platform_driver(rproc_virtio_driver);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..542a3d4664f2 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -616,6 +616,7 @@ struct rproc_vring {
>   * struct rproc_vdev - remoteproc state for a supported virtio device
>   * @refcount: reference counter for the vdev and vring allocations
>   * @subdev: handle for registering the vdev as a rproc subdevice
> + * @pdev: remoteproc virtio platform device
>   * @dev: device struct used for reference count semantics
>   * @id: virtio device id (as in virtio_ids.h)
>   * @node: list node
> @@ -628,6 +629,7 @@ struct rproc_vdev {
>  	struct kref refcount;
>  
>  	struct rproc_subdev subdev;
> +	struct platform_device *pdev;
>  	struct device dev;
>  
>  	unsigned int id;
> -- 
> 2.17.1
> 

[RFC PATCH 6/7] remoteproc: virtio: Add helper to create platform device

[Arnaud Pouliquen]

Add capability to create platform device for the rproc virtio.
This is a step to move forward the management of the rproc virtio
as an independent device.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_internal.h | 15 ++++++++++
 drivers/remoteproc/remoteproc_virtio.c   | 36 ++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 1b963a8912ed..0bb1b14e5136 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -39,11 +39,26 @@ int rproc_of_parse_firmware(struct device *dev, int index,
 #if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
 
 int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
+struct platform_device *
+rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data);
+void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
 void rproc_vdev_release(struct kref *ref);
 
 #else
 
+static inline struct platform_device *
+rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data)
+{
+	return ERR_PTR(-ENXIO);
+}
+
+static inline void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+}
+
 int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
 {
 	/* This shouldn't be possible */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 9b2ab79e4c4c..7188fb8ce40f 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -574,6 +574,42 @@ void rproc_vdev_release(struct kref *ref)
 	rproc_rvdev_remove_device(rvdev);
 }
 
+/**
+ * rproc_virtio_register_device() - register a remoteproc virtio device
+ * @rproc: rproc handle to add the remoteproc virtio device to
+ * @vdev_data: platform device data
+ *
+ * Return: 0 on success, and an appropriate error value otherwise
+ */
+struct platform_device *
+rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data)
+{
+	struct device *dev = &rproc->dev;
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
+					     sizeof(*vdev_data));
+	if (PTR_ERR_OR_ZERO(pdev)) {
+		dev_err(rproc->dev.parent,
+			"failed to create rproc-virtio device\n");
+	}
+
+	return  pdev;
+}
+EXPORT_SYMBOL(rproc_virtio_register_device);
+
+/**
+ * rproc_virtio_unregister_device() - unregister a remoteproc virtio device
+ * @rvdev: remote proc virtio handle to unregister
+ *
+ */
+void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
+{
+	if (rvdev->pdev)
+		platform_device_unregister(rvdev->pdev);
+}
+EXPORT_SYMBOL(rproc_virtio_unregister_device);
+
 static int rproc_virtio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-- 
2.17.1

[RFC PATCH 7/7] remoteproc: Instantiate the new remoteproc virtio platform device

[Arnaud Pouliquen]

Migrate from the rvdev device to the rvdev platform device.
From this patch, when a vdev resource is found in the resource table
the remoteproc core register a platform device.

All reference to the rvdev->dev has been updated to rvdev-pdev->dev

The use of kref counter is replaced by get/put_device on the remoteproc
virtio device.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c     | 49 +++++++------
 drivers/remoteproc/remoteproc_internal.h | 16 ----
 drivers/remoteproc/remoteproc_virtio.c   | 93 ++++++------------------
 include/linux/remoteproc.h               |  4 -
 4 files changed, 50 insertions(+), 112 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 67ccd088db8f..d9256db8b130 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -467,6 +467,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
 {
 	struct fw_rsc_vdev *rsc = ptr;
 	struct device *dev = &rproc->dev;
+	struct rproc_vdev_data vdev_data;
+	struct platform_device *pdev;
 	struct rproc_vdev *rvdev;
 	int i, ret;
 
@@ -486,28 +488,34 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
 	dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
 		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
 
-	/* we currently support only two vrings per rvdev */
-	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
-		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
-		return -EINVAL;
-	}
+	/* platform data of the new rvdev platform */
+	vdev_data.rsc_offset = offset;
+	vdev_data.id  = rsc->id;
+	vdev_data.index  = rproc->nb_vdev;
 
-	rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
-	if (!rvdev)
-		return -ENOMEM;
-
-	kref_init(&rvdev->refcount);
-
-	rvdev->id = rsc->id;
-	rvdev->rproc = rproc;
-	rvdev->index = rproc->nb_vdev;
+	pdev = rproc_virtio_register_device(rproc, &vdev_data);
+	if (PTR_ERR_OR_ZERO(pdev)) {
+		dev_err(rproc->dev.parent,
+			"failed to create rproc-virtio device\n");
+		return PTR_ERR_OR_ZERO(pdev);
+	}
 
-	ret = rproc_rvdev_add_device(rvdev);
-	if (ret)
-		return ret;
+	/* If we made it to this point the remote proc virtio platform at been probed */
+	rvdev = platform_get_drvdata(pdev);
+	if (WARN_ON(!rvdev)) {
+		ret = -EINVAL;
+		goto free_rvdev;
+	}
 
 	rproc->nb_vdev++;
 
+	/* we currently support only two vrings per rvdev */
+	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
+		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
+		ret = -EINVAL;
+		goto free_rvdev;
+	}
+
 	/* parse the vrings */
 	for (i = 0; i < rsc->num_of_vrings; i++) {
 		ret = rproc_parse_vring(rvdev, rsc, i);
@@ -515,9 +523,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
 			goto free_rvdev;
 	}
 
-	/* remember the resource offset*/
-	rvdev->rsc_offset = offset;
-
 	/* allocate the vring resources */
 	for (i = 0; i < rsc->num_of_vrings; i++) {
 		ret = rproc_alloc_vring(rvdev, i);
@@ -531,7 +536,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
 	for (i--; i >= 0; i--)
 		rproc_free_vring(&rvdev->vring[i]);
 free_rvdev:
-	device_unregister(&rvdev->dev);
+	rproc_virtio_unregister_device(rvdev);
 	return ret;
 }
 
@@ -1270,7 +1275,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
 
 	/* clean up remote vdev entries */
 	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
-		kref_put(&rvdev->refcount, rproc_vdev_release);
+		rproc_virtio_unregister_device(rvdev);
 
 	rproc_coredump_cleanup(rproc);
 }
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 0bb1b14e5136..2f68e7380c77 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -38,12 +38,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
 /* from remoteproc_virtio.c */
 #if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
 
-int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
 struct platform_device *
 rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data);
 void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
-void rproc_vdev_release(struct kref *ref);
 
 #else
 
@@ -59,14 +57,6 @@ static inline void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
 	WARN_ON(1);
 }
 
-int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
-{
-	/* This shouldn't be possible */
-	WARN_ON(1);
-
-	return -ENXIO;
-}
-
 static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
 {
 	/* This shouldn't be possible */
@@ -75,12 +65,6 @@ static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
 	return IRQ_NONE;
 }
 
-static inline void rproc_vdev_release(struct kref *ref)
-{
-	/* This shouldn't be possible */
-	WARN_ON(1);
-}
-
 #endif
 
 /* from remoteproc_debugfs.c */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 7188fb8ce40f..34781a2136e6 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -29,7 +29,11 @@
 
 static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
+	struct platform_device *pdev;
+
+	pdev = container_of(vdev->dev.parent, struct platform_device, dev);
+
+	return platform_get_drvdata(pdev);
 }
 
 static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
@@ -343,13 +347,10 @@ static void rproc_virtio_dev_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);
 
 	kfree(vdev);
 
-	kref_put(&rvdev->refcount, rproc_vdev_release);
-
-	put_device(&rproc->dev);
+	put_device(&rvdev->pdev->dev);
 }
 
 /**
@@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
 static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = &rvdev->dev;
+	struct device *dev = &rvdev->pdev->dev;
 	struct virtio_device *vdev;
 	struct rproc_mem_entry *mem;
 	int ret;
@@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 	vdev->dev.release = rproc_virtio_dev_release;
 
 	/*
-	 * We're indirectly making a non-temporary copy of the rproc pointer
+	 * We're indirectly making a non-temporary copy of the rvdev pointer
 	 * here, because drivers probed with this vdev will indirectly
 	 * access the wrapping rproc.
 	 *
-	 * Therefore we must increment the rproc refcount here, and decrement
+	 * Therefore we must increment the rvdev refcount here, and decrement
 	 * it _only_ when the vdev is released.
 	 */
-	get_device(&rproc->dev);
+	get_device(dev);
 
-	/* Reference the vdev and vring allocations */
-	kref_get(&rvdev->refcount);
 
 	ret = register_virtio_device(vdev);
 	if (ret) {
@@ -488,57 +487,33 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
 static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
 {
 	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
+	struct device *dev = &rvdev->pdev->dev;
 	int ret;
 
-	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
+	ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
 	if (ret)
-		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
-}
-
-/**
- * rproc_rvdev_release() - release the existence of a rvdev
- *
- * @dev: the subdevice's dev
- */
-static void rproc_rvdev_release(struct device *dev)
-{
-	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
-
-	of_reserved_mem_device_release(dev);
-
-	kfree(rvdev);
+		dev_warn(dev, "can't remove vdev child device: %d\n", ret);
 }
 
-int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
+static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
 {
 	struct rproc *rproc = rvdev->rproc;
-	char name[16];
+	struct device *dev = &rvdev->pdev->dev;
 	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);
+	ret = copy_dma_range_map(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));
+	set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
 
-	ret = dma_coerce_mask_and_coherent(&rvdev->dev,
-					   dma_get_mask(rproc->dev.parent));
+	ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
 	if (ret) {
-		dev_warn(&rvdev->dev,
-			 "Failed to set DMA mask %llx. Trying to continue... %x\n",
+		dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... %x\n",
 			 dma_get_mask(rproc->dev.parent), ret);
+		return ret;
 	}
 
 	rproc_register_rvdev(rvdev);
@@ -548,30 +523,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
 
 	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);
-}
-
-void rproc_vdev_release(struct kref *ref)
-{
-	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
-	struct rproc_vring *rvring;
-	int id;
+	dev_dbg(dev, "virtio dev %d added\n",  rvdev->index);
 
-	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
-		rvring = &rvdev->vring[id];
-		rproc_free_vring(rvring);
-	}
-
-	rproc_rvdev_remove_device(rvdev);
+	return 0;
 }
 
 /**
@@ -590,8 +544,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_d
 	pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
 					     sizeof(*vdev_data));
 	if (PTR_ERR_OR_ZERO(pdev)) {
-		dev_err(rproc->dev.parent,
-			"failed to create rproc-virtio device\n");
+		dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
 	}
 
 	return  pdev;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 542a3d4664f2..7951a3e2b62a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -614,10 +614,8 @@ struct rproc_vring {
 
 /**
  * struct rproc_vdev - remoteproc state for a supported virtio device
- * @refcount: reference counter for the vdev and vring allocations
  * @subdev: handle for registering the vdev as a rproc subdevice
  * @pdev: remoteproc virtio platform device
- * @dev: device struct used for reference count semantics
  * @id: virtio device id (as in virtio_ids.h)
  * @node: list node
  * @rproc: the rproc handle
@@ -626,11 +624,9 @@ struct rproc_vring {
  * @index: vdev position versus other vdev declared in resource table
  */
 struct rproc_vdev {
-	struct kref refcount;
 
 	struct rproc_subdev subdev;
 	struct platform_device *pdev;
-	struct device dev;
 
 	unsigned int id;
 	struct list_head node;
-- 
2.17.1

Re: [RFC PATCH 7/7] remoteproc: Instantiate the new remoteproc virtio platform device

[Mathieu Poirier]

Good morning,

On Fri, Oct 01, 2021 at 12:12:34PM +0200, Arnaud Pouliquen wrote:
> Migrate from the rvdev device to the rvdev platform device.
> From this patch, when a vdev resource is found in the resource table
> the remoteproc core register a platform device.
> 
> All reference to the rvdev->dev has been updated to rvdev-pdev->dev
> 
> The use of kref counter is replaced by get/put_device on the remoteproc
> virtio device.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 49 +++++++------
>  drivers/remoteproc/remoteproc_internal.h | 16 ----
>  drivers/remoteproc/remoteproc_virtio.c   | 93 ++++++------------------
>  include/linux/remoteproc.h               |  4 -
>  4 files changed, 50 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 67ccd088db8f..d9256db8b130 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -467,6 +467,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  {
>  	struct fw_rsc_vdev *rsc = ptr;
>  	struct device *dev = &rproc->dev;
> +	struct rproc_vdev_data vdev_data;
> +	struct platform_device *pdev;
>  	struct rproc_vdev *rvdev;
>  	int i, ret;
>  
> @@ -486,28 +488,34 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
>  		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
>  
> -	/* we currently support only two vrings per rvdev */
> -	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> -		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> -		return -EINVAL;
> -	}
> +	/* platform data of the new rvdev platform */

This comment is very confusing... Shouldn't it be "rvdev device" instead?

> +	vdev_data.rsc_offset = offset;
> +	vdev_data.id  = rsc->id;
> +	vdev_data.index  = rproc->nb_vdev;
>  
> -	rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
> -	if (!rvdev)
> -		return -ENOMEM;
> -
> -	kref_init(&rvdev->refcount);
> -
> -	rvdev->id = rsc->id;
> -	rvdev->rproc = rproc;
> -	rvdev->index = rproc->nb_vdev;
> +	pdev = rproc_virtio_register_device(rproc, &vdev_data);
> +	if (PTR_ERR_OR_ZERO(pdev)) {
> +		dev_err(rproc->dev.parent,
> +			"failed to create rproc-virtio device\n");
> +		return PTR_ERR_OR_ZERO(pdev);
> +	}
>  
> -	ret = rproc_rvdev_add_device(rvdev);
> -	if (ret)
> -		return ret;
> +	/* If we made it to this point the remote proc virtio platform at been probed */

This too is just as confusing.  I have to guess you are referring to
rproc_virtio_probe().  All that guessing is very time consuming and makes
reviewing this work quite difficult.

I will review this set one more time but from an overall perspective I think it
is holding together.  That said I would like to see what other people think,
especially Christoph.

More comments to come...

Thanks,
Mathieu

> +	rvdev = platform_get_drvdata(pdev);
> +	if (WARN_ON(!rvdev)) {
> +		ret = -EINVAL;
> +		goto free_rvdev;
> +	}
>  
>  	rproc->nb_vdev++;
>  
> +	/* we currently support only two vrings per rvdev */
> +	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> +		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> +		ret = -EINVAL;
> +		goto free_rvdev;
> +	}
> +
>  	/* parse the vrings */
>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>  		ret = rproc_parse_vring(rvdev, rsc, i);
> @@ -515,9 +523,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  			goto free_rvdev;
>  	}
>  
> -	/* remember the resource offset*/
> -	rvdev->rsc_offset = offset;
> -
>  	/* allocate the vring resources */
>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>  		ret = rproc_alloc_vring(rvdev, i);
> @@ -531,7 +536,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	for (i--; i >= 0; i--)
>  		rproc_free_vring(&rvdev->vring[i]);
>  free_rvdev:
> -	device_unregister(&rvdev->dev);
> +	rproc_virtio_unregister_device(rvdev);
>  	return ret;
>  }
>  
> @@ -1270,7 +1275,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
>  
>  	/* clean up remote vdev entries */
>  	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> -		kref_put(&rvdev->refcount, rproc_vdev_release);
> +		rproc_virtio_unregister_device(rvdev);
>  
>  	rproc_coredump_cleanup(rproc);
>  }
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 0bb1b14e5136..2f68e7380c77 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -38,12 +38,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>  /* from remoteproc_virtio.c */
>  #if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>  struct platform_device *
>  rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data);
>  void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> -void rproc_vdev_release(struct kref *ref);
>  
>  #else
>  
> @@ -59,14 +57,6 @@ static inline void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
>  	WARN_ON(1);
>  }
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> -{
> -	/* This shouldn't be possible */
> -	WARN_ON(1);
> -
> -	return -ENXIO;
> -}
> -
>  static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>  {
>  	/* This shouldn't be possible */
> @@ -75,12 +65,6 @@ static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>  	return IRQ_NONE;
>  }
>  
> -static inline void rproc_vdev_release(struct kref *ref)
> -{
> -	/* This shouldn't be possible */
> -	WARN_ON(1);
> -}
> -
>  #endif
>  
>  /* from remoteproc_debugfs.c */
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 7188fb8ce40f..34781a2136e6 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -29,7 +29,11 @@
>  
>  static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
>  {
> -	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
> +	struct platform_device *pdev;
> +
> +	pdev = container_of(vdev->dev.parent, struct platform_device, dev);
> +
> +	return platform_get_drvdata(pdev);
>  }
>  
>  static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> @@ -343,13 +347,10 @@ static void rproc_virtio_dev_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);
>  
>  	kfree(vdev);
>  
> -	kref_put(&rvdev->refcount, rproc_vdev_release);
> -
> -	put_device(&rproc->dev);
> +	put_device(&rvdev->pdev->dev);
>  }
>  
>  /**
> @@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
>  static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
> -	struct device *dev = &rvdev->dev;
> +	struct device *dev = &rvdev->pdev->dev;
>  	struct virtio_device *vdev;
>  	struct rproc_mem_entry *mem;
>  	int ret;
> @@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  	vdev->dev.release = rproc_virtio_dev_release;
>  
>  	/*
> -	 * We're indirectly making a non-temporary copy of the rproc pointer
> +	 * We're indirectly making a non-temporary copy of the rvdev pointer
>  	 * here, because drivers probed with this vdev will indirectly
>  	 * access the wrapping rproc.
>  	 *
> -	 * Therefore we must increment the rproc refcount here, and decrement
> +	 * Therefore we must increment the rvdev refcount here, and decrement
>  	 * it _only_ when the vdev is released.
>  	 */
> -	get_device(&rproc->dev);
> +	get_device(dev);
>  
> -	/* Reference the vdev and vring allocations */
> -	kref_get(&rvdev->refcount);
>  
>  	ret = register_virtio_device(vdev);
>  	if (ret) {
> @@ -488,57 +487,33 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
>  static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
>  {
>  	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> +	struct device *dev = &rvdev->pdev->dev;
>  	int ret;
>  
> -	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
> +	ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
>  	if (ret)
> -		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
> -}
> -
> -/**
> - * rproc_rvdev_release() - release the existence of a rvdev
> - *
> - * @dev: the subdevice's dev
> - */
> -static void rproc_rvdev_release(struct device *dev)
> -{
> -	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> -
> -	of_reserved_mem_device_release(dev);
> -
> -	kfree(rvdev);
> +		dev_warn(dev, "can't remove vdev child device: %d\n", ret);
>  }
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>  {
>  	struct rproc *rproc = rvdev->rproc;
> -	char name[16];
> +	struct device *dev = &rvdev->pdev->dev;
>  	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);
> +	ret = copy_dma_range_map(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));
> +	set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
>  
> -	ret = dma_coerce_mask_and_coherent(&rvdev->dev,
> -					   dma_get_mask(rproc->dev.parent));
> +	ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
>  	if (ret) {
> -		dev_warn(&rvdev->dev,
> -			 "Failed to set DMA mask %llx. Trying to continue... %x\n",
> +		dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... %x\n",
>  			 dma_get_mask(rproc->dev.parent), ret);
> +		return ret;
>  	}
>  
>  	rproc_register_rvdev(rvdev);
> @@ -548,30 +523,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>  
>  	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);
> -}
> -
> -void rproc_vdev_release(struct kref *ref)
> -{
> -	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
> -	struct rproc_vring *rvring;
> -	int id;
> +	dev_dbg(dev, "virtio dev %d added\n",  rvdev->index);
>  
> -	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> -		rvring = &rvdev->vring[id];
> -		rproc_free_vring(rvring);
> -	}
> -
> -	rproc_rvdev_remove_device(rvdev);
> +	return 0;
>  }
>  
>  /**
> @@ -590,8 +544,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_d
>  	pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
>  					     sizeof(*vdev_data));
>  	if (PTR_ERR_OR_ZERO(pdev)) {
> -		dev_err(rproc->dev.parent,
> -			"failed to create rproc-virtio device\n");
> +		dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
>  	}
>  
>  	return  pdev;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 542a3d4664f2..7951a3e2b62a 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -614,10 +614,8 @@ struct rproc_vring {
>  
>  /**
>   * struct rproc_vdev - remoteproc state for a supported virtio device
> - * @refcount: reference counter for the vdev and vring allocations
>   * @subdev: handle for registering the vdev as a rproc subdevice
>   * @pdev: remoteproc virtio platform device
> - * @dev: device struct used for reference count semantics
>   * @id: virtio device id (as in virtio_ids.h)
>   * @node: list node
>   * @rproc: the rproc handle
> @@ -626,11 +624,9 @@ struct rproc_vring {
>   * @index: vdev position versus other vdev declared in resource table
>   */
>  struct rproc_vdev {
> -	struct kref refcount;
>  
>  	struct rproc_subdev subdev;
>  	struct platform_device *pdev;
> -	struct device dev;
>  
>  	unsigned int id;
>  	struct list_head node;
> -- 
> 2.17.1
> 

Re: [RFC PATCH 7/7] remoteproc: Instantiate the new remoteproc virtio platform device

[Mathieu Poirier]

On Fri, Oct 01, 2021 at 12:12:34PM +0200, Arnaud Pouliquen wrote:
> Migrate from the rvdev device to the rvdev platform device.
> From this patch, when a vdev resource is found in the resource table
> the remoteproc core register a platform device.
> 
> All reference to the rvdev->dev has been updated to rvdev-pdev->dev
> 
> The use of kref counter is replaced by get/put_device on the remoteproc
> virtio device.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 49 +++++++------
>  drivers/remoteproc/remoteproc_internal.h | 16 ----
>  drivers/remoteproc/remoteproc_virtio.c   | 93 ++++++------------------
>  include/linux/remoteproc.h               |  4 -
>  4 files changed, 50 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 67ccd088db8f..d9256db8b130 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -467,6 +467,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  {
>  	struct fw_rsc_vdev *rsc = ptr;
>  	struct device *dev = &rproc->dev;
> +	struct rproc_vdev_data vdev_data;
> +	struct platform_device *pdev;
>  	struct rproc_vdev *rvdev;
>  	int i, ret;
>  
> @@ -486,28 +488,34 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
>  		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
>  
> -	/* we currently support only two vrings per rvdev */
> -	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> -		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> -		return -EINVAL;
> -	}
> +	/* platform data of the new rvdev platform */
> +	vdev_data.rsc_offset = offset;
> +	vdev_data.id  = rsc->id;
> +	vdev_data.index  = rproc->nb_vdev;
>  
> -	rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
> -	if (!rvdev)
> -		return -ENOMEM;
> -
> -	kref_init(&rvdev->refcount);
> -
> -	rvdev->id = rsc->id;
> -	rvdev->rproc = rproc;
> -	rvdev->index = rproc->nb_vdev;
> +	pdev = rproc_virtio_register_device(rproc, &vdev_data);
> +	if (PTR_ERR_OR_ZERO(pdev)) {
> +		dev_err(rproc->dev.parent,
> +			"failed to create rproc-virtio device\n");
> +		return PTR_ERR_OR_ZERO(pdev);
> +	}
>  
> -	ret = rproc_rvdev_add_device(rvdev);
> -	if (ret)
> -		return ret;
> +	/* If we made it to this point the remote proc virtio platform at been probed */
> +	rvdev = platform_get_drvdata(pdev);
> +	if (WARN_ON(!rvdev)) {
> +		ret = -EINVAL;
> +		goto free_rvdev;
> +	}
>  
>  	rproc->nb_vdev++;
>  
> +	/* we currently support only two vrings per rvdev */
> +	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> +		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> +		ret = -EINVAL;
> +		goto free_rvdev;
> +	}
> +
>  	/* parse the vrings */
>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>  		ret = rproc_parse_vring(rvdev, rsc, i);
> @@ -515,9 +523,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  			goto free_rvdev;
>  	}
>  
> -	/* remember the resource offset*/
> -	rvdev->rsc_offset = offset;
> -
>  	/* allocate the vring resources */
>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>  		ret = rproc_alloc_vring(rvdev, i);
> @@ -531,7 +536,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	for (i--; i >= 0; i--)
>  		rproc_free_vring(&rvdev->vring[i]);
>  free_rvdev:
> -	device_unregister(&rvdev->dev);
> +	rproc_virtio_unregister_device(rvdev);
>  	return ret;
>  }

I have made it up to here but running out of time for this patchset.  I will review
the rest in another revision.  I haven't found a justification for
rproc_register_rvdev() and rproc_unregister_rvdev().  Am I missing something or
they can simply be removed and the list manipulations done in the code as it
currently is?

Thanks,
Mathieu

>  
> @@ -1270,7 +1275,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
>  
>  	/* clean up remote vdev entries */
>  	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> -		kref_put(&rvdev->refcount, rproc_vdev_release);
> +		rproc_virtio_unregister_device(rvdev);
>  
>  	rproc_coredump_cleanup(rproc);
>  }
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 0bb1b14e5136..2f68e7380c77 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -38,12 +38,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>  /* from remoteproc_virtio.c */
>  #if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>  struct platform_device *
>  rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data);
>  void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> -void rproc_vdev_release(struct kref *ref);
>  
>  #else
>  
> @@ -59,14 +57,6 @@ static inline void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
>  	WARN_ON(1);
>  }
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> -{
> -	/* This shouldn't be possible */
> -	WARN_ON(1);
> -
> -	return -ENXIO;
> -}
> -
>  static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>  {
>  	/* This shouldn't be possible */
> @@ -75,12 +65,6 @@ static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>  	return IRQ_NONE;
>  }
>  
> -static inline void rproc_vdev_release(struct kref *ref)
> -{
> -	/* This shouldn't be possible */
> -	WARN_ON(1);
> -}
> -
>  #endif
>  
>  /* from remoteproc_debugfs.c */
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 7188fb8ce40f..34781a2136e6 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -29,7 +29,11 @@
>  
>  static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
>  {
> -	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
> +	struct platform_device *pdev;
> +
> +	pdev = container_of(vdev->dev.parent, struct platform_device, dev);
> +
> +	return platform_get_drvdata(pdev);
>  }
>  
>  static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> @@ -343,13 +347,10 @@ static void rproc_virtio_dev_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);
>  
>  	kfree(vdev);
>  
> -	kref_put(&rvdev->refcount, rproc_vdev_release);
> -
> -	put_device(&rproc->dev);
> +	put_device(&rvdev->pdev->dev);
>  }
>  
>  /**
> @@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
>  static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
> -	struct device *dev = &rvdev->dev;
> +	struct device *dev = &rvdev->pdev->dev;
>  	struct virtio_device *vdev;
>  	struct rproc_mem_entry *mem;
>  	int ret;
> @@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  	vdev->dev.release = rproc_virtio_dev_release;
>  
>  	/*
> -	 * We're indirectly making a non-temporary copy of the rproc pointer
> +	 * We're indirectly making a non-temporary copy of the rvdev pointer
>  	 * here, because drivers probed with this vdev will indirectly
>  	 * access the wrapping rproc.
>  	 *
> -	 * Therefore we must increment the rproc refcount here, and decrement
> +	 * Therefore we must increment the rvdev refcount here, and decrement
>  	 * it _only_ when the vdev is released.
>  	 */
> -	get_device(&rproc->dev);
> +	get_device(dev);
>  
> -	/* Reference the vdev and vring allocations */
> -	kref_get(&rvdev->refcount);
>  
>  	ret = register_virtio_device(vdev);
>  	if (ret) {
> @@ -488,57 +487,33 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
>  static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
>  {
>  	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> +	struct device *dev = &rvdev->pdev->dev;
>  	int ret;
>  
> -	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
> +	ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
>  	if (ret)
> -		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
> -}
> -
> -/**
> - * rproc_rvdev_release() - release the existence of a rvdev
> - *
> - * @dev: the subdevice's dev
> - */
> -static void rproc_rvdev_release(struct device *dev)
> -{
> -	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> -
> -	of_reserved_mem_device_release(dev);
> -
> -	kfree(rvdev);
> +		dev_warn(dev, "can't remove vdev child device: %d\n", ret);
>  }
>  
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>  {
>  	struct rproc *rproc = rvdev->rproc;
> -	char name[16];
> +	struct device *dev = &rvdev->pdev->dev;
>  	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);
> +	ret = copy_dma_range_map(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));
> +	set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
>  
> -	ret = dma_coerce_mask_and_coherent(&rvdev->dev,
> -					   dma_get_mask(rproc->dev.parent));
> +	ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
>  	if (ret) {
> -		dev_warn(&rvdev->dev,
> -			 "Failed to set DMA mask %llx. Trying to continue... %x\n",
> +		dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... %x\n",
>  			 dma_get_mask(rproc->dev.parent), ret);
> +		return ret;
>  	}
>  
>  	rproc_register_rvdev(rvdev);
> @@ -548,30 +523,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>  
>  	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);
> -}
> -
> -void rproc_vdev_release(struct kref *ref)
> -{
> -	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
> -	struct rproc_vring *rvring;
> -	int id;
> +	dev_dbg(dev, "virtio dev %d added\n",  rvdev->index);
>  
> -	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> -		rvring = &rvdev->vring[id];
> -		rproc_free_vring(rvring);
> -	}
> -
> -	rproc_rvdev_remove_device(rvdev);
> +	return 0;
>  }
>  
>  /**
> @@ -590,8 +544,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_d
>  	pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
>  					     sizeof(*vdev_data));
>  	if (PTR_ERR_OR_ZERO(pdev)) {
> -		dev_err(rproc->dev.parent,
> -			"failed to create rproc-virtio device\n");
> +		dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
>  	}
>  
>  	return  pdev;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 542a3d4664f2..7951a3e2b62a 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -614,10 +614,8 @@ struct rproc_vring {
>  
>  /**
>   * struct rproc_vdev - remoteproc state for a supported virtio device
> - * @refcount: reference counter for the vdev and vring allocations
>   * @subdev: handle for registering the vdev as a rproc subdevice
>   * @pdev: remoteproc virtio platform device
> - * @dev: device struct used for reference count semantics
>   * @id: virtio device id (as in virtio_ids.h)
>   * @node: list node
>   * @rproc: the rproc handle
> @@ -626,11 +624,9 @@ struct rproc_vring {
>   * @index: vdev position versus other vdev declared in resource table
>   */
>  struct rproc_vdev {
> -	struct kref refcount;
>  
>  	struct rproc_subdev subdev;
>  	struct platform_device *pdev;
> -	struct device dev;
>  
>  	unsigned int id;
>  	struct list_head node;
> -- 
> 2.17.1
> 

Re: [RFC PATCH 7/7] remoteproc: Instantiate the new remoteproc virtio platform device

[Arnaud POULIQUEN]

Hi Mathieu

On 10/22/21 7:58 PM, Mathieu Poirier wrote:
> On Fri, Oct 01, 2021 at 12:12:34PM +0200, Arnaud Pouliquen wrote:
>> Migrate from the rvdev device to the rvdev platform device.
>> From this patch, when a vdev resource is found in the resource table
>> the remoteproc core register a platform device.
>>
>> All reference to the rvdev->dev has been updated to rvdev-pdev->dev
>>
>> The use of kref counter is replaced by get/put_device on the remoteproc
>> virtio device.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c     | 49 +++++++------
>>  drivers/remoteproc/remoteproc_internal.h | 16 ----
>>  drivers/remoteproc/remoteproc_virtio.c   | 93 ++++++------------------
>>  include/linux/remoteproc.h               |  4 -
>>  4 files changed, 50 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 67ccd088db8f..d9256db8b130 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -467,6 +467,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>  {
>>  	struct fw_rsc_vdev *rsc = ptr;
>>  	struct device *dev = &rproc->dev;
>> +	struct rproc_vdev_data vdev_data;
>> +	struct platform_device *pdev;
>>  	struct rproc_vdev *rvdev;
>>  	int i, ret;
>>  
>> @@ -486,28 +488,34 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>  	dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
>>  		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
>>  
>> -	/* we currently support only two vrings per rvdev */
>> -	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
>> -		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
>> -		return -EINVAL;
>> -	}
>> +	/* platform data of the new rvdev platform */
>> +	vdev_data.rsc_offset = offset;
>> +	vdev_data.id  = rsc->id;
>> +	vdev_data.index  = rproc->nb_vdev;
>>  
>> -	rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
>> -	if (!rvdev)
>> -		return -ENOMEM;
>> -
>> -	kref_init(&rvdev->refcount);
>> -
>> -	rvdev->id = rsc->id;
>> -	rvdev->rproc = rproc;
>> -	rvdev->index = rproc->nb_vdev;
>> +	pdev = rproc_virtio_register_device(rproc, &vdev_data);
>> +	if (PTR_ERR_OR_ZERO(pdev)) {
>> +		dev_err(rproc->dev.parent,
>> +			"failed to create rproc-virtio device\n");
>> +		return PTR_ERR_OR_ZERO(pdev);
>> +	}
>>  
>> -	ret = rproc_rvdev_add_device(rvdev);
>> -	if (ret)
>> -		return ret;
>> +	/* If we made it to this point the remote proc virtio platform at been probed */
>> +	rvdev = platform_get_drvdata(pdev);
>> +	if (WARN_ON(!rvdev)) {
>> +		ret = -EINVAL;
>> +		goto free_rvdev;
>> +	}
>>  
>>  	rproc->nb_vdev++;
>>  
>> +	/* we currently support only two vrings per rvdev */
>> +	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
>> +		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
>> +		ret = -EINVAL;
>> +		goto free_rvdev;
>> +	}
>> +
>>  	/* parse the vrings */
>>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>>  		ret = rproc_parse_vring(rvdev, rsc, i);
>> @@ -515,9 +523,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>  			goto free_rvdev;
>>  	}
>>  
>> -	/* remember the resource offset*/
>> -	rvdev->rsc_offset = offset;
>> -
>>  	/* allocate the vring resources */
>>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>>  		ret = rproc_alloc_vring(rvdev, i);
>> @@ -531,7 +536,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>  	for (i--; i >= 0; i--)
>>  		rproc_free_vring(&rvdev->vring[i]);
>>  free_rvdev:
>> -	device_unregister(&rvdev->dev);
>> +	rproc_virtio_unregister_device(rvdev);
>>  	return ret;
>>  }
> 
> I have made it up to here but running out of time for this patchset.  I will review
> the rest in another revision.  I haven't found a justification for
> rproc_register_rvdev() and rproc_unregister_rvdev().  Am I missing something or
> they can simply be removed and the list manipulations done in the code as it
> currently is?
> 

You are right, it seems unnecessary. I need to double check why I introduced
these functions, but this needs at least a rational in the commit.

I will send a new revision taking into account your and Bjorn's remarks.

Thanks,
Arnaud


> Thanks,
> Mathieu
> 
>>  
>> @@ -1270,7 +1275,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
>>  
>>  	/* clean up remote vdev entries */
>>  	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
>> -		kref_put(&rvdev->refcount, rproc_vdev_release);
>> +		rproc_virtio_unregister_device(rvdev);
>>  
>>  	rproc_coredump_cleanup(rproc);
>>  }
>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>> index 0bb1b14e5136..2f68e7380c77 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -38,12 +38,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>>  /* from remoteproc_virtio.c */
>>  #if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
>>  
>> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
>>  struct platform_device *
>>  rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_data);
>>  void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
>>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>> -void rproc_vdev_release(struct kref *ref);
>>  
>>  #else
>>  
>> @@ -59,14 +57,6 @@ static inline void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
>>  	WARN_ON(1);
>>  }
>>  
>> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>> -{
>> -	/* This shouldn't be possible */
>> -	WARN_ON(1);
>> -
>> -	return -ENXIO;
>> -}
>> -
>>  static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>>  {
>>  	/* This shouldn't be possible */
>> @@ -75,12 +65,6 @@ static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
>>  	return IRQ_NONE;
>>  }
>>  
>> -static inline void rproc_vdev_release(struct kref *ref)
>> -{
>> -	/* This shouldn't be possible */
>> -	WARN_ON(1);
>> -}
>> -
>>  #endif
>>  
>>  /* from remoteproc_debugfs.c */
>> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
>> index 7188fb8ce40f..34781a2136e6 100644
>> --- a/drivers/remoteproc/remoteproc_virtio.c
>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>> @@ -29,7 +29,11 @@
>>  
>>  static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
>>  {
>> -	return container_of(vdev->dev.parent, struct rproc_vdev, dev);
>> +	struct platform_device *pdev;
>> +
>> +	pdev = container_of(vdev->dev.parent, struct platform_device, dev);
>> +
>> +	return platform_get_drvdata(pdev);
>>  }
>>  
>>  static  struct rproc *vdev_to_rproc(struct virtio_device *vdev)
>> @@ -343,13 +347,10 @@ static void rproc_virtio_dev_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);
>>  
>>  	kfree(vdev);
>>  
>> -	kref_put(&rvdev->refcount, rproc_vdev_release);
>> -
>> -	put_device(&rproc->dev);
>> +	put_device(&rvdev->pdev->dev);
>>  }
>>  
>>  /**
>> @@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
>>  static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>  {
>>  	struct rproc *rproc = rvdev->rproc;
>> -	struct device *dev = &rvdev->dev;
>> +	struct device *dev = &rvdev->pdev->dev;
>>  	struct virtio_device *vdev;
>>  	struct rproc_mem_entry *mem;
>>  	int ret;
>> @@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>  	vdev->dev.release = rproc_virtio_dev_release;
>>  
>>  	/*
>> -	 * We're indirectly making a non-temporary copy of the rproc pointer
>> +	 * We're indirectly making a non-temporary copy of the rvdev pointer
>>  	 * here, because drivers probed with this vdev will indirectly
>>  	 * access the wrapping rproc.
>>  	 *
>> -	 * Therefore we must increment the rproc refcount here, and decrement
>> +	 * Therefore we must increment the rvdev refcount here, and decrement
>>  	 * it _only_ when the vdev is released.
>>  	 */
>> -	get_device(&rproc->dev);
>> +	get_device(dev);
>>  
>> -	/* Reference the vdev and vring allocations */
>> -	kref_get(&rvdev->refcount);
>>  
>>  	ret = register_virtio_device(vdev);
>>  	if (ret) {
>> @@ -488,57 +487,33 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
>>  static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
>>  {
>>  	struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
>> +	struct device *dev = &rvdev->pdev->dev;
>>  	int ret;
>>  
>> -	ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
>> +	ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
>>  	if (ret)
>> -		dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
>> -}
>> -
>> -/**
>> - * rproc_rvdev_release() - release the existence of a rvdev
>> - *
>> - * @dev: the subdevice's dev
>> - */
>> -static void rproc_rvdev_release(struct device *dev)
>> -{
>> -	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
>> -
>> -	of_reserved_mem_device_release(dev);
>> -
>> -	kfree(rvdev);
>> +		dev_warn(dev, "can't remove vdev child device: %d\n", ret);
>>  }
>>  
>> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>> +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>>  {
>>  	struct rproc *rproc = rvdev->rproc;
>> -	char name[16];
>> +	struct device *dev = &rvdev->pdev->dev;
>>  	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);
>> +	ret = copy_dma_range_map(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));
>> +	set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
>>  
>> -	ret = dma_coerce_mask_and_coherent(&rvdev->dev,
>> -					   dma_get_mask(rproc->dev.parent));
>> +	ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
>>  	if (ret) {
>> -		dev_warn(&rvdev->dev,
>> -			 "Failed to set DMA mask %llx. Trying to continue... %x\n",
>> +		dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... %x\n",
>>  			 dma_get_mask(rproc->dev.parent), ret);
>> +		return ret;
>>  	}
>>  
>>  	rproc_register_rvdev(rvdev);
>> @@ -548,30 +523,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>>  
>>  	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);
>> -}
>> -
>> -void rproc_vdev_release(struct kref *ref)
>> -{
>> -	struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
>> -	struct rproc_vring *rvring;
>> -	int id;
>> +	dev_dbg(dev, "virtio dev %d added\n",  rvdev->index);
>>  
>> -	for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
>> -		rvring = &rvdev->vring[id];
>> -		rproc_free_vring(rvring);
>> -	}
>> -
>> -	rproc_rvdev_remove_device(rvdev);
>> +	return 0;
>>  }
>>  
>>  /**
>> @@ -590,8 +544,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_data *vdev_d
>>  	pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
>>  					     sizeof(*vdev_data));
>>  	if (PTR_ERR_OR_ZERO(pdev)) {
>> -		dev_err(rproc->dev.parent,
>> -			"failed to create rproc-virtio device\n");
>> +		dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
>>  	}
>>  
>>  	return  pdev;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 542a3d4664f2..7951a3e2b62a 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -614,10 +614,8 @@ struct rproc_vring {
>>  
>>  /**
>>   * struct rproc_vdev - remoteproc state for a supported virtio device
>> - * @refcount: reference counter for the vdev and vring allocations
>>   * @subdev: handle for registering the vdev as a rproc subdevice
>>   * @pdev: remoteproc virtio platform device
>> - * @dev: device struct used for reference count semantics
>>   * @id: virtio device id (as in virtio_ids.h)
>>   * @node: list node
>>   * @rproc: the rproc handle
>> @@ -626,11 +624,9 @@ struct rproc_vring {
>>   * @index: vdev position versus other vdev declared in resource table
>>   */
>>  struct rproc_vdev {
>> -	struct kref refcount;
>>  
>>  	struct rproc_subdev subdev;
>>  	struct platform_device *pdev;
>> -	struct device dev;
>>  
>>  	unsigned int id;
>>  	struct list_head node;
>> -- 
>> 2.17.1
>>