2020-07-31 12:13:46

by Arnaud POULIQUEN

[permalink] [raw]
Subject: [PATCH 00/13] introduce IOCTL interface for RPMsg channel management

This serie is inspired by the RPMsg char driver. The RPMsg char driver
provides interfaces that:
- expose a char RPMsg device for communication with the remote processor
- expose controls interface for applications to create and release channels

The purpose of this series is to decorrelate the two interfaces:
- provide a char device for a RPMsg raw service similar to the rpmsg_char
but that can be probed by a RPMsg bus on a ns announcement
- generalize the use of the ioctl for all RPMsg services.

1) rpmsg raw driver: rpmsg_raw.c
For legacy reason, I created a new file instead of modifying the rpmsg_char
driver. But the rework of the rpmsg_char driver to only support the RPMsg
raw service could be an alternative
Few differences can be found:
- The rpmsg class has not been implemented. The associated attributes
are already available in /sys/bus/rpmsg/.
- The eptdev device is now an RPMsg device probed by a RPMsg bus driver
(probed only by the ioctl in rpmsg_char driver).
- The associated endpoint is now created by the bus no more on the
devfs open.

2) RPMsg control driver: rpmsg_ctrl.c
This driver is based on the control part of the RPMsg_char driver.
On probe a /dev/rpmsg_ctrl<X> interface is created to allow to manage the
channels.
The principles are the following:
- The RPMsg service driver registers it's name and the associated service
using the rpmsg_ctrl_unregister_ctl API. The list of supported services
is defined in include/uapi/linux/rpmsg.h and exposed to the
application thanks to a new field in rpmsg_endpoint_info struct.
- On the RPMsg bus probe(e.g virtio bus) an rpmsg_ctrl device is
registered that creates the control interface.
- The application can then create or release a channel by specifying:
- the name service
- the source address (default: RPMSG_ADDR_ANY)
- the destination address ( default: RPMSG_ADDR_ANY)
- the associated service (new)
- The rpmsg_ctrl uses the same interface than the ns announcement to
create and release the channel but using the driver_override field to
force the service name.
The "driver_override" allows to force the name service associated to
an RPMsg driver, bypassing the rpmsg_device_id based match check.
- At least for virtio bus, an associated ns announcement is sent to the
remote side.

Know current Limitations:
- Tested only with virtio RPMsg bus and for one vdev instance.
- rpmsg_device_match does not allow to match a local endpoint created by
ioctl with a same service requested by the ns announcement callback.
- Current implementation of the release makes it possible to release any
endpoint, even not created by the ioctl. Should we limit the release to
the RPMsg channel created with the ioctl?

This serie can be applied in Bjorn's rpmsg-next branch on top of the
RPMsg_ns series.

This series can be tested using rpmsgexport tools updated according to the
ioctl update and available here: https://github.com/arnopo/rpmsgexport.

Arnaud Pouliquen (13):
rpmsg: introduce rpmsg raw driver
rpmsg: introduce rpmsg_control driver for channel creation
rpmsg: add helper to create the rpmsg ctrl device
rpmsg: virtio: probe the rpmsg_ctrl device
rpmsg: uapi: add service param for create destroy ioctls
rpmsg: add RPMsg control info structure
rpmsg: control: add driver registration API
rpmsg: raw: register service to the rpmsg control
rpmsg: add override field in channel info
rpmsg: ns: initialize channel info override field
rpmsg: virtio: use the driver_override in channel creation
rpmsg: control: implement the ioctrl function to create device
rpmsg: ctrl: add support of the endpoints release

drivers/rpmsg/Kconfig | 17 ++
drivers/rpmsg/Makefile | 2 +
drivers/rpmsg/rpmsg_ctrl.c | 314 ++++++++++++++++++++++++++
drivers/rpmsg/rpmsg_internal.h | 18 ++
drivers/rpmsg/rpmsg_ns.c | 1 +
drivers/rpmsg/rpmsg_raw.c | 364 +++++++++++++++++++++++++++++++
drivers/rpmsg/virtio_rpmsg_bus.c | 37 +++-
include/linux/rpmsg.h | 15 ++
include/uapi/linux/rpmsg.h | 17 ++
9 files changed, 784 insertions(+), 1 deletion(-)
create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
create mode 100644 drivers/rpmsg/rpmsg_raw.c

--
2.17.1


2020-07-31 12:13:55

by Arnaud POULIQUEN

[permalink] [raw]
Subject: [PATCH 05/13] rpmsg: uapi: add service param for create destroy ioctls

To allow to associate an endpoint creation to a service, add
a new parameter in rpmsg_endpoint_info struct.
For each RPMsg service driver that want to support the ioctl interface
a new service will have to be added.
The RPMSG_START_PRIVATE_SERVICES is used to allow developer to add is
own services with ID higher or equal to 1024.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
include/uapi/linux/rpmsg.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..2ccc10ffacd4 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -9,16 +9,32 @@
#include <linux/ioctl.h>
#include <linux/types.h>

+/**
+ * enum rpmsg_services - list of supported RPMsg services
+ *
+ * @RPMSG_RAW_SERVICE: char device RPMSG service
+ * @RPMSG_START_PRIVATE_SERVICES: private services have to be declared after.
+ */
+enum rpmsg_services {
+ /* Reserved services */
+ RPMSG_RAW_SERVICE = 0,
+
+ /* Private services */
+ RPMSG_START_PRIVATE_SERVICES = 1024,
+};
+
/**
* struct rpmsg_endpoint_info - endpoint info representation
* @name: name of service
* @src: local address
* @dst: destination address
+ * @service: associated RPMsg service
*/
struct rpmsg_endpoint_info {
char name[32];
__u32 src;
__u32 dst;
+ __u32 service;
};

#define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
--
2.17.1

2020-07-31 12:14:02

by Arnaud POULIQUEN

[permalink] [raw]
Subject: [PATCH 11/13] rpmsg: virtio: use the driver_override in channel creation

Use the override information from the channel info structure
to set the rpdev override and so link the channel to a specific
driver.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index eaeaefdabf18..dbf7a597ae74 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -352,6 +352,7 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
rpdev = &vch->rpdev;
rpdev->src = chinfo->src;
rpdev->dst = chinfo->dst;
+ rpdev->driver_override = (char *)chinfo->driver_override;
if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
rpdev->ops = &virtio_rpmsg_w_nsa_ops;
else
--
2.17.1

2020-07-31 12:14:14

by Arnaud POULIQUEN

[permalink] [raw]
Subject: [PATCH 12/13] rpmsg: control: implement the ioctrl function to create device

Implement the ioctrl function that parses the list of
rpmsg drivers registered to create an associated device.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_ctrl.c | 39 ++++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 8773c8395401..d2a6dbb8798f 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -55,12 +55,47 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
return 0;
}

+static const char *rpmsg_ctrl_get_drv_name(u32 service)
+{
+ struct rpmsg_ctl_info *drv_info;
+
+ list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
+ if (drv_info->ctrl->service == service)
+ return drv_info->ctrl->drv_name;
+ }
+
+ return NULL;
+}
+
static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
unsigned long arg)
{
struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
-
- dev_info(&ctrldev->dev, "Control not yet implemented\n");
+ void __user *argp = (void __user *)arg;
+ struct rpmsg_channel_info chinfo;
+ struct rpmsg_endpoint_info eptinfo;
+ struct rpmsg_device *newch;
+
+ if (cmd != RPMSG_CREATE_EPT_IOCTL)
+ return -EINVAL;
+
+ if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+ return -EFAULT;
+
+ chinfo.driver_override = rpmsg_ctrl_get_drv_name(eptinfo.service);
+ if (!chinfo.driver_override)
+ return -ENODEV;
+
+ memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+ chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+ chinfo.src = eptinfo.src;
+ chinfo.dst = eptinfo.dst;
+
+ newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+ if (!newch) {
+ dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
+ return -ENXIO;
+ }

return 0;
};
--
2.17.1

2020-07-31 12:15:36

by Arnaud POULIQUEN

[permalink] [raw]
Subject: [PATCH 01/13] rpmsg: introduce rpmsg raw driver

This driver is a copy past of the rpmsg_char driver with following
update:
- The control part has been removed.
- The rpmsg class has been removed because the information already
available in /sys/bus.
- The device is now an RPMsg device probed by a RPMsg bus driver.
- The associated endpoint is now created by the bus not on the fs open.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/Kconfig | 9 +
drivers/rpmsg/Makefile | 1 +
drivers/rpmsg/rpmsg_raw.c | 347 ++++++++++++++++++++++++++++++++++++++
3 files changed, 357 insertions(+)
create mode 100644 drivers/rpmsg/rpmsg_raw.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0143c9864c45..900ec8f54081 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -23,6 +23,15 @@ config RPMSG_NS
channel that probes the associate RPMsg device on remote endpoint
service announcement.

+config RPMSG_RAW
+ tristate "RPMSG raw service driver"
+ depends on RPMSG
+ depends on NET
+ help
+ Say Y here to export rpmsg endpoints as char device files, found
+ in /dev. They make it possible for user-space programs to send and
+ receive rpmsg packets through a basic char device.
+
config RPMSG_MTK_SCP
tristate "MediaTek SCP"
depends on MTK_SCP
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..d75f0a65e71d 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
obj-$(CONFIG_RPMSG_QCOM_SMD) += qcom_smd.o
obj-$(CONFIG_RPMSG_VIRTIO) += virtio_rpmsg_bus.o
+obj-$(CONFIG_RPMSG_RAW) += rpmsg_raw.o
diff --git a/drivers/rpmsg/rpmsg_raw.c b/drivers/rpmsg/rpmsg_raw.c
new file mode 100644
index 000000000000..8684a78ab4d1
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_raw.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
+ *
+ * Based on rpmsg_char driver.
+ */
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/rpmsg.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/rpmsg.h>
+
+#include "rpmsg_internal.h"
+
+#define RPMSG_DEV_MAX (MINORMASK + 1)
+
+#define dev_to_eptdev(dev) container_of(dev, struct rpmsg_raw, dev)
+#define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_raw, cdev)
+
+static dev_t rpmsg_raw_major;
+
+static DEFINE_IDA(rpmsg_minor_ida);
+static DEFINE_IDA(rpmsg_ept_ida);
+
+/**
+ * struct rpmsg_raw - endpoint device context
+ * @dev: endpoint device
+ * @cdev: cdev for the endpoint device
+ * @rpdev: underlaying rpmsg device
+ * @ept_lock: synchronization of @ept modifications
+ * @queue_lock: synchronization of @queue operations
+ * @queue: incoming message queue
+ * @readq: wait object for incoming queue
+ */
+struct rpmsg_raw {
+ struct device dev;
+ struct cdev cdev;
+ struct rpmsg_device *rpdev;
+ struct mutex ept_lock;
+ spinlock_t queue_lock;
+ struct sk_buff_head queue;
+ wait_queue_head_t readq;
+};
+
+static int rpmsg_raw_cb(struct rpmsg_device *rpdev, void *buf, int len,
+ void *priv, u32 addr)
+{
+ struct rpmsg_raw *eptdev = dev_get_drvdata(&rpdev->dev);
+ struct sk_buff *skb;
+
+ skb = alloc_skb(len, GFP_ATOMIC);
+ if (!skb)
+ return -ENOMEM;
+
+ skb_put_data(skb, buf, len);
+
+ spin_lock(&eptdev->queue_lock);
+ skb_queue_tail(&eptdev->queue, skb);
+ spin_unlock(&eptdev->queue_lock);
+
+ /* wake up any blocking processes, waiting for new data */
+ wake_up_interruptible(&eptdev->readq);
+
+ return 0;
+}
+
+static int rpmsg_raw_open(struct inode *inode, struct file *filp)
+{
+ struct rpmsg_raw *eptdev = cdev_to_eptdev(inode->i_cdev);
+ struct device *dev = &eptdev->dev;
+
+ get_device(dev);
+
+ filp->private_data = eptdev;
+
+ return 0;
+}
+
+static int rpmsg_raw_release(struct inode *inode, struct file *filp)
+{
+ struct rpmsg_raw *eptdev = cdev_to_eptdev(inode->i_cdev);
+
+ /* Discard all SKBs */
+ skb_queue_purge(&eptdev->queue);
+
+ put_device(&eptdev->dev);
+
+ return 0;
+}
+
+static ssize_t rpmsg_raw_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+ struct file *filp = iocb->ki_filp;
+ struct rpmsg_raw *eptdev = filp->private_data;
+ unsigned long flags;
+ struct sk_buff *skb;
+ int use;
+
+ if (!eptdev->rpdev->ept)
+ return -EPIPE;
+
+ spin_lock_irqsave(&eptdev->queue_lock, flags);
+
+ /* Wait for data in the queue */
+ if (skb_queue_empty(&eptdev->queue)) {
+ spin_unlock_irqrestore(&eptdev->queue_lock, flags);
+
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ /* Wait until we get data or the endpoint goes away */
+ if (wait_event_interruptible(eptdev->readq,
+ !skb_queue_empty(&eptdev->queue) ||
+ !eptdev->rpdev->ept))
+ return -ERESTARTSYS;
+
+ /* We lost the endpoint while waiting */
+ if (!eptdev->rpdev->ept)
+ return -EPIPE;
+
+ spin_lock_irqsave(&eptdev->queue_lock, flags);
+ }
+
+ skb = skb_dequeue(&eptdev->queue);
+ spin_unlock_irqrestore(&eptdev->queue_lock, flags);
+ if (!skb)
+ return -EFAULT;
+
+ use = min_t(size_t, iov_iter_count(to), skb->len);
+ if (copy_to_iter(skb->data, use, to) != use)
+ use = -EFAULT;
+
+ kfree_skb(skb);
+
+ return use;
+}
+
+static ssize_t rpmsg_raw_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+ struct file *filp = iocb->ki_filp;
+ struct rpmsg_raw *eptdev = filp->private_data;
+ size_t len = iov_iter_count(from);
+ void *kbuf;
+ int ret;
+
+ kbuf = kzalloc(len, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ if (!copy_from_iter_full(kbuf, len, from)) {
+ ret = -EFAULT;
+ goto free_kbuf;
+ }
+
+ if (mutex_lock_interruptible(&eptdev->ept_lock)) {
+ ret = -ERESTARTSYS;
+ goto free_kbuf;
+ }
+
+ if (!eptdev->rpdev->ept) {
+ ret = -EPIPE;
+ goto unlock_eptdev;
+ }
+
+ if (filp->f_flags & O_NONBLOCK)
+ ret = rpmsg_trysend(eptdev->rpdev->ept, kbuf, len);
+ else
+ ret = rpmsg_send(eptdev->rpdev->ept, kbuf, len);
+
+unlock_eptdev:
+ mutex_unlock(&eptdev->ept_lock);
+
+free_kbuf:
+ kfree(kbuf);
+ return ret < 0 ? ret : len;
+}
+
+static __poll_t rpmsg_raw_poll(struct file *filp, poll_table *wait)
+{
+ struct rpmsg_raw *eptdev = filp->private_data;
+ __poll_t mask = 0;
+
+ if (!eptdev->rpdev->ept)
+ return EPOLLERR;
+
+ poll_wait(filp, &eptdev->readq, wait);
+
+ if (!skb_queue_empty(&eptdev->queue))
+ mask |= EPOLLIN | EPOLLRDNORM;
+
+ mask |= rpmsg_poll(eptdev->rpdev->ept, filp, wait);
+
+ return mask;
+}
+
+static void rpmsg_raw_release_device(struct device *dev)
+{
+ struct rpmsg_raw *eptdev = dev_to_eptdev(dev);
+
+ ida_simple_remove(&rpmsg_ept_ida, dev->id);
+ ida_simple_remove(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
+ cdev_del(&eptdev->cdev);
+ kfree(eptdev);
+}
+
+static const struct file_operations rpmsg_raw_fops = {
+ .owner = THIS_MODULE,
+ .open = rpmsg_raw_open,
+ .release = rpmsg_raw_release,
+ .read_iter = rpmsg_raw_read_iter,
+ .write_iter = rpmsg_raw_write_iter,
+ .poll = rpmsg_raw_poll,
+};
+
+static int rpmsg_raw_probe(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_raw *eptdev;
+ struct device *dev;
+ int ret;
+
+ eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
+ if (!eptdev)
+ return -ENOMEM;
+
+ dev = &eptdev->dev;
+ eptdev->rpdev = rpdev;
+
+ mutex_init(&eptdev->ept_lock);
+ spin_lock_init(&eptdev->queue_lock);
+ skb_queue_head_init(&eptdev->queue);
+ init_waitqueue_head(&eptdev->readq);
+
+ device_initialize(dev);
+ dev->parent = &rpdev->dev;
+ dev_set_drvdata(dev, eptdev);
+
+ cdev_init(&eptdev->cdev, &rpmsg_raw_fops);
+ eptdev->cdev.owner = THIS_MODULE;
+
+ ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
+ if (ret < 0) {
+ dev_err(dev, "failed to get IDA (%d)\n", ret);
+ goto free_eptdev;
+ }
+
+ dev->devt = MKDEV(MAJOR(rpmsg_raw_major), ret);
+
+ ret = ida_simple_get(&rpmsg_ept_ida, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto free_minor_ida;
+ dev->id = ret;
+ dev_set_name(dev, "rpmsg%d", dev->id);
+
+ ret = cdev_add(&eptdev->cdev, dev->devt, 1);
+ if (ret) {
+ ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+ dev_err(&rpdev->dev, "failed to add char device(%d)\n", ret);
+ goto free_ept_ida;
+ }
+
+ /* We can now rely on the release function for cleanup */
+ dev->release = rpmsg_raw_release_device;
+
+ ret = device_add(dev);
+ if (ret) {
+ dev_err(dev, "device_add failed: %d\n", ret);
+ put_device(dev);
+ }
+
+ dev_set_drvdata(&rpdev->dev, eptdev);
+
+ return ret;
+
+free_ept_ida:
+ ida_simple_remove(&rpmsg_ept_ida, dev->id);
+free_minor_ida:
+ ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+free_eptdev:
+ put_device(dev);
+ kfree(eptdev);
+
+ return ret;
+}
+
+static void rpmsg_raw_remove(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_raw *eptdev = dev_get_drvdata(&rpdev->dev);
+
+ /* wake up any blocked readers */
+ wake_up_interruptible(&eptdev->readq);
+
+ device_del(&eptdev->dev);
+
+ put_device(&eptdev->dev);
+}
+
+static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = {
+ { .name = "rpmsg-raw" },
+ { },
+};
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_sample_id_table);
+
+static struct rpmsg_driver rpmsg_raw_client = {
+ .drv.name = KBUILD_MODNAME,
+ .id_table = rpmsg_driver_sample_id_table,
+ .probe = rpmsg_raw_probe,
+ .callback = rpmsg_raw_cb,
+ .remove = rpmsg_raw_remove,
+};
+
+static int rpmsg_raw_init(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&rpmsg_raw_major, 0, RPMSG_DEV_MAX,
+ "rpmsg");
+ if (ret < 0) {
+ pr_err("rpmsg: failed to allocate char dev region\n");
+ return ret;
+ }
+
+ ret = register_rpmsg_driver(&rpmsg_raw_client);
+ if (ret < 0)
+ unregister_chrdev_region(rpmsg_raw_major, RPMSG_DEV_MAX);
+
+ return ret;
+}
+module_init(rpmsg_raw_init);
+
+static void rpmsg_raw_exit(void)
+{
+ unregister_rpmsg_driver(&rpmsg_raw_client);
+ unregister_chrdev_region(rpmsg_raw_major, RPMSG_DEV_MAX);
+}
+module_exit(rpmsg_raw_exit);
+
+MODULE_DESCRIPTION("RPMSG raw service driver");
+MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
+MODULE_ALIAS("rpmsg_raw");
+MODULE_LICENSE("GPL v2");
--
2.17.1