1) Update from V8 [1]:
- rebase on for-next branch [2], to apply series on top of commit
7d7f8fe4e399 ("remoteproc: Harden rproc_handle_vdev() against integer overflow")
[1] https://lkml.org/lkml/2022/8/26/532
[2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/commit/?h=for-next&id=c81b67199cc4e2e4e8caf1abbbca1dc1b80bc642
2) Patchset description:
This series is a part of the work initiated a long time ago in
the series "remoteproc: Decorelate virtio from core"[3]
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 I2C, audio, video, ...).
For instance be able to declare a I2C device in a virtio-i2C node.
- Keep the legacy working!
- Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
[3] https://lkml.org/lkml/2020/4/16/1817
[4] https://lkml.org/lkml/2021/6/23/607
[5] https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
In term of device tree this would result in such hierarchy (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 probe 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 (4):
remoteproc: core: Introduce rproc_rvdev_add_device function
remoteproc: core: Introduce rproc_add_rvdev function
remoteproc: Move rproc_vdev management to remoteproc_virtio.c
remoteproc: virtio: Create platform device for the remoteproc_virtio
drivers/remoteproc/remoteproc_core.c | 154 +++---------------
drivers/remoteproc/remoteproc_internal.h | 23 ++-
drivers/remoteproc/remoteproc_virtio.c | 189 ++++++++++++++++++++---
include/linux/remoteproc.h | 6 +-
4 files changed, 210 insertions(+), 162 deletions(-)
--
2.24.3
The rproc structure contains a list of registered rproc_vdev structure.
To be able to move the management of the rproc_vdev structure in
remoteproc_virtio.c (i.e rproc_rvdev_add_device function),
introduce the rproc_add_rvdev and rproc_remove_rvdev functions.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f8dd8c526299..ddf2c716d7d8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -486,6 +486,18 @@ static int copy_dma_range_map(struct device *to, struct device *from)
return 0;
}
+static void rproc_add_rvdev(struct rproc *rproc, struct rproc_vdev *rvdev)
+{
+ if (rvdev && rproc)
+ list_add_tail(&rvdev->node, &rproc->rvdevs);
+}
+
+static void rproc_remove_rvdev(struct rproc_vdev *rvdev)
+{
+ if (rvdev)
+ list_del(&rvdev->node);
+}
+
static struct rproc_vdev *
rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
{
@@ -549,7 +561,7 @@ rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
goto unwind_vring_allocations;
}
- list_add_tail(&rvdev->node, &rproc->rvdevs);
+ rproc_add_rvdev(rproc, rvdev);
rvdev->subdev.start = rproc_vdev_do_start;
rvdev->subdev.stop = rproc_vdev_do_stop;
@@ -579,7 +591,7 @@ void rproc_vdev_release(struct kref *ref)
}
rproc_remove_subdev(rproc, &rvdev->subdev);
- list_del(&rvdev->node);
+ rproc_remove_rvdev(rvdev);
device_unregister(&rvdev->dev);
}
--
2.24.3
Define a platform driver to manage the remoteproc virtio device as
a 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.
remoteproc_virtio.c update:
- Add rproc_virtio_driver platform driver. The probe ops replaces
the rproc_rvdev_add_device function.
- All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
- rproc_rvdev_release is removed as associated to the rvdev device.
- The use of rvdev->kref counter is replaced by get/put_device on the
remoteproc virtio platform device.
- The vdev device no longer increments rproc device counter.
increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
function in charge of the vrings allocation/free.
remoteproc_core.c update:
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.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 12 +-
drivers/remoteproc/remoteproc_internal.h | 2 -
drivers/remoteproc/remoteproc_virtio.c | 143 ++++++++++++-----------
include/linux/remoteproc.h | 6 +-
4 files changed, 82 insertions(+), 81 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 2e88f933a4eb..e7c25477b0af 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -480,6 +480,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
struct rproc_vdev *rvdev;
size_t rsc_size;
struct rproc_vdev_data rvdev_data;
+ struct platform_device *pdev;
/* make sure resource isn't truncated */
rsc_size = struct_size(rsc, vring, rsc->num_of_vrings);
@@ -508,9 +509,12 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
rvdev_data.rsc_offset = offset;
rvdev_data.rsc = rsc;
- rvdev = rproc_rvdev_add_device(rproc, &rvdev_data);
- if (IS_ERR(rvdev))
- return PTR_ERR(rvdev);
+ pdev = platform_device_register_data(dev, "rproc-virtio", rvdev_data.index, &rvdev_data,
+ sizeof(rvdev_data));
+ if (IS_ERR(pdev)) {
+ dev_err(dev, "failed to create rproc-virtio device\n");
+ return PTR_ERR(pdev);
+ }
return 0;
}
@@ -1246,7 +1250,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);
+ platform_device_unregister(rvdev->pdev);
rproc_coredump_cleanup(rproc);
}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 711b0e1f2118..bf1fb7bba1a3 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -45,9 +45,7 @@ int rproc_of_parse_firmware(struct device *dev, int index,
const char **fw_name);
/* from remoteproc_virtio.c */
-struct rproc_vdev *rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data);
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);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 0aaa70d91aa8..a29e3b8ff69c 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -13,6 +13,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>
@@ -46,7 +47,11 @@ static int copy_dma_range_map(struct device *to, struct device *from)
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 +348,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 +367,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;
@@ -435,18 +437,8 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
vdev->dev.parent = dev;
vdev->dev.release = rproc_virtio_dev_release;
- /*
- * We're indirectly making a non-temporary copy of the rproc pointer
- * here, because drivers probed with this vdev will indirectly
- * access the wrapping rproc.
- *
- * Therefore we must increment the rproc refcount here, and decrement
- * it _only_ when the vdev is released.
- */
- get_device(&rproc->dev);
-
/* Reference the vdev and vring allocations */
- kref_get(&rvdev->refcount);
+ get_device(dev);
ret = register_virtio_device(vdev);
if (ret) {
@@ -488,79 +480,57 @@ 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);
+ dev_warn(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);
- dma_release_coherent_memory(dev);
-
- kfree(rvdev);
-}
-
-struct rproc_vdev *
-rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
+static int rproc_virtio_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
+ struct rproc_vdev_data *rvdev_data = dev->platform_data;
struct rproc_vdev *rvdev;
- struct fw_rsc_vdev *rsc = rvdev_data->rsc;
- char name[16];
+ struct rproc *rproc = container_of(dev->parent, struct rproc, dev);
+ struct fw_rsc_vdev *rsc;
int i, ret;
- rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
- if (!rvdev)
- return ERR_PTR(-ENOMEM);
+ if (!rvdev_data)
+ return -EINVAL;
- kref_init(&rvdev->refcount);
+ rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
+ if (!rvdev)
+ return -ENOMEM;
rvdev->id = rvdev_data->id;
rvdev->rproc = rproc;
rvdev->index = rvdev_data->index;
- /* Initialise vdev subdevice */
- snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
- rvdev->dev.parent = &rproc->dev;
- rvdev->dev.release = rproc_rvdev_release;
- dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
- dev_set_drvdata(&rvdev->dev, rvdev);
-
- ret = device_register(&rvdev->dev);
- if (ret) {
- put_device(&rvdev->dev);
- return ERR_PTR(ret);
- }
-
- ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
+ ret = copy_dma_range_map(dev, rproc->dev.parent);
if (ret)
- goto free_rvdev;
+ 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... (%pe)\n",
+ dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
}
+ platform_set_drvdata(pdev, rvdev);
+ rvdev->pdev = pdev;
+
+ rsc = rvdev_data->rsc;
+
/* parse the vrings */
for (i = 0; i < rsc->num_of_vrings; i++) {
ret = rproc_parse_vring(rvdev, rsc, i);
if (ret)
- goto free_rvdev;
+ return ret;
}
/* remember the resource offset*/
@@ -580,21 +550,30 @@ rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
rproc_add_subdev(rproc, &rvdev->subdev);
- return rvdev;
+ /*
+ * We're indirectly making a non-temporary copy of the rproc pointer
+ * here, because the platform device or the vdev device will indirectly
+ * access the wrapping rproc.
+ *
+ * Therefore we must increment the rproc refcount here, and decrement
+ * it _only_ on platform remove.
+ */
+ get_device(&rproc->dev);
+
+ return 0;
unwind_vring_allocations:
for (i--; i >= 0; i--)
rproc_free_vring(&rvdev->vring[i]);
-free_rvdev:
- device_unregister(&rvdev->dev);
- return ERR_PTR(ret);
+
+ return ret;
}
-void rproc_vdev_release(struct kref *ref)
+static int rproc_virtio_remove(struct platform_device *pdev)
{
- struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
- struct rproc_vring *rvring;
+ 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++) {
@@ -604,5 +583,27 @@ void rproc_vdev_release(struct kref *ref)
rproc_remove_subdev(rproc, &rvdev->subdev);
rproc_remove_rvdev(rvdev);
- device_unregister(&rvdev->dev);
+
+ of_reserved_mem_device_release(&pdev->dev);
+ dma_release_coherent_memory(&pdev->dev);
+
+ put_device(&rproc->dev);
+
+ return 0;
}
+
+/* Platform driver */
+static const struct of_device_id rproc_virtio_match[] = {
+ { .compatible = "virtio,rproc" },
+ {},
+};
+
+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 aea79c77db0f..1abf56ad02da 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -616,9 +616,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
- * @dev: device struct used for reference count semantics
+ * @pdev: remoteproc virtio platform device
* @id: virtio device id (as in virtio_ids.h)
* @node: list node
* @rproc: the rproc handle
@@ -627,10 +626,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 device dev;
+ struct platform_device *pdev;
unsigned int id;
struct list_head node;
--
2.24.3
Move functions related to the management of the rproc_vdev
structure in the remoteproc_virtio.c.
The aim is to decorrelate as possible the virtio management from
the core part.
Due to the strong correlation between the vrings and the resource table
the rproc_alloc/parse/free_vring functions are kept in the remoteproc core.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 157 +----------------------
drivers/remoteproc/remoteproc_internal.h | 10 +-
drivers/remoteproc/remoteproc_virtio.c | 154 +++++++++++++++++++++-
3 files changed, 161 insertions(+), 160 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ddf2c716d7d8..2e88f933a4eb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -23,9 +23,7 @@
#include <linux/panic_notifier.h>
#include <linux/slab.h>
#include <linux/mutex.h>
-#include <linux/dma-map-ops.h>
#include <linux/dma-mapping.h>
-#include <linux/dma-direct.h> /* XXX: pokes into bus_dma_range */
#include <linux/firmware.h>
#include <linux/string.h>
#include <linux/debugfs.h>
@@ -384,7 +382,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
return 0;
}
-static int
+int
rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
{
struct rproc *rproc = rvdev->rproc;
@@ -435,166 +433,17 @@ 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);
- dma_release_coherent_memory(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_add_rvdev(struct rproc *rproc, struct rproc_vdev *rvdev)
+void rproc_add_rvdev(struct rproc *rproc, struct rproc_vdev *rvdev)
{
if (rvdev && rproc)
list_add_tail(&rvdev->node, &rproc->rvdevs);
}
-static void rproc_remove_rvdev(struct rproc_vdev *rvdev)
+void rproc_remove_rvdev(struct rproc_vdev *rvdev)
{
if (rvdev)
list_del(&rvdev->node);
}
-
-static struct rproc_vdev *
-rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
-{
- struct rproc_vdev *rvdev;
- struct fw_rsc_vdev *rsc = rvdev_data->rsc;
- char name[16];
- int i, ret;
-
- rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
- if (!rvdev)
- return ERR_PTR(-ENOMEM);
-
- kref_init(&rvdev->refcount);
-
- rvdev->id = rvdev_data->id;
- rvdev->rproc = rproc;
- rvdev->index = rvdev_data->index;
-
- /* Initialise vdev subdevice */
- snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
- rvdev->dev.parent = &rproc->dev;
- rvdev->dev.release = rproc_rvdev_release;
- dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
- dev_set_drvdata(&rvdev->dev, rvdev);
-
- ret = device_register(&rvdev->dev);
- if (ret) {
- put_device(&rvdev->dev);
- return ERR_PTR(ret);
- }
-
- ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
- if (ret)
- goto free_rvdev;
-
- /* Make device dma capable by inheriting from parent's capabilities */
- set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent));
-
- ret = dma_coerce_mask_and_coherent(&rvdev->dev,
- dma_get_mask(rproc->dev.parent));
- if (ret) {
- dev_warn(&rvdev->dev,
- "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
- dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
- }
-
- /* parse the vrings */
- for (i = 0; i < rsc->num_of_vrings; i++) {
- ret = rproc_parse_vring(rvdev, rsc, i);
- if (ret)
- goto free_rvdev;
- }
-
- /* remember the resource offset*/
- rvdev->rsc_offset = rvdev_data->rsc_offset;
-
- /* allocate the vring resources */
- for (i = 0; i < rsc->num_of_vrings; i++) {
- ret = rproc_alloc_vring(rvdev, i);
- if (ret)
- goto unwind_vring_allocations;
- }
-
- rproc_add_rvdev(rproc, rvdev);
-
- rvdev->subdev.start = rproc_vdev_do_start;
- rvdev->subdev.stop = rproc_vdev_do_stop;
-
- rproc_add_subdev(rproc, &rvdev->subdev);
-
- return rvdev;
-
-unwind_vring_allocations:
- for (i--; i >= 0; i--)
- rproc_free_vring(&rvdev->vring[i]);
-free_rvdev:
- device_unregister(&rvdev->dev);
- return ERR_PTR(ret);
-}
-
-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++) {
- rvring = &rvdev->vring[id];
- rproc_free_vring(rvring);
- }
-
- rproc_remove_subdev(rproc, &rvdev->subdev);
- rproc_remove_rvdev(rvdev);
- device_unregister(&rvdev->dev);
-}
-
/**
* rproc_handle_vdev() - handle a vdev fw resource
* @rproc: the remote processor
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 07c503de0f95..711b0e1f2118 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -41,14 +41,13 @@ struct rproc_vdev_data {
/* 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);
+struct rproc_vdev *rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data);
+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);
@@ -98,6 +97,7 @@ static inline void rproc_char_device_remove(struct rproc *rproc)
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
+int rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i);
phys_addr_t rproc_va_to_pa(void *cpu_addr);
int rproc_trigger_recovery(struct rproc *rproc);
@@ -110,6 +110,8 @@ struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
const struct firmware *fw);
struct rproc_mem_entry *
rproc_find_carveout_by_name(struct rproc *rproc, const char *name, ...);
+void rproc_add_rvdev(struct rproc *rproc, struct rproc_vdev *rvdev);
+void rproc_remove_rvdev(struct rproc_vdev *rvdev);
static inline int rproc_prepare_device(struct rproc *rproc)
{
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 0f7706e23eb9..0aaa70d91aa8 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -9,7 +9,9 @@
* Brian Swetland <[email protected]>
*/
+#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;
+}
+
static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
{
return container_of(vdev->dev.parent, struct rproc_vdev, dev);
@@ -341,7 +362,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;
@@ -449,10 +470,139 @@ 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);
+ dma_release_coherent_memory(dev);
+
+ kfree(rvdev);
+}
+
+struct rproc_vdev *
+rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
+{
+ struct rproc_vdev *rvdev;
+ struct fw_rsc_vdev *rsc = rvdev_data->rsc;
+ char name[16];
+ int i, ret;
+
+ rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
+ if (!rvdev)
+ return ERR_PTR(-ENOMEM);
+
+ kref_init(&rvdev->refcount);
+
+ rvdev->id = rvdev_data->id;
+ rvdev->rproc = rproc;
+ rvdev->index = rvdev_data->index;
+
+ /* Initialise vdev subdevice */
+ snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
+ rvdev->dev.parent = &rproc->dev;
+ rvdev->dev.release = rproc_rvdev_release;
+ dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
+ dev_set_drvdata(&rvdev->dev, rvdev);
+
+ ret = device_register(&rvdev->dev);
+ if (ret) {
+ put_device(&rvdev->dev);
+ return ERR_PTR(ret);
+ }
+
+ ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
+ if (ret)
+ goto free_rvdev;
+
+ /* Make device dma capable by inheriting from parent's capabilities */
+ set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent));
+
+ ret = dma_coerce_mask_and_coherent(&rvdev->dev,
+ dma_get_mask(rproc->dev.parent));
+ if (ret) {
+ dev_warn(&rvdev->dev,
+ "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
+ dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
+ }
+
+ /* parse the vrings */
+ for (i = 0; i < rsc->num_of_vrings; i++) {
+ ret = rproc_parse_vring(rvdev, rsc, i);
+ if (ret)
+ goto free_rvdev;
+ }
+
+ /* remember the resource offset*/
+ rvdev->rsc_offset = rvdev_data->rsc_offset;
+
+ /* allocate the vring resources */
+ for (i = 0; i < rsc->num_of_vrings; i++) {
+ ret = rproc_alloc_vring(rvdev, i);
+ if (ret)
+ goto unwind_vring_allocations;
+ }
+
+ rproc_add_rvdev(rproc, rvdev);
+
+ rvdev->subdev.start = rproc_vdev_do_start;
+ rvdev->subdev.stop = rproc_vdev_do_stop;
+
+ rproc_add_subdev(rproc, &rvdev->subdev);
+
+ return rvdev;
+
+unwind_vring_allocations:
+ for (i--; i >= 0; i--)
+ rproc_free_vring(&rvdev->vring[i]);
+free_rvdev:
+ device_unregister(&rvdev->dev);
+ return ERR_PTR(ret);
+}
+
+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++) {
+ rvring = &rvdev->vring[id];
+ rproc_free_vring(rvring);
+ }
+
+ rproc_remove_subdev(rproc, &rvdev->subdev);
+ rproc_remove_rvdev(rvdev);
+ device_unregister(&rvdev->dev);
+}
--
2.24.3
In preparation of the migration of the management of rvdev in
remoteproc_virtio.c, this patch spins off a new function to manage the
remoteproc virtio device creation.
The rproc_rvdev_add_device will be moved to remoteproc_virtio.c.
The rproc_vdev_data structure is introduced to provide information for
the rvdev creation. This structure allows to manage the rvdev and vrings
allocation in the rproc_rvdev_add_device function.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
Update from previous version:
- rebase on top of commit:
7d7f8fe4e399 ("remoteproc: Harden rproc_handle_vdev() against integer overflow")
drivers/remoteproc/remoteproc_core.c | 147 +++++++++++++----------
drivers/remoteproc/remoteproc_internal.h | 15 +++
2 files changed, 98 insertions(+), 64 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f5ba3b305aaf..f8dd8c526299 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -486,75 +486,23 @@ static int copy_dma_range_map(struct device *to, struct device *from)
return 0;
}
-/**
- * rproc_handle_vdev() - handle a vdev fw resource
- * @rproc: the remote processor
- * @ptr: the vring resource descriptor
- * @offset: offset of the resource entry
- * @avail: size of available data (for sanity checking the image)
- *
- * This resource entry requests the host to statically register a virtio
- * device (vdev), and setup everything needed to support it. It contains
- * everything needed to make it possible: the virtio device id, virtio
- * device features, vrings information, virtio config space, etc...
- *
- * Before registering the vdev, the vrings are allocated from non-cacheable
- * physically contiguous memory. Currently we only support two vrings per
- * remote processor (temporary limitation). We might also want to consider
- * doing the vring allocation only later when ->find_vqs() is invoked, and
- * then release them upon ->del_vqs().
- *
- * Note: @da is currently not really handled correctly: we dynamically
- * allocate it using the DMA API, ignoring requested hard coded addresses,
- * and we don't take care of any required IOMMU programming. This is all
- * going to be taken care of when the generic iommu-based DMA API will be
- * merged. Meanwhile, statically-addressed iommu-based firmware images should
- * use RSC_DEVMEM resource entries to map their required @da to the physical
- * address of their base CMA region (ouch, hacky!).
- *
- * Return: 0 on success, or an appropriate error code otherwise
- */
-static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
- int offset, int avail)
+static struct rproc_vdev *
+rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
{
- struct fw_rsc_vdev *rsc = ptr;
- struct device *dev = &rproc->dev;
struct rproc_vdev *rvdev;
- size_t rsc_size;
- int i, ret;
+ struct fw_rsc_vdev *rsc = rvdev_data->rsc;
char name[16];
-
- /* make sure resource isn't truncated */
- rsc_size = struct_size(rsc, vring, rsc->num_of_vrings);
- if (size_add(rsc_size, rsc->config_len) > avail) {
- dev_err(dev, "vdev rsc is truncated\n");
- return -EINVAL;
- }
-
- /* make sure reserved bytes are zeroes */
- if (rsc->reserved[0] || rsc->reserved[1]) {
- dev_err(dev, "vdev rsc has non zero reserved bytes\n");
- return -EINVAL;
- }
-
- 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;
- }
+ int i, ret;
rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
if (!rvdev)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
kref_init(&rvdev->refcount);
- rvdev->id = rsc->id;
+ rvdev->id = rvdev_data->id;
rvdev->rproc = rproc;
- rvdev->index = rproc->nb_vdev++;
+ rvdev->index = rvdev_data->index;
/* Initialise vdev subdevice */
snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
@@ -566,7 +514,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
ret = device_register(&rvdev->dev);
if (ret) {
put_device(&rvdev->dev);
- return ret;
+ return ERR_PTR(ret);
}
ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
@@ -579,7 +527,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
ret = dma_coerce_mask_and_coherent(&rvdev->dev,
dma_get_mask(rproc->dev.parent));
if (ret) {
- dev_warn(dev,
+ dev_warn(&rvdev->dev,
"Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
}
@@ -592,7 +540,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
}
/* remember the resource offset*/
- rvdev->rsc_offset = offset;
+ rvdev->rsc_offset = rvdev_data->rsc_offset;
/* allocate the vring resources */
for (i = 0; i < rsc->num_of_vrings; i++) {
@@ -608,14 +556,14 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
rproc_add_subdev(rproc, &rvdev->subdev);
- return 0;
+ return rvdev;
unwind_vring_allocations:
for (i--; i >= 0; i--)
rproc_free_vring(&rvdev->vring[i]);
free_rvdev:
device_unregister(&rvdev->dev);
- return ret;
+ return ERR_PTR(ret);
}
void rproc_vdev_release(struct kref *ref)
@@ -635,6 +583,77 @@ void rproc_vdev_release(struct kref *ref)
device_unregister(&rvdev->dev);
}
+/**
+ * rproc_handle_vdev() - handle a vdev fw resource
+ * @rproc: the remote processor
+ * @ptr: the vring resource descriptor
+ * @offset: offset of the resource entry
+ * @avail: size of available data (for sanity checking the image)
+ *
+ * This resource entry requests the host to statically register a virtio
+ * device (vdev), and setup everything needed to support it. It contains
+ * everything needed to make it possible: the virtio device id, virtio
+ * device features, vrings information, virtio config space, etc...
+ *
+ * Before registering the vdev, the vrings are allocated from non-cacheable
+ * physically contiguous memory. Currently we only support two vrings per
+ * remote processor (temporary limitation). We might also want to consider
+ * doing the vring allocation only later when ->find_vqs() is invoked, and
+ * then release them upon ->del_vqs().
+ *
+ * Note: @da is currently not really handled correctly: we dynamically
+ * allocate it using the DMA API, ignoring requested hard coded addresses,
+ * and we don't take care of any required IOMMU programming. This is all
+ * going to be taken care of when the generic iommu-based DMA API will be
+ * merged. Meanwhile, statically-addressed iommu-based firmware images should
+ * use RSC_DEVMEM resource entries to map their required @da to the physical
+ * address of their base CMA region (ouch, hacky!).
+ *
+ * Return: 0 on success, or an appropriate error code otherwise
+ */
+static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
+ int offset, int avail)
+{
+ struct fw_rsc_vdev *rsc = ptr;
+ struct device *dev = &rproc->dev;
+ struct rproc_vdev *rvdev;
+ size_t rsc_size;
+ struct rproc_vdev_data rvdev_data;
+
+ /* make sure resource isn't truncated */
+ rsc_size = struct_size(rsc, vring, rsc->num_of_vrings);
+ if (size_add(rsc_size, rsc->config_len) > avail) {
+ dev_err(dev, "vdev rsc is truncated\n");
+ return -EINVAL;
+ }
+
+ /* make sure reserved bytes are zeroes */
+ if (rsc->reserved[0] || rsc->reserved[1]) {
+ dev_err(dev, "vdev rsc has non zero reserved bytes\n");
+ return -EINVAL;
+ }
+
+ 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;
+ }
+
+ rvdev_data.id = rsc->id;
+ rvdev_data.index = rproc->nb_vdev++;
+ rvdev_data.rsc_offset = offset;
+ rvdev_data.rsc = rsc;
+
+ rvdev = rproc_rvdev_add_device(rproc, &rvdev_data);
+ if (IS_ERR(rvdev))
+ return PTR_ERR(rvdev);
+
+ return 0;
+}
+
/**
* 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 72d4d3d7d94d..07c503de0f95 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,21 @@ struct rproc_debug_trace {
struct rproc_mem_entry trace_mem;
};
+/**
+ * struct rproc_vdev_data - remoteproc virtio device data
+ * @rsc_offset: offset of the vdev's resource entry
+ * @id: virtio device id (as in virtio_ids.h)
+ * @index: vdev position versus other vdev declared in resource table
+ * @rsc: pointer to the vdev resource entry. Valid only during vdev init as
+ * the resource can be cached by rproc.
+ */
+struct rproc_vdev_data {
+ u32 rsc_offset;
+ unsigned int id;
+ u32 index;
+ struct fw_rsc_vdev *rsc;
+};
+
/* from remoteproc_core.c */
void rproc_release(struct kref *kref);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
--
2.24.3
On Wed, Sep 21, 2022 at 03:50:40PM +0200, Arnaud Pouliquen wrote:
> 1) Update from V8 [1]:
>
> - rebase on for-next branch [2], to apply series on top of commit
> 7d7f8fe4e399 ("remoteproc: Harden rproc_handle_vdev() against integer overflow")
>
> [1] https://lkml.org/lkml/2022/8/26/532
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/commit/?h=for-next&id=c81b67199cc4e2e4e8caf1abbbca1dc1b80bc642
>
> 2) Patchset description:
>
> This series is a part of the work initiated a long time ago in
> the series "remoteproc: Decorelate virtio from core"[3]
>
> 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 I2C, audio, video, ...).
> For instance be able to declare a I2C device in a virtio-i2C node.
> - Keep the legacy working!
> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
>
> [3] https://lkml.org/lkml/2020/4/16/1817
> [4] https://lkml.org/lkml/2021/6/23/607
> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>
> In term of device tree this would result in such hierarchy (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 probe 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 (4):
> remoteproc: core: Introduce rproc_rvdev_add_device function
> remoteproc: core: Introduce rproc_add_rvdev function
> remoteproc: Move rproc_vdev management to remoteproc_virtio.c
> remoteproc: virtio: Create platform device for the remoteproc_virtio
>
> drivers/remoteproc/remoteproc_core.c | 154 +++---------------
> drivers/remoteproc/remoteproc_internal.h | 23 ++-
> drivers/remoteproc/remoteproc_virtio.c | 189 ++++++++++++++++++++---
> include/linux/remoteproc.h | 6 +-
> 4 files changed, 210 insertions(+), 162 deletions(-)
Applied
Thanks,
Mathieu
>
> --
> 2.24.3
>
On Wed, Sep 21, 2022 at 03:50:44PM +0200, Arnaud Pouliquen wrote:
> Define a platform driver to manage the remoteproc virtio device as
> a 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.
>
> remoteproc_virtio.c update:
> - Add rproc_virtio_driver platform driver. The probe ops replaces
> the rproc_rvdev_add_device function.
> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
> - rproc_rvdev_release is removed as associated to the rvdev device.
> - The use of rvdev->kref counter is replaced by get/put_device on the
> remoteproc virtio platform device.
> - The vdev device no longer increments rproc device counter.
> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
> function in charge of the vrings allocation/free.
>
> remoteproc_core.c update:
> 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.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 12 +-
> drivers/remoteproc/remoteproc_internal.h | 2 -
> drivers/remoteproc/remoteproc_virtio.c | 143 ++++++++++++-----------
> include/linux/remoteproc.h | 6 +-
> 4 files changed, 82 insertions(+), 81 deletions(-)
[...]
> +/* Platform driver */
> +static const struct of_device_id rproc_virtio_match[] = {
> + { .compatible = "virtio,rproc" },
This is not documented. Add a binding schema if you need DT support.
> + {},
> +};
> +
> +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,
> + },
> +};
Hello Rob,
On 10/4/22 16:39, Rob Herring wrote:
> On Wed, Sep 21, 2022 at 03:50:44PM +0200, Arnaud Pouliquen wrote:
>> Define a platform driver to manage the remoteproc virtio device as
>> a 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.
>>
>> remoteproc_virtio.c update:
>> - Add rproc_virtio_driver platform driver. The probe ops replaces
>> the rproc_rvdev_add_device function.
>> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
>> - rproc_rvdev_release is removed as associated to the rvdev device.
>> - The use of rvdev->kref counter is replaced by get/put_device on the
>> remoteproc virtio platform device.
>> - The vdev device no longer increments rproc device counter.
>> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
>> function in charge of the vrings allocation/free.
>>
>> remoteproc_core.c update:
>> 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.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> Reviewed-by: Mathieu Poirier <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 12 +-
>> drivers/remoteproc/remoteproc_internal.h | 2 -
>> drivers/remoteproc/remoteproc_virtio.c | 143 ++++++++++++-----------
>> include/linux/remoteproc.h | 6 +-
>> 4 files changed, 82 insertions(+), 81 deletions(-)
>
> [...]
>
>> +/* Platform driver */
>> +static const struct of_device_id rproc_virtio_match[] = {
>> + { .compatible = "virtio,rproc" },
>
> This is not documented. Add a binding schema if you need DT support.
Mathieu also pointed this out to me in V8, you can see the discussion here [1]
Here is an extract:
"Yes I saw the warning, but for this first series it is not possible to declare
the associated "rproc-virtio" device in device tree.
So at this step it seems not make senses to create the devicetree bindings file.
More than that I don't know how I could justify the properties in bindings if
there is not driver code associated.
So i would be in favor of not adding the bindings in this series but to define
bindings in the first patch of my "step 2" series; as done on my github:
https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
"
[1] https://lore.kernel.org/lkml/20220920202201.GB1042164@p14s/
Regards,
Arnaud
>
>> + {},
>> +};
>> +
>> +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,
>> + },
>> +};
On 10/4/22 17:43, Rob Herring wrote:
> On Tue, Oct 4, 2022 at 10:18 AM Arnaud POULIQUEN
> <[email protected]> wrote:
>>
>> Hello Rob,
>>
>> On 10/4/22 16:39, Rob Herring wrote:
>>> On Wed, Sep 21, 2022 at 03:50:44PM +0200, Arnaud Pouliquen wrote:
>>>> Define a platform driver to manage the remoteproc virtio device as
>>>> a 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.
>>>>
>>>> remoteproc_virtio.c update:
>>>> - Add rproc_virtio_driver platform driver. The probe ops replaces
>>>> the rproc_rvdev_add_device function.
>>>> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
>>>> - rproc_rvdev_release is removed as associated to the rvdev device.
>>>> - The use of rvdev->kref counter is replaced by get/put_device on the
>>>> remoteproc virtio platform device.
>>>> - The vdev device no longer increments rproc device counter.
>>>> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
>>>> function in charge of the vrings allocation/free.
>>>>
>>>> remoteproc_core.c update:
>>>> 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.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>>>> Reviewed-by: Mathieu Poirier <[email protected]>
>>>> ---
>>>> drivers/remoteproc/remoteproc_core.c | 12 +-
>>>> drivers/remoteproc/remoteproc_internal.h | 2 -
>>>> drivers/remoteproc/remoteproc_virtio.c | 143 ++++++++++++-----------
>>>> include/linux/remoteproc.h | 6 +-
>>>> 4 files changed, 82 insertions(+), 81 deletions(-)
>>>
>>> [...]
>>>
>>>> +/* Platform driver */
>>>> +static const struct of_device_id rproc_virtio_match[] = {
>>>> + { .compatible = "virtio,rproc" },
>>>
>>> This is not documented. Add a binding schema if you need DT support.
>>
>>
>> Mathieu also pointed this out to me in V8, you can see the discussion here [1]
>>
>> Here is an extract:
>> "Yes I saw the warning, but for this first series it is not possible to declare
>> the associated "rproc-virtio" device in device tree.
>> So at this step it seems not make senses to create the devicetree bindings file.
>> More than that I don't know how I could justify the properties in bindings if
>> there is not driver code associated.
>>
>> So i would be in favor of not adding the bindings in this series but to define
>> bindings in the first patch of my "step 2" series; as done on my github:
>> https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
>> "
>
> Okay, since I only just started checking this (in a more reliable way
> than checkpatch does).
>
> But why do you need the DT match entry if it is not usable yet? You
> could just add that in later when the binding is defined. Review of
> the binding could say that 'virtio,rproc' should be something else and
> you'd have to change it.
Probably because I am too formatted to add a compatible when I create a driver,
this solution yet logical did not come to my mind...
I will send a fix to suppress the compatible.
Thanks,
Arnaud
>
> Rob
On Tue, Oct 4, 2022 at 10:18 AM Arnaud POULIQUEN
<[email protected]> wrote:
>
> Hello Rob,
>
> On 10/4/22 16:39, Rob Herring wrote:
> > On Wed, Sep 21, 2022 at 03:50:44PM +0200, Arnaud Pouliquen wrote:
> >> Define a platform driver to manage the remoteproc virtio device as
> >> a 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.
> >>
> >> remoteproc_virtio.c update:
> >> - Add rproc_virtio_driver platform driver. The probe ops replaces
> >> the rproc_rvdev_add_device function.
> >> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
> >> - rproc_rvdev_release is removed as associated to the rvdev device.
> >> - The use of rvdev->kref counter is replaced by get/put_device on the
> >> remoteproc virtio platform device.
> >> - The vdev device no longer increments rproc device counter.
> >> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
> >> function in charge of the vrings allocation/free.
> >>
> >> remoteproc_core.c update:
> >> 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.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> Reviewed-by: Mathieu Poirier <[email protected]>
> >> ---
> >> drivers/remoteproc/remoteproc_core.c | 12 +-
> >> drivers/remoteproc/remoteproc_internal.h | 2 -
> >> drivers/remoteproc/remoteproc_virtio.c | 143 ++++++++++++-----------
> >> include/linux/remoteproc.h | 6 +-
> >> 4 files changed, 82 insertions(+), 81 deletions(-)
> >
> > [...]
> >
> >> +/* Platform driver */
> >> +static const struct of_device_id rproc_virtio_match[] = {
> >> + { .compatible = "virtio,rproc" },
> >
> > This is not documented. Add a binding schema if you need DT support.
>
>
> Mathieu also pointed this out to me in V8, you can see the discussion here [1]
>
> Here is an extract:
> "Yes I saw the warning, but for this first series it is not possible to declare
> the associated "rproc-virtio" device in device tree.
> So at this step it seems not make senses to create the devicetree bindings file.
> More than that I don't know how I could justify the properties in bindings if
> there is not driver code associated.
>
> So i would be in favor of not adding the bindings in this series but to define
> bindings in the first patch of my "step 2" series; as done on my github:
> https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
> "
Okay, since I only just started checking this (in a more reliable way
than checkpatch does).
But why do you need the DT match entry if it is not usable yet? You
could just add that in later when the binding is defined. Review of
the binding could say that 'virtio,rproc' should be something else and
you'd have to change it.
Rob