* [PATCH v4 01/16] rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 02/16] rpmsg: move RPMSG_ADDR_ANY in user API Arnaud Pouliquen
` (14 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
To be coherent with the other functions which are prefixed by
rpmsg_chrdev, rename the rpmsg_char_init function.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4bbbacdbf3bb..9e33b53bbf56 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -543,7 +543,7 @@ static struct rpmsg_driver rpmsg_chrdev_driver = {
},
};
-static int rpmsg_char_init(void)
+static int rpmsg_chrdev_init(void)
{
int ret;
@@ -569,7 +569,7 @@ static int rpmsg_char_init(void)
return ret;
}
-postcore_initcall(rpmsg_char_init);
+postcore_initcall(rpmsg_chrdev_init);
static void rpmsg_chrdev_exit(void)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 02/16] rpmsg: move RPMSG_ADDR_ANY in user API
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 01/16] rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 03/16] rpmsg: add short description of the IOCTL defined in UAPI Arnaud Pouliquen
` (13 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
As the RPMSG_ADDR_ANY is a valid src or dst address that can be set by
user applications, migrate its definition in user API.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
include/linux/rpmsg.h | 3 +--
include/uapi/linux/rpmsg.h | 2 ++
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index a5db828b2420..d97dcd049f18 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -18,8 +18,7 @@
#include <linux/mutex.h>
#include <linux/poll.h>
#include <linux/rpmsg/byteorder.h>
-
-#define RPMSG_ADDR_ANY 0xFFFFFFFF
+#include <uapi/linux/rpmsg.h>
struct rpmsg_device;
struct rpmsg_endpoint;
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..5e00748da319 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -9,6 +9,8 @@
#include <linux/ioctl.h>
#include <linux/types.h>
+#define RPMSG_ADDR_ANY 0xFFFFFFFF
+
/**
* struct rpmsg_endpoint_info - endpoint info representation
* @name: name of service
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 03/16] rpmsg: add short description of the IOCTL defined in UAPI.
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 01/16] rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 02/16] rpmsg: move RPMSG_ADDR_ANY in user API Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 04/16] rpmsg: char: export eptdev create an destroy functions Arnaud Pouliquen
` (12 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Add a description of the IOCTL and provide information on the default
value of the source and destination addresses.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
include/uapi/linux/rpmsg.h | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index 5e00748da319..f5ca8740f3fb 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -14,8 +14,8 @@
/**
* struct rpmsg_endpoint_info - endpoint info representation
* @name: name of service
- * @src: local address
- * @dst: destination address
+ * @src: local address. To set to RPMSG_ADDR_ANY if not used.
+ * @dst: destination address. To set to RPMSG_ADDR_ANY if not used.
*/
struct rpmsg_endpoint_info {
char name[32];
@@ -23,7 +23,14 @@ struct rpmsg_endpoint_info {
__u32 dst;
};
+/**
+ * Instantiate a new rmpsg char device endpoint.
+ */
#define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
+
+/**
+ * Destroy a rpmsg char device endpoint created by the RPMSG_CREATE_EPT_IOCTL.
+ */
#define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 04/16] rpmsg: char: export eptdev create an destroy functions
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (2 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 03/16] rpmsg: add short description of the IOCTL defined in UAPI Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 05/16] rpmsg: char: dissociate the control device from the rpmsg class Arnaud Pouliquen
` (11 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
To prepare the split code related to the control and the endpoint
devices in separate files:
- suppress the dependency with the rpmsg_ctrldev struct,
- rename and export the functions in rpmsg_char.h.
Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 22 ++++++++++------
drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 7 deletions(-)
create mode 100644 drivers/rpmsg/rpmsg_char.h
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 9e33b53bbf56..78a6d19fdf82 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (C) 2021, STMicroelectronics
* Copyright (c) 2016, Linaro Ltd.
* Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
* Copyright (c) 2012, PetaLogix
@@ -22,6 +23,7 @@
#include <linux/uaccess.h>
#include <uapi/linux/rpmsg.h>
+#include "rpmsg_char.h"
#include "rpmsg_internal.h"
#define RPMSG_DEV_MAX (MINORMASK + 1)
@@ -78,7 +80,7 @@ struct rpmsg_eptdev {
wait_queue_head_t readq;
};
-static int rpmsg_eptdev_destroy(struct device *dev, void *data)
+static int rpmsg_eptdev_destroy(struct device *dev)
{
struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
@@ -277,7 +279,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
if (cmd != RPMSG_DESTROY_EPT_IOCTL)
return -EINVAL;
- return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+ return rpmsg_eptdev_destroy(&eptdev->dev);
}
static const struct file_operations rpmsg_eptdev_fops = {
@@ -336,10 +338,15 @@ static void rpmsg_eptdev_release_device(struct device *dev)
kfree(eptdev);
}
-static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+ return rpmsg_eptdev_destroy(dev);
+}
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
+
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
struct rpmsg_channel_info chinfo)
{
- struct rpmsg_device *rpdev = ctrldev->rpdev;
struct rpmsg_eptdev *eptdev;
struct device *dev;
int ret;
@@ -359,7 +366,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
device_initialize(dev);
dev->class = rpmsg_class;
- dev->parent = &ctrldev->dev;
+ dev->parent = parent;
dev->groups = rpmsg_eptdev_groups;
dev_set_drvdata(dev, eptdev);
@@ -402,6 +409,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
return ret;
}
+EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
{
@@ -441,7 +449,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
chinfo.src = eptinfo.src;
chinfo.dst = eptinfo.dst;
- return rpmsg_eptdev_create(ctrldev, chinfo);
+ return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
};
static const struct file_operations rpmsg_ctrldev_fops = {
@@ -527,7 +535,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
int ret;
/* Destroy all endpoints */
- ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
+ ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
if (ret)
dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
new file mode 100644
index 000000000000..0feb3ea9445c
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) STMicroelectronics 2021.
+ */
+
+#ifndef __RPMSG_CHRDEV_H__
+#define __RPMSG_CHRDEV_H__
+
+#if IS_ENABLED(CONFIG_RPMSG_CHAR)
+/**
+ * rpmsg_chrdev_create_eptdev() - register char device based on an endpoint
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ * @parent: parent device
+ * @chinfo: assiated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo);
+
+/**
+ * rpmsg_chrdev_eptdev_destroy() - destroy created char device
+ * @data: parent device
+ * @chinfo: assiated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
+
+#else /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+static inline int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
+ struct device *parent,
+ struct rpmsg_channel_info chinfo)
+{
+ return -EINVAL;
+}
+
+static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+#endif /*__RPMSG_CHRDEV_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 05/16] rpmsg: char: dissociate the control device from the rpmsg class
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (3 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 04/16] rpmsg: char: export eptdev create an destroy functions Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl Arnaud Pouliquen
` (10 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
The RPMsg control device is a RPMsg device, it is already
referenced in the RPMsg bus. There is only an interest to
reference the ept char devices in the rpmsg class.
This patch prepares the code split of the control and end point
devices in two separate files.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 78a6d19fdf82..23e369a00531 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -485,7 +485,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
dev = &ctrldev->dev;
device_initialize(dev);
dev->parent = &rpdev->dev;
- dev->class = rpmsg_class;
cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
ctrldev->cdev.owner = THIS_MODULE;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (4 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 05/16] rpmsg: char: dissociate the control device from the rpmsg class Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 07/16] rpmsg: update rpmsg_chrdev_register_device function Arnaud Pouliquen
` (9 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Move the code related to the rpmsg_ctrl char device to the new
rpmsg_ctrl.c module.
Manage the dependency in the kconfig.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/Kconfig | 9 ++
drivers/rpmsg/Makefile | 1 +
drivers/rpmsg/rpmsg_char.c | 163 ----------------------------
drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
4 files changed, 226 insertions(+), 163 deletions(-)
create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0b4407abdf13..2d0cd7fdd710 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -10,11 +10,20 @@ config RPMSG_CHAR
tristate "RPMSG device interface"
depends on RPMSG
depends on NET
+ select RPMSG_CTRL
help
Say Y here to export rpmsg endpoints as device files, usually found
in /dev. They make it possible for user-space programs to send and
receive rpmsg packets.
+config RPMSG_CTRL
+ tristate "RPMSG control interface"
+ depends on RPMSG
+ help
+ Say Y here to enable the support of the /dev/rpmsg_ctlX API. This API
+ allows user-space programs to create endpoints with specific service name,
+ source and destination addresses.
+
config RPMSG_NS
tristate "RPMSG name service announcement"
depends on RPMSG
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..58e3b382e316 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_RPMSG) += rpmsg_core.o
obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o
+obj-$(CONFIG_RPMSG_CTRL) += rpmsg_ctrl.o
obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o
obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o
qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 23e369a00531..83c10b39b139 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -31,28 +31,12 @@
static dev_t rpmsg_major;
static struct class *rpmsg_class;
-static DEFINE_IDA(rpmsg_ctrl_ida);
static DEFINE_IDA(rpmsg_ept_ida);
static DEFINE_IDA(rpmsg_minor_ida);
#define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
#define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
-#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
-#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
-
-/**
- * struct rpmsg_ctrldev - control device for instantiating endpoint devices
- * @rpdev: underlaying rpmsg device
- * @cdev: cdev for the ctrl device
- * @dev: device for the ctrl device
- */
-struct rpmsg_ctrldev {
- struct rpmsg_device *rpdev;
- struct cdev cdev;
- struct device dev;
-};
-
/**
* struct rpmsg_eptdev - endpoint device context
* @dev: endpoint device
@@ -411,145 +395,6 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
}
EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
-static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
-{
- struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
- get_device(&ctrldev->dev);
- filp->private_data = ctrldev;
-
- return 0;
-}
-
-static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
-{
- struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
- put_device(&ctrldev->dev);
-
- return 0;
-}
-
-static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
- unsigned long arg)
-{
- struct rpmsg_ctrldev *ctrldev = fp->private_data;
- void __user *argp = (void __user *)arg;
- struct rpmsg_endpoint_info eptinfo;
- struct rpmsg_channel_info chinfo;
-
- if (cmd != RPMSG_CREATE_EPT_IOCTL)
- return -EINVAL;
-
- if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
- return -EFAULT;
-
- memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
- chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
- chinfo.src = eptinfo.src;
- chinfo.dst = eptinfo.dst;
-
- return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
-};
-
-static const struct file_operations rpmsg_ctrldev_fops = {
- .owner = THIS_MODULE,
- .open = rpmsg_ctrldev_open,
- .release = rpmsg_ctrldev_release,
- .unlocked_ioctl = rpmsg_ctrldev_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
-};
-
-static void rpmsg_ctrldev_release_device(struct device *dev)
-{
- struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
-
- ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
- ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
- cdev_del(&ctrldev->cdev);
- kfree(ctrldev);
-}
-
-static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
-{
- struct rpmsg_ctrldev *ctrldev;
- struct device *dev;
- int ret;
-
- ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
- if (!ctrldev)
- return -ENOMEM;
-
- ctrldev->rpdev = rpdev;
-
- dev = &ctrldev->dev;
- device_initialize(dev);
- dev->parent = &rpdev->dev;
-
- cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
- ctrldev->cdev.owner = THIS_MODULE;
-
- ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
- if (ret < 0)
- goto free_ctrldev;
- dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-
- ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
- if (ret < 0)
- goto free_minor_ida;
- dev->id = ret;
- dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
-
- ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
- if (ret)
- goto free_ctrl_ida;
-
- /* We can now rely on the release function for cleanup */
- dev->release = rpmsg_ctrldev_release_device;
-
- ret = device_add(dev);
- if (ret) {
- dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
- put_device(dev);
- }
-
- dev_set_drvdata(&rpdev->dev, ctrldev);
-
- return ret;
-
-free_ctrl_ida:
- ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
-free_minor_ida:
- ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
-free_ctrldev:
- put_device(dev);
- kfree(ctrldev);
-
- return ret;
-}
-
-static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
-{
- struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
- int ret;
-
- /* Destroy all endpoints */
- ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
- if (ret)
- dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
-
- device_del(&ctrldev->dev);
- put_device(&ctrldev->dev);
-}
-
-static struct rpmsg_driver rpmsg_chrdev_driver = {
- .probe = rpmsg_chrdev_probe,
- .remove = rpmsg_chrdev_remove,
- .drv = {
- .name = "rpmsg_chrdev",
- },
-};
-
static int rpmsg_chrdev_init(void)
{
int ret;
@@ -567,20 +412,12 @@ static int rpmsg_chrdev_init(void)
return PTR_ERR(rpmsg_class);
}
- ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
- if (ret < 0) {
- pr_err("rpmsgchr: failed to register rpmsg driver\n");
- class_destroy(rpmsg_class);
- unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
- }
-
return ret;
}
postcore_initcall(rpmsg_chrdev_init);
static void rpmsg_chrdev_exit(void)
{
- unregister_rpmsg_driver(&rpmsg_chrdev_driver);
class_destroy(rpmsg_class);
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
new file mode 100644
index 000000000000..fa05b67d24da
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021, STMicroelectronics
+ * Copyright (c) 2016, Linaro Ltd.
+ * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
+ * Copyright (c) 2012, PetaLogix
+ * Copyright (c) 2011, Texas Instruments, Inc.
+ * Copyright (c) 2011, Google, Inc.
+ *
+ * Based on rpmsg performance statistics driver by Michal Simek, which in turn
+ * was based on TI & Google OMX rpmsg 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/rpmsg.h>
+#include <linux/slab.h>
+#include <uapi/linux/rpmsg.h>
+
+#include "rpmsg_char.h"
+#include "rpmsg_internal.h"
+
+#define RPMSG_DEV_MAX (MINORMASK + 1)
+
+static dev_t rpmsg_major;
+
+static DEFINE_IDA(rpmsg_ctrl_ida);
+static DEFINE_IDA(rpmsg_minor_ida);
+
+#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl, dev)
+#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl, cdev)
+
+/**
+ * struct rpmsg_ctrl - control device for instantiating endpoint devices
+ * @rpdev: underlaying rpmsg device
+ * @cdev: cdev for the ctrl device
+ * @dev: device for the ctrl device
+ */
+struct rpmsg_ctrl {
+ struct rpmsg_device *rpdev;
+ struct cdev cdev;
+ struct device dev;
+};
+
+static int rpmsg_ctrl_open(struct inode *inode, struct file *filp)
+{
+ struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+ get_device(&ctrldev->dev);
+ filp->private_data = ctrldev;
+
+ return 0;
+}
+
+static int rpmsg_ctrl_release(struct inode *inode, struct file *filp)
+{
+ struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+ put_device(&ctrldev->dev);
+
+ return 0;
+}
+
+static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ struct rpmsg_ctrl *ctrldev = fp->private_data;
+ void __user *argp = (void __user *)arg;
+ struct rpmsg_endpoint_info eptinfo;
+ struct rpmsg_channel_info chinfo;
+
+ if (cmd != RPMSG_CREATE_EPT_IOCTL)
+ return -EINVAL;
+
+ if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+ return -EFAULT;
+
+ memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+ chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+ chinfo.src = eptinfo.src;
+ chinfo.dst = eptinfo.dst;
+
+ return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+};
+
+static const struct file_operations rpmsg_ctrl_fops = {
+ .owner = THIS_MODULE,
+ .open = rpmsg_ctrl_open,
+ .release = rpmsg_ctrl_release,
+ .unlocked_ioctl = rpmsg_ctrl_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+};
+
+static void rpmsg_ctrl_release_device(struct device *dev)
+{
+ struct rpmsg_ctrl *ctrldev = dev_to_ctrldev(dev);
+
+ ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+ ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+ cdev_del(&ctrldev->cdev);
+ kfree(ctrldev);
+}
+
+static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_ctrl *ctrldev;
+ struct device *dev;
+ int ret;
+
+ ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
+ if (!ctrldev)
+ return -ENOMEM;
+
+ ctrldev->rpdev = rpdev;
+
+ dev = &ctrldev->dev;
+ device_initialize(dev);
+ dev->parent = &rpdev->dev;
+
+ cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
+ ctrldev->cdev.owner = THIS_MODULE;
+
+ ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
+ if (ret < 0)
+ goto free_ctrldev;
+ dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
+ ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto free_minor_ida;
+ dev->id = ret;
+ dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
+
+ ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
+ if (ret)
+ goto free_ctrl_ida;
+
+ /* We can now rely on the release function for cleanup */
+ dev->release = rpmsg_ctrl_release_device;
+
+ ret = device_add(dev);
+ if (ret) {
+ dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
+ put_device(dev);
+ }
+
+ dev_set_drvdata(&rpdev->dev, ctrldev);
+
+ return ret;
+
+free_ctrl_ida:
+ ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+free_minor_ida:
+ ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+free_ctrldev:
+ put_device(dev);
+ kfree(ctrldev);
+
+ return ret;
+}
+
+static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_ctrl *ctrldev = dev_get_drvdata(&rpdev->dev);
+ int ret;
+
+ /* Destroy all endpoints */
+ ret = device_for_each_child(&ctrldev->dev, NULL,
+ rpmsg_chrdev_eptdev_destroy);
+ if (ret)
+ dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
+
+ device_del(&ctrldev->dev);
+ put_device(&ctrldev->dev);
+}
+
+static struct rpmsg_driver rpmsg_ctrl_driver = {
+ .probe = rpmsg_ctrl_probe,
+ .remove = rpmsg_ctrl_remove,
+ .drv = {
+ .name = "rpmsg_chrdev",
+ },
+};
+
+static int rpmsg_ctrl_init(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&rpmsg_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_ctrl_driver);
+ if (ret < 0) {
+ pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
+ unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+ }
+
+ return ret;
+}
+postcore_initcall(rpmsg_ctrl_init);
+
+static void rpmsg_ctrl_exit(void)
+{
+ unregister_rpmsg_driver(&rpmsg_ctrl_driver);
+ unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+}
+module_exit(rpmsg_ctrl_exit);
+
+MODULE_DESCRIPTION("rpmsg control interface");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 07/16] rpmsg: update rpmsg_chrdev_register_device function
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (5 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 08/16] rpmsg: glink: add sendto and trysendto ops Arnaud Pouliquen
` (8 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
As driver is now the rpmsg_ioctl, rename the function.
In addition, initialize the rpdev addresses to RPMSG_ADDR_ANY as not
defined.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/qcom_glink_native.c | 2 +-
drivers/rpmsg/qcom_smd.c | 2 +-
drivers/rpmsg/rpmsg_ctrl.c | 2 +-
drivers/rpmsg/rpmsg_internal.h | 10 ++++++----
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 27a05167c18c..d4e4dd482614 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1625,7 +1625,7 @@ static int qcom_glink_create_chrdev(struct qcom_glink *glink)
rpdev->dev.parent = glink->dev;
rpdev->dev.release = qcom_glink_device_release;
- return rpmsg_chrdev_register_device(rpdev);
+ return rpmsg_ctrl_register_device(rpdev);
}
struct qcom_glink *qcom_glink_native_probe(struct device *dev,
diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 19903de6268d..40a1c415c775 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1097,7 +1097,7 @@ static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)
qsdev->rpdev.dev.parent = &edge->dev;
qsdev->rpdev.dev.release = qcom_smd_release_device;
- return rpmsg_chrdev_register_device(&qsdev->rpdev);
+ return rpmsg_ctrl_register_device(&qsdev->rpdev);
}
/*
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index fa05b67d24da..2e43b4096aa8 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -180,7 +180,7 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
.probe = rpmsg_ctrl_probe,
.remove = rpmsg_ctrl_remove,
.drv = {
- .name = "rpmsg_chrdev",
+ .name = KBUILD_MODNAME,
},
};
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index a76c344253bf..7428f4465d17 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -82,16 +82,18 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
int rpmsg_release_channel(struct rpmsg_device *rpdev,
struct rpmsg_channel_info *chinfo);
/**
- * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
+ * rpmsg_ctrl_register_device() - register a char device for control based on rpdev
* @rpdev: prepared rpdev to be used for creating endpoints
*
* This function wraps rpmsg_register_device() preparing the rpdev for use as
* basis for the rpmsg chrdev.
*/
-static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
+static inline int rpmsg_ctrl_register_device(struct rpmsg_device *rpdev)
{
- strcpy(rpdev->id.name, "rpmsg_chrdev");
- rpdev->driver_override = "rpmsg_chrdev";
+ strcpy(rpdev->id.name, "rpmsg_ctrl");
+ rpdev->driver_override = "rpmsg_ctrl";
+ rpdev->src = RPMSG_ADDR_ANY;
+ rpdev->dst = RPMSG_ADDR_ANY;
return rpmsg_register_device(rpdev);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 08/16] rpmsg: glink: add sendto and trysendto ops
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (6 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 07/16] rpmsg: update rpmsg_chrdev_register_device function Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 09/16] rpmsg: smd: " Arnaud Pouliquen
` (7 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Implement the sendto ops to support the future rpmsg_char update for the
vitio backend support.
The use of sendto in rpmsg_char is needed as a destination address is
requested at least by the virtio backend.
The glink implementation does not need a destination address so ignores it.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/qcom_glink_native.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index d4e4dd482614..ae2c03b59c55 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1332,6 +1332,20 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
return __qcom_glink_send(channel, data, len, false);
}
+static int qcom_glink_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+ struct glink_channel *channel = to_glink_channel(ept);
+
+ return __qcom_glink_send(channel, data, len, true);
+}
+
+static int qcom_glink_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+ struct glink_channel *channel = to_glink_channel(ept);
+
+ return __qcom_glink_send(channel, data, len, false);
+}
+
/*
* Finds the device_node for the glink child interested in this channel.
*/
@@ -1364,7 +1378,9 @@ static const struct rpmsg_device_ops glink_device_ops = {
static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
.destroy_ept = qcom_glink_destroy_ept,
.send = qcom_glink_send,
+ .sendto = qcom_glink_sendto,
.trysend = qcom_glink_trysend,
+ .trysendto = qcom_glink_trysendto,
};
static void qcom_glink_rpdev_release(struct device *dev)
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 09/16] rpmsg: smd: add sendto and trysendto ops
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (7 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 08/16] rpmsg: glink: add sendto and trysendto ops Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:28 ` [PATCH v4 10/16] rpmsg: char: use sendto to specify the message destination address Arnaud Pouliquen
` (6 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Implement the sendto ops to support the future rpmsg_char update for the
vitio backend support.
The use of sendto in rpmsg_char is needed as a destination address is
requested at least by the virtio backend.
The SMD implementation does not need a destination address so ignores it.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/qcom_smd.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 40a1c415c775..2d279c03a090 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -974,6 +974,20 @@ static int qcom_smd_trysend(struct rpmsg_endpoint *ept, void *data, int len)
return __qcom_smd_send(qsept->qsch, data, len, false);
}
+static int qcom_smd_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+ struct qcom_smd_endpoint *qsept = to_smd_endpoint(ept);
+
+ return __qcom_smd_send(qsept->qsch, data, len, true);
+}
+
+static int qcom_smd_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+ struct qcom_smd_endpoint *qsept = to_smd_endpoint(ept);
+
+ return __qcom_smd_send(qsept->qsch, data, len, false);
+}
+
static __poll_t qcom_smd_poll(struct rpmsg_endpoint *ept,
struct file *filp, poll_table *wait)
{
@@ -1038,7 +1052,9 @@ static const struct rpmsg_device_ops qcom_smd_device_ops = {
static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
.destroy_ept = qcom_smd_destroy_ept,
.send = qcom_smd_send,
+ .sendto = qcom_smd_sendto,
.trysend = qcom_smd_trysend,
+ .trysendto = qcom_smd_trysendto,
.poll = qcom_smd_poll,
};
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 10/16] rpmsg: char: use sendto to specify the message destination address
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (8 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 09/16] rpmsg: smd: " Arnaud Pouliquen
@ 2021-02-17 13:28 ` Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
` (5 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
When the endpoint device is created by the application a destination
address as been specified in the rpmsg_channel_info structure.
Send the message to this address instead of the default one.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 83c10b39b139..09ae1304837c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -225,9 +225,9 @@ static ssize_t rpmsg_eptdev_write_iter(struct kiocb *iocb,
}
if (filp->f_flags & O_NONBLOCK)
- ret = rpmsg_trysend(eptdev->ept, kbuf, len);
+ ret = rpmsg_trysendto(eptdev->ept, kbuf, len, eptdev->chinfo.dst);
else
- ret = rpmsg_send(eptdev->ept, kbuf, len);
+ ret = rpmsg_sendto(eptdev->ept, kbuf, len, eptdev->chinfo.dst);
unlock_eptdev:
mutex_unlock(&eptdev->ept_lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (9 preceding siblings ...)
2021-02-17 13:28 ` [PATCH v4 10/16] rpmsg: char: use sendto to specify the message destination address Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
2021-02-18 12:27 ` Dan Carpenter
2021-02-17 13:29 ` [PATCH v4 12/16] rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL Arnaud Pouliquen
` (4 subsequent siblings)
15 siblings, 1 reply; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
to create RPMsg chdev endpoints.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
V3:
Fix compilation issue
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:978 rpmsg_probe() error: uninitialized symbol 'vch'.
drivers/rpmsg/virtio_rpmsg_bus.c:979 rpmsg_probe() error: uninitialized symbol 'rpdev_ctrl'.
---
drivers/rpmsg/virtio_rpmsg_bus.c | 37 +++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e87d4cf926eb..5143fdeca306 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -813,6 +813,35 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
wake_up_interruptible(&vrp->sendq);
}
+static struct rpmsg_device *rpmsg_virtio_add_char_dev(struct virtio_device *vdev)
+{
+ struct virtproc_info *vrp = vdev->priv;
+ struct virtio_rpmsg_channel *vch;
+ struct rpmsg_device *rpdev_ctrl;
+ int err = 0;
+
+ vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+ if (!vch)
+ return ERR_PTR(-ENOMEM);
+
+ /* Link the channel to the vrp */
+ vch->vrp = vrp;
+
+ /* Assign public information to the rpmsg_device */
+ rpdev_ctrl = &vch->rpdev;
+ rpdev_ctrl->ops = &virtio_rpmsg_ops;
+
+ rpdev_ctrl->dev.parent = &vrp->vdev->dev;
+ rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
+ rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
+
+ err = rpmsg_ctrl_register_device(rpdev_ctrl);
+ if (err)
+ return ERR_PTR(err);
+
+ return rpdev_ctrl;
+}
+
static int rpmsg_probe(struct virtio_device *vdev)
{
vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -820,7 +849,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
struct virtqueue *vqs[2];
struct virtproc_info *vrp;
struct virtio_rpmsg_channel *vch;
- struct rpmsg_device *rpdev_ns;
+ struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
void *bufs_va;
int err = 0, i;
size_t total_buf_space;
@@ -918,6 +947,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
goto free_coherent;
}
+ rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
+ if (IS_ERR(rpdev_ctrl)) {
+ err = PTR_ERR(rpdev_ctrl);
+ goto free_coherent;
+ }
/*
* Prepare to kick but don't notify yet - we can't do this before
* device is ready.
@@ -941,6 +975,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
free_coherent:
kfree(vch);
+ kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
2021-02-18 12:27 ` Dan Carpenter
@ 2021-02-18 12:27 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
To: kbuild, Arnaud Pouliquen, Bjorn Andersson, Ohad Ben-Cohen,
Mathieu Poirier, Andy Gross
Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
linux-arm-msm, arnaud.pouliquen
[-- Attachment #1: Type: text/plain, Size: 13919 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: x86_64-randconfig-m001-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.
vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 845 static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 846 {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 847 vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3c Stefan Hajnoczi 2015-12-17 848 static const char * const names[] = { "input", "output" };
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 849 struct virtqueue *vqs[2];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 850 struct virtproc_info *vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 851 struct virtio_rpmsg_channel *vch;
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 852 struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 853 void *bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 854 int err = 0, i;
b1b9891441fa33 Suman Anna 2014-09-16 855 size_t total_buf_space;
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 856 bool notify;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 857
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 858 vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
You might want to consider updating this stuff to devm_kzalloc() which
is trendy with the kids these days. It's cleaned up automatically when
the module is released.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 859 if (!vrp)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 860 return -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 861
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 862 vrp->vdev = vdev;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 863
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 864 idr_init(&vrp->endpoints);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 865 mutex_init(&vrp->endpoints_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 866 mutex_init(&vrp->tx_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 867 init_waitqueue_head(&vrp->sendq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 868
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 869 /* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb2275884 Michael S. Tsirkin 2017-03-06 870 err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 871 if (err)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 872 goto free_vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 873
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 874 vrp->rvq = vqs[0];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 875 vrp->svq = vqs[1];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 876
b1b9891441fa33 Suman Anna 2014-09-16 877 /* we expect symmetric tx/rx vrings */
b1b9891441fa33 Suman Anna 2014-09-16 878 WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33 Suman Anna 2014-09-16 879 virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33 Suman Anna 2014-09-16 880
b1b9891441fa33 Suman Anna 2014-09-16 881 /* we need less buffers if vrings are small */
b1b9891441fa33 Suman Anna 2014-09-16 882 if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33 Suman Anna 2014-09-16 883 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33 Suman Anna 2014-09-16 884 else
b1b9891441fa33 Suman Anna 2014-09-16 885 vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33 Suman Anna 2014-09-16 886
f93848f9eeb0f8 Loic Pallardy 2017-03-28 887 vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 888
f93848f9eeb0f8 Loic Pallardy 2017-03-28 889 total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33 Suman Anna 2014-09-16 890
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 891 /* allocate coherent memory for the buffers */
d999b622fcfb39 Loic Pallardy 2019-01-10 892 bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33 Suman Anna 2014-09-16 893 total_buf_space, &vrp->bufs_dma,
b1b9891441fa33 Suman Anna 2014-09-16 894 GFP_KERNEL);
3119b487e03650 Wei Yongjun 2013-04-29 895 if (!bufs_va) {
3119b487e03650 Wei Yongjun 2013-04-29 896 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 897 goto vqs_del;
3119b487e03650 Wei Yongjun 2013-04-29 898 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 899
de4064af76537f Suman Anna 2018-10-23 900 dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b1 Anna, Suman 2016-08-12 901 bufs_va, &vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 902
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 903 /* half of the buffers is dedicated for RX */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 904 vrp->rbufs = bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 905
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 906 /* and half is dedicated for TX */
b1b9891441fa33 Suman Anna 2014-09-16 907 vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 908
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 909 /* set up the receive buffers */
b1b9891441fa33 Suman Anna 2014-09-16 910 for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 911 struct scatterlist sg;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 912 void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 913
9dd87c2af651b0 Loic Pallardy 2017-03-28 914 rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 915
cee51d69a45b6c Rusty Russell 2013-03-20 916 err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 917 GFP_KERNEL);
57e1a37347d31c Rusty Russell 2012-10-16 918 WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 919 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 920
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 921 /* suppress "tx-complete" interrupts */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 922 virtqueue_disable_cb(vrp->svq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 923
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 924 vdev->priv = vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 925
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 926 /* if supported by the remote processor, enable the name service */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 927 if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 928 vch = kzalloc(sizeof(*vch), GFP_KERNEL);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 929 if (!vch) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 930 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 931 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 932 }
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 933
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 934 /* Link the channel to our vrp */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 935 vch->vrp = vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 936
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 937 /* Assign public information to the rpmsg_device */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 938 rpdev_ns = &vch->rpdev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 939 rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 940 rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 941
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 942 rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 943 rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 944
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 945 err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 946 if (err)
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 947 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 948 }
"vch" not initialized on else path. Also keep in mind that "rpdev_ctrl"
is NULL at this point.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 949
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 950 rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 951 if (IS_ERR(rpdev_ctrl)) {
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 952 err = PTR_ERR(rpdev_ctrl);
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 953 goto free_coherent;
I should create a Smatch warning for this as well.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 954 }
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 955 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 956 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 957 * device is ready.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 958 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 959 notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 960
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 961 /* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 962 virtio_device_ready(vdev);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 963
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 964 /* tell the remote processor it can start sending messages */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 965 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 966 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 967 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 968 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 969 if (notify)
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 970 virtqueue_notify(vrp->rvq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 971
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 972 dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 973
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 974 return 0;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 975
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 976 free_coherent:
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 @977 kfree(vch);
^^^^^^^^^^
Uninitalized.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 978 kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
Now, let's say that "rpdev_ctrl" is NULL. That's fine because
to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
is a no-op. But why even have the to_virtio_rpmsg_channel() function
if we are going to rely on it being a no-op?
If "rpdev_ctrl" is an error pointer this will crash. The problem is we
are freeing stuff that was not allocated. The fix for this is:
1) Free the most recent successful allocation.
2) The unwind code should mirror the allocation code.
3) Choose label names which say what the goto does. If the most recent
allocation was "vch" the goto should be "goto free_vch;"
4) Every allocation function should have a matching free function. It's
a layering violation to have to know that the internals of
rpmsg_virtio_add_char_dev().
So create function:
void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
{
if (!rpdev_ctrl)
return;
kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
}
The clean up code looks like this:
return 0;
free_vch:
if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
kfree(vch);
free_ctrl:
rpmsg_virtio_del_char_dev(rpdev_ctrl);
free_coherent:
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:
vdev->config->del_vqs(vrp->vdev);
Then just go through the function and as you read down the code keep
track of the most recent successful allocation and goto the correct
free label.
d999b622fcfb39 Loic Pallardy 2019-01-10 979 dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29 980 bufs_va, vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 981 vqs_del:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 982 vdev->config->del_vqs(vrp->vdev);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 983 free_vrp:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 984 kfree(vrp);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 985 return err;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 986 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
@ 2021-02-18 12:27 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 14145 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: x86_64-randconfig-m001-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.
vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 845 static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 846 {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 847 vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3c Stefan Hajnoczi 2015-12-17 848 static const char * const names[] = { "input", "output" };
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 849 struct virtqueue *vqs[2];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 850 struct virtproc_info *vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 851 struct virtio_rpmsg_channel *vch;
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 852 struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 853 void *bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 854 int err = 0, i;
b1b9891441fa33 Suman Anna 2014-09-16 855 size_t total_buf_space;
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 856 bool notify;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 857
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 858 vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
You might want to consider updating this stuff to devm_kzalloc() which
is trendy with the kids these days. It's cleaned up automatically when
the module is released.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 859 if (!vrp)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 860 return -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 861
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 862 vrp->vdev = vdev;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 863
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 864 idr_init(&vrp->endpoints);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 865 mutex_init(&vrp->endpoints_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 866 mutex_init(&vrp->tx_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 867 init_waitqueue_head(&vrp->sendq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 868
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 869 /* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb2275884 Michael S. Tsirkin 2017-03-06 870 err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 871 if (err)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 872 goto free_vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 873
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 874 vrp->rvq = vqs[0];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 875 vrp->svq = vqs[1];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 876
b1b9891441fa33 Suman Anna 2014-09-16 877 /* we expect symmetric tx/rx vrings */
b1b9891441fa33 Suman Anna 2014-09-16 878 WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33 Suman Anna 2014-09-16 879 virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33 Suman Anna 2014-09-16 880
b1b9891441fa33 Suman Anna 2014-09-16 881 /* we need less buffers if vrings are small */
b1b9891441fa33 Suman Anna 2014-09-16 882 if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33 Suman Anna 2014-09-16 883 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33 Suman Anna 2014-09-16 884 else
b1b9891441fa33 Suman Anna 2014-09-16 885 vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33 Suman Anna 2014-09-16 886
f93848f9eeb0f8 Loic Pallardy 2017-03-28 887 vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 888
f93848f9eeb0f8 Loic Pallardy 2017-03-28 889 total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33 Suman Anna 2014-09-16 890
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 891 /* allocate coherent memory for the buffers */
d999b622fcfb39 Loic Pallardy 2019-01-10 892 bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33 Suman Anna 2014-09-16 893 total_buf_space, &vrp->bufs_dma,
b1b9891441fa33 Suman Anna 2014-09-16 894 GFP_KERNEL);
3119b487e03650 Wei Yongjun 2013-04-29 895 if (!bufs_va) {
3119b487e03650 Wei Yongjun 2013-04-29 896 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 897 goto vqs_del;
3119b487e03650 Wei Yongjun 2013-04-29 898 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 899
de4064af76537f Suman Anna 2018-10-23 900 dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b1 Anna, Suman 2016-08-12 901 bufs_va, &vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 902
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 903 /* half of the buffers is dedicated for RX */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 904 vrp->rbufs = bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 905
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 906 /* and half is dedicated for TX */
b1b9891441fa33 Suman Anna 2014-09-16 907 vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 908
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 909 /* set up the receive buffers */
b1b9891441fa33 Suman Anna 2014-09-16 910 for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 911 struct scatterlist sg;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 912 void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 913
9dd87c2af651b0 Loic Pallardy 2017-03-28 914 rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 915
cee51d69a45b6c Rusty Russell 2013-03-20 916 err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 917 GFP_KERNEL);
57e1a37347d31c Rusty Russell 2012-10-16 918 WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 919 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 920
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 921 /* suppress "tx-complete" interrupts */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 922 virtqueue_disable_cb(vrp->svq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 923
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 924 vdev->priv = vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 925
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 926 /* if supported by the remote processor, enable the name service */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 927 if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 928 vch = kzalloc(sizeof(*vch), GFP_KERNEL);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 929 if (!vch) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 930 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 931 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 932 }
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 933
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 934 /* Link the channel to our vrp */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 935 vch->vrp = vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 936
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 937 /* Assign public information to the rpmsg_device */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 938 rpdev_ns = &vch->rpdev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 939 rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 940 rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 941
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 942 rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 943 rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 944
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 945 err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 946 if (err)
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 947 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 948 }
"vch" not initialized on else path. Also keep in mind that "rpdev_ctrl"
is NULL at this point.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 949
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 950 rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 951 if (IS_ERR(rpdev_ctrl)) {
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 952 err = PTR_ERR(rpdev_ctrl);
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 953 goto free_coherent;
I should create a Smatch warning for this as well.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 954 }
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 955 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 956 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 957 * device is ready.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 958 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 959 notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 960
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 961 /* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 962 virtio_device_ready(vdev);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 963
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 964 /* tell the remote processor it can start sending messages */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 965 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 966 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 967 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 968 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 969 if (notify)
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 970 virtqueue_notify(vrp->rvq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 971
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 972 dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 973
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 974 return 0;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 975
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 976 free_coherent:
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 @977 kfree(vch);
^^^^^^^^^^
Uninitalized.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 978 kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
Now, let's say that "rpdev_ctrl" is NULL. That's fine because
to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
is a no-op. But why even have the to_virtio_rpmsg_channel() function
if we are going to rely on it being a no-op?
If "rpdev_ctrl" is an error pointer this will crash. The problem is we
are freeing stuff that was not allocated. The fix for this is:
1) Free the most recent successful allocation.
2) The unwind code should mirror the allocation code.
3) Choose label names which say what the goto does. If the most recent
allocation was "vch" the goto should be "goto free_vch;"
4) Every allocation function should have a matching free function. It's
a layering violation to have to know that the internals of
rpmsg_virtio_add_char_dev().
So create function:
void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
{
if (!rpdev_ctrl)
return;
kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
}
The clean up code looks like this:
return 0;
free_vch:
if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
kfree(vch);
free_ctrl:
rpmsg_virtio_del_char_dev(rpdev_ctrl);
free_coherent:
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:
vdev->config->del_vqs(vrp->vdev);
Then just go through the function and as you read down the code keep
track of the most recent successful allocation and goto the correct
free label.
d999b622fcfb39 Loic Pallardy 2019-01-10 979 dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29 980 bufs_va, vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 981 vqs_del:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 982 vdev->config->del_vqs(vrp->vdev);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 983 free_vrp:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 984 kfree(vrp);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 985 return err;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 986 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
@ 2021-02-18 12:27 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 14145 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: x86_64-randconfig-m001-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.
vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 845 static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 846 {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 847 vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3c Stefan Hajnoczi 2015-12-17 848 static const char * const names[] = { "input", "output" };
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 849 struct virtqueue *vqs[2];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 850 struct virtproc_info *vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 851 struct virtio_rpmsg_channel *vch;
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 852 struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 853 void *bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 854 int err = 0, i;
b1b9891441fa33 Suman Anna 2014-09-16 855 size_t total_buf_space;
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 856 bool notify;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 857
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 858 vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
You might want to consider updating this stuff to devm_kzalloc() which
is trendy with the kids these days. It's cleaned up automatically when
the module is released.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 859 if (!vrp)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 860 return -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 861
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 862 vrp->vdev = vdev;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 863
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 864 idr_init(&vrp->endpoints);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 865 mutex_init(&vrp->endpoints_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 866 mutex_init(&vrp->tx_lock);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 867 init_waitqueue_head(&vrp->sendq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 868
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 869 /* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb2275884 Michael S. Tsirkin 2017-03-06 870 err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 871 if (err)
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 872 goto free_vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 873
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 874 vrp->rvq = vqs[0];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 875 vrp->svq = vqs[1];
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 876
b1b9891441fa33 Suman Anna 2014-09-16 877 /* we expect symmetric tx/rx vrings */
b1b9891441fa33 Suman Anna 2014-09-16 878 WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33 Suman Anna 2014-09-16 879 virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33 Suman Anna 2014-09-16 880
b1b9891441fa33 Suman Anna 2014-09-16 881 /* we need less buffers if vrings are small */
b1b9891441fa33 Suman Anna 2014-09-16 882 if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33 Suman Anna 2014-09-16 883 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33 Suman Anna 2014-09-16 884 else
b1b9891441fa33 Suman Anna 2014-09-16 885 vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33 Suman Anna 2014-09-16 886
f93848f9eeb0f8 Loic Pallardy 2017-03-28 887 vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 888
f93848f9eeb0f8 Loic Pallardy 2017-03-28 889 total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33 Suman Anna 2014-09-16 890
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 891 /* allocate coherent memory for the buffers */
d999b622fcfb39 Loic Pallardy 2019-01-10 892 bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33 Suman Anna 2014-09-16 893 total_buf_space, &vrp->bufs_dma,
b1b9891441fa33 Suman Anna 2014-09-16 894 GFP_KERNEL);
3119b487e03650 Wei Yongjun 2013-04-29 895 if (!bufs_va) {
3119b487e03650 Wei Yongjun 2013-04-29 896 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 897 goto vqs_del;
3119b487e03650 Wei Yongjun 2013-04-29 898 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 899
de4064af76537f Suman Anna 2018-10-23 900 dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b1 Anna, Suman 2016-08-12 901 bufs_va, &vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 902
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 903 /* half of the buffers is dedicated for RX */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 904 vrp->rbufs = bufs_va;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 905
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 906 /* and half is dedicated for TX */
b1b9891441fa33 Suman Anna 2014-09-16 907 vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 908
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 909 /* set up the receive buffers */
b1b9891441fa33 Suman Anna 2014-09-16 910 for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 911 struct scatterlist sg;
f93848f9eeb0f8 Loic Pallardy 2017-03-28 912 void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 913
9dd87c2af651b0 Loic Pallardy 2017-03-28 914 rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 915
cee51d69a45b6c Rusty Russell 2013-03-20 916 err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 917 GFP_KERNEL);
57e1a37347d31c Rusty Russell 2012-10-16 918 WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 919 }
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 920
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 921 /* suppress "tx-complete" interrupts */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 922 virtqueue_disable_cb(vrp->svq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 923
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 924 vdev->priv = vrp;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 925
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 926 /* if supported by the remote processor, enable the name service */
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 927 if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 928 vch = kzalloc(sizeof(*vch), GFP_KERNEL);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 929 if (!vch) {
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 930 err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 931 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 932 }
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 933
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 934 /* Link the channel to our vrp */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 935 vch->vrp = vrp;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 936
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 937 /* Assign public information to the rpmsg_device */
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 938 rpdev_ns = &vch->rpdev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 939 rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 940 rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 941
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 942 rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 943 rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 944
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 945 err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 946 if (err)
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 947 goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 948 }
"vch" not initialized on else path. Also keep in mind that "rpdev_ctrl"
is NULL at this point.
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 949
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 950 rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 951 if (IS_ERR(rpdev_ctrl)) {
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 952 err = PTR_ERR(rpdev_ctrl);
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 953 goto free_coherent;
I should create a Smatch warning for this as well.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 954 }
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 955 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 956 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 957 * device is ready.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 958 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 959 notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 960
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 961 /* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 962 virtio_device_ready(vdev);
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 963
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 964 /* tell the remote processor it can start sending messages */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 965 /*
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 966 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 967 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 968 */
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 969 if (notify)
71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 970 virtqueue_notify(vrp->rvq);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 971
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 972 dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 973
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 974 return 0;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 975
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 976 free_coherent:
950a7388f02bf7 Arnaud Pouliquen 2020-11-20 @977 kfree(vch);
^^^^^^^^^^
Uninitalized.
e3bba4363fe87b Arnaud Pouliquen 2021-02-17 978 kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
Now, let's say that "rpdev_ctrl" is NULL. That's fine because
to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
is a no-op. But why even have the to_virtio_rpmsg_channel() function
if we are going to rely on it being a no-op?
If "rpdev_ctrl" is an error pointer this will crash. The problem is we
are freeing stuff that was not allocated. The fix for this is:
1) Free the most recent successful allocation.
2) The unwind code should mirror the allocation code.
3) Choose label names which say what the goto does. If the most recent
allocation was "vch" the goto should be "goto free_vch;"
4) Every allocation function should have a matching free function. It's
a layering violation to have to know that the internals of
rpmsg_virtio_add_char_dev().
So create function:
void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
{
if (!rpdev_ctrl)
return;
kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
}
The clean up code looks like this:
return 0;
free_vch:
if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
kfree(vch);
free_ctrl:
rpmsg_virtio_del_char_dev(rpdev_ctrl);
free_coherent:
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:
vdev->config->del_vqs(vrp->vdev);
Then just go through the function and as you read down the code keep
track of the most recent successful allocation and goto the correct
free label.
d999b622fcfb39 Loic Pallardy 2019-01-10 979 dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29 980 bufs_va, vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 981 vqs_del:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 982 vdev->config->del_vqs(vrp->vdev);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 983 free_vrp:
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 984 kfree(vrp);
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 985 return err;
bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 986 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
2021-02-18 12:27 ` Dan Carpenter
(?)
(?)
@ 2021-02-18 17:14 ` Arnaud POULIQUEN
-1 siblings, 0 replies; 26+ messages in thread
From: Arnaud POULIQUEN @ 2021-02-18 17:14 UTC (permalink / raw)
To: Dan Carpenter, kbuild, Bjorn Andersson, Ohad Ben-Cohen,
Mathieu Poirier, Andy Gross
Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
linux-arm-msm
Hi Dan,
On 2/18/21 1:27 PM, Dan Carpenter wrote:
> Hi Arnaud,
>
> url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
> config: x86_64-randconfig-m001-20210215 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.
>
> vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c
>
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 845 static int rpmsg_probe(struct virtio_device *vdev)
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 846 {
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 847 vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> f7ad26ff952b3c Stefan Hajnoczi 2015-12-17 848 static const char * const names[] = { "input", "output" };
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 849 struct virtqueue *vqs[2];
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 850 struct virtproc_info *vrp;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 851 struct virtio_rpmsg_channel *vch;
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 852 struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 853 void *bufs_va;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 854 int err = 0, i;
> b1b9891441fa33 Suman Anna 2014-09-16 855 size_t total_buf_space;
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 856 bool notify;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 857
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 858 vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
>
> You might want to consider updating this stuff to devm_kzalloc() which
> is trendy with the kids these days. It's cleaned up automatically when
> the module is released.
>
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 859 if (!vrp)
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 860 return -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 861
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 862 vrp->vdev = vdev;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 863
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 864 idr_init(&vrp->endpoints);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 865 mutex_init(&vrp->endpoints_lock);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 866 mutex_init(&vrp->tx_lock);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 867 init_waitqueue_head(&vrp->sendq);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 868
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 869 /* We expect two virtqueues, rx and tx (and in this order) */
> 9b2bbdb2275884 Michael S. Tsirkin 2017-03-06 870 err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 871 if (err)
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 872 goto free_vrp;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 873
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 874 vrp->rvq = vqs[0];
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 875 vrp->svq = vqs[1];
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 876
> b1b9891441fa33 Suman Anna 2014-09-16 877 /* we expect symmetric tx/rx vrings */
> b1b9891441fa33 Suman Anna 2014-09-16 878 WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
> b1b9891441fa33 Suman Anna 2014-09-16 879 virtqueue_get_vring_size(vrp->svq));
> b1b9891441fa33 Suman Anna 2014-09-16 880
> b1b9891441fa33 Suman Anna 2014-09-16 881 /* we need less buffers if vrings are small */
> b1b9891441fa33 Suman Anna 2014-09-16 882 if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> b1b9891441fa33 Suman Anna 2014-09-16 883 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
> b1b9891441fa33 Suman Anna 2014-09-16 884 else
> b1b9891441fa33 Suman Anna 2014-09-16 885 vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> b1b9891441fa33 Suman Anna 2014-09-16 886
> f93848f9eeb0f8 Loic Pallardy 2017-03-28 887 vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> f93848f9eeb0f8 Loic Pallardy 2017-03-28 888
> f93848f9eeb0f8 Loic Pallardy 2017-03-28 889 total_buf_space = vrp->num_bufs * vrp->buf_size;
> b1b9891441fa33 Suman Anna 2014-09-16 890
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 891 /* allocate coherent memory for the buffers */
> d999b622fcfb39 Loic Pallardy 2019-01-10 892 bufs_va = dma_alloc_coherent(vdev->dev.parent,
> b1b9891441fa33 Suman Anna 2014-09-16 893 total_buf_space, &vrp->bufs_dma,
> b1b9891441fa33 Suman Anna 2014-09-16 894 GFP_KERNEL);
> 3119b487e03650 Wei Yongjun 2013-04-29 895 if (!bufs_va) {
> 3119b487e03650 Wei Yongjun 2013-04-29 896 err = -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 897 goto vqs_del;
> 3119b487e03650 Wei Yongjun 2013-04-29 898 }
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 899
> de4064af76537f Suman Anna 2018-10-23 900 dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
> 8d95b322ba34b1 Anna, Suman 2016-08-12 901 bufs_va, &vrp->bufs_dma);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 902
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 903 /* half of the buffers is dedicated for RX */
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 904 vrp->rbufs = bufs_va;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 905
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 906 /* and half is dedicated for TX */
> b1b9891441fa33 Suman Anna 2014-09-16 907 vrp->sbufs = bufs_va + total_buf_space / 2;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 908
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 909 /* set up the receive buffers */
> b1b9891441fa33 Suman Anna 2014-09-16 910 for (i = 0; i < vrp->num_bufs / 2; i++) {
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 911 struct scatterlist sg;
> f93848f9eeb0f8 Loic Pallardy 2017-03-28 912 void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 913
> 9dd87c2af651b0 Loic Pallardy 2017-03-28 914 rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 915
> cee51d69a45b6c Rusty Russell 2013-03-20 916 err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 917 GFP_KERNEL);
> 57e1a37347d31c Rusty Russell 2012-10-16 918 WARN_ON(err); /* sanity check; this can't really happen */
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 919 }
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 920
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 921 /* suppress "tx-complete" interrupts */
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 922 virtqueue_disable_cb(vrp->svq);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 923
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 924 vdev->priv = vrp;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 925
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 926 /* if supported by the remote processor, enable the name service */
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 927 if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 928 vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 929 if (!vch) {
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 930 err = -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 931 goto free_coherent;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 932 }
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 933
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 934 /* Link the channel to our vrp */
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 935 vch->vrp = vrp;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 936
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 937 /* Assign public information to the rpmsg_device */
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 938 rpdev_ns = &vch->rpdev;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 939 rpdev_ns->ops = &virtio_rpmsg_ops;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 940 rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 941
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 942 rpdev_ns->dev.parent = &vrp->vdev->dev;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 943 rpdev_ns->dev.release = virtio_rpmsg_release_device;
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 944
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 945 err = rpmsg_ns_register_device(rpdev_ns);
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 946 if (err)
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 947 goto free_coherent;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 948 }
>
> "vch" not initialized on else path. Also keep in mind that "rpdev_ctrl"
> is NULL at this point.
>
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 949
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 950 rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 951 if (IS_ERR(rpdev_ctrl)) {
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 952 err = PTR_ERR(rpdev_ctrl);
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 953 goto free_coherent;
>
> I should create a Smatch warning for this as well.
>
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 954 }
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 955 /*
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 956 * Prepare to kick but don't notify yet - we can't do this before
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 957 * device is ready.
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 958 */
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 959 notify = virtqueue_kick_prepare(vrp->rvq);
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 960
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 961 /* From this point on, we can notify and get callbacks. */
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 962 virtio_device_ready(vdev);
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 963
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 964 /* tell the remote processor it can start sending messages */
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 965 /*
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 966 * this might be concurrent with callbacks, but we are only
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 967 * doing notify, not a full kick here, so that's ok.
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 968 */
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 969 if (notify)
> 71e4b8bf0482fc Michael S. Tsirkin 2015-03-12 970 virtqueue_notify(vrp->rvq);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 971
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 972 dev_info(&vdev->dev, "rpmsg host is online\n");
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 973
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 974 return 0;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 975
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 976 free_coherent:
> 950a7388f02bf7 Arnaud Pouliquen 2020-11-20 @977 kfree(vch);
> ^^^^^^^^^^
> Uninitalized.
>
> e3bba4363fe87b Arnaud Pouliquen 2021-02-17 978 kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
>
> Now, let's say that "rpdev_ctrl" is NULL. That's fine because
> to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
> is a no-op. But why even have the to_virtio_rpmsg_channel() function
> if we are going to rely on it being a no-op?
>
> If "rpdev_ctrl" is an error pointer this will crash. The problem is we
> are freeing stuff that was not allocated. The fix for this is:
>
> 1) Free the most recent successful allocation.
> 2) The unwind code should mirror the allocation code.
> 3) Choose label names which say what the goto does. If the most recent
> allocation was "vch" the goto should be "goto free_vch;"
> 4) Every allocation function should have a matching free function. It's
> a layering violation to have to know that the internals of
> rpmsg_virtio_add_char_dev().
>
> So create function:
>
> void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
> {
> if (!rpdev_ctrl)
> return;
> kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
> }
>
> The clean up code looks like this:
>
> return 0;
>
> free_vch:
> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
> kfree(vch);
> free_ctrl:
> rpmsg_virtio_del_char_dev(rpdev_ctrl);
> free_coherent:
> dma_free_coherent(vdev->dev.parent, total_buf_space,
> bufs_va, vrp->bufs_dma);
> vqs_del:
> vdev->config->del_vqs(vrp->vdev);
>
> Then just go through the function and as you read down the code keep
> track of the most recent successful allocation and goto the correct
> free label.
You're right, my error management is very bad here. I will fix this based on
your recommandation.
Thanks,
Arnaud
>
> d999b622fcfb39 Loic Pallardy 2019-01-10 979 dma_free_coherent(vdev->dev.parent, total_buf_space,
> eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29 980 bufs_va, vrp->bufs_dma);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 981 vqs_del:
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 982 vdev->config->del_vqs(vrp->vdev);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 983 free_vrp:
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 984 kfree(vrp);
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 985 return err;
> bcabbccabffe73 Ohad Ben-Cohen 2011-10-20 986 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 12/16] rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (10 preceding siblings ...)
2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function Arnaud Pouliquen
` (3 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
initiate a communication through a new RPMsg channel.
This Ioctl can be used to instantiate a local RPMsg device.
Depending on the back-end implementation, a NS announcement can be sent
to the remote processor.
Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_ctrl.c | 21 +++++++++++++++++----
include/uapi/linux/rpmsg.h | 5 +++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 2e43b4096aa8..78c13816bfc6 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -70,9 +70,7 @@ static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long ar
void __user *argp = (void __user *)arg;
struct rpmsg_endpoint_info eptinfo;
struct rpmsg_channel_info chinfo;
-
- if (cmd != RPMSG_CREATE_EPT_IOCTL)
- return -EINVAL;
+ struct rpmsg_device *newch;
if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
return -EFAULT;
@@ -82,7 +80,22 @@ static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long ar
chinfo.src = eptinfo.src;
chinfo.dst = eptinfo.dst;
- return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+ switch (cmd) {
+ case RPMSG_CREATE_EPT_IOCTL:
+ return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+
+ case RPMSG_CREATE_DEV_IOCTL:
+ newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+ if (!newch) {
+ dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
+ return -ENXIO;
+ }
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+
};
static const struct file_operations rpmsg_ctrl_fops = {
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f5ca8740f3fb..f9d5a74e7801 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
*/
#define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
+/**
+ * Instantiate a rpmsg service device.
+ */
+#define RPMSG_CREATE_DEV_IOCTL _IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (11 preceding siblings ...)
2021-02-17 13:29 ` [PATCH v4 12/16] rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device Arnaud Pouliquen
` (2 subsequent siblings)
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
the rpmsg_eptdev context structure.
This patch prepares the introduction of a RPMsg device for the
char device. the RPMsg device will need a reference to the context.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 09ae1304837c..66dcb8845d6c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
}
EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
-int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
- struct rpmsg_channel_info chinfo)
+static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
+ struct device *parent,
+ struct rpmsg_channel_info chinfo)
{
struct rpmsg_eptdev *eptdev;
struct device *dev;
@@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
if (!eptdev)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
dev = &eptdev->dev;
eptdev->rpdev = rpdev;
@@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
put_device(dev);
}
- return ret;
+ return eptdev;
free_ept_ida:
ida_simple_remove(&rpmsg_ept_ida, dev->id);
@@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
put_device(dev);
kfree(eptdev);
- return ret;
+ return ERR_PTR(ret);
+}
+
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo)
+{
+ struct rpmsg_eptdev *eptdev;
+
+ eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
+ if (IS_ERR(eptdev))
+ return PTR_ERR(eptdev);
+
+ return 0;
}
EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (12 preceding siblings ...)
2021-02-17 13:29 ` [PATCH v4 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
2021-02-18 12:33 ` Dan Carpenter
2021-02-17 13:29 ` [PATCH v4 15/16] rpmsg: char: no dynamic endpoint management for the default one Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 16/16] rpmsg: char: return an error if device already open Arnaud Pouliquen
15 siblings, 1 reply; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
A RPMsg char device allows to probe the endpoint device on a remote name
service announcement. With this patch the /dev/rpmsgX interface is created
either by a user application or by the remote firmware.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 63 +++++++++++++++++++++++++++++++++++++-
1 file changed, 62 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 66dcb8845d6c..d5aa874865f7 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -28,6 +28,8 @@
#define RPMSG_DEV_MAX (MINORMASK + 1)
+#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
+
static dev_t rpmsg_major;
static struct class *rpmsg_class;
@@ -408,6 +410,51 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
}
EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
+static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_channel_info chinfo;
+ struct rpmsg_eptdev *eptdev;
+
+ memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
+ chinfo.src = rpdev->src;
+ chinfo.dst = rpdev->dst;
+
+ eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
+ if (IS_ERR(eptdev) && rpdev->ept) {
+ rpmsg_destroy_ept(rpdev->ept);
+ return PTR_ERR(eptdev);
+ }
+
+ /* Set the private field of the default endpoint to retrieve context on callback. */
+ rpdev->ept->priv = eptdev;
+
+ return 0;
+}
+
+static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
+{
+ int ret;
+
+ ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
+ if (ret)
+ dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
+}
+
+static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
+ { .name = RPMSG_CHAR_DEVNAME },
+ { },
+};
+
+static struct rpmsg_driver rpmsg_chrdev_driver = {
+ .probe = rpmsg_chrdev_probe,
+ .remove = rpmsg_chrdev_remove,
+ .id_table = rpmsg_chrdev_id_table,
+ .callback = rpmsg_ept_cb,
+ .drv = {
+ .name = "rpmsg_chrdev",
+ },
+};
+
static int rpmsg_chrdev_init(void)
{
int ret;
@@ -422,9 +469,23 @@ static int rpmsg_chrdev_init(void)
if (IS_ERR(rpmsg_class)) {
pr_err("failed to create rpmsg class\n");
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
- return PTR_ERR(rpmsg_class);
+ ret = PTR_ERR(rpmsg_class);
+ goto free_region;
}
+ ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+ if (ret < 0) {
+ pr_err("rpmsg raw: failed to register rpmsg driver\n");
+ goto free_class;
+ }
+
+ return 0;
+
+free_class:
+ class_destroy(rpmsg_class);
+free_region:
+ unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+
return ret;
}
postcore_initcall(rpmsg_chrdev_init);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device
2021-02-17 13:29 ` [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device Arnaud Pouliquen
2021-02-18 12:33 ` Dan Carpenter
@ 2021-02-18 12:33 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:33 UTC (permalink / raw)
To: kbuild, Arnaud Pouliquen, Bjorn Andersson, Ohad Ben-Cohen,
Mathieu Poirier, Andy Gross
Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
linux-arm-msm, arnaud.pouliquen
[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: riscv-randconfig-m031-20210215 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/rpmsg_char.c:429 rpmsg_chrdev_probe() error: we previously assumed 'rpdev->ept' could be null (see line 423)
vim +429 drivers/rpmsg/rpmsg_char.c
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 413 static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 414 {
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 415 struct rpmsg_channel_info chinfo;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 416 struct rpmsg_eptdev *eptdev;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 417
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 418 memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 419 chinfo.src = rpdev->src;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 420 chinfo.dst = rpdev->dst;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 421
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 422 eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @423 if (IS_ERR(eptdev) && rpdev->ept) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This condition is strange.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 424 rpmsg_destroy_ept(rpdev->ept);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What? Why are we undoing this when it's not something that we created?
This seems like a layering violation...
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 425 return PTR_ERR(eptdev);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 426 }
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 427
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 428 /* Set the private field of the default endpoint to retrieve context on callback. */
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @429 rpdev->ept->priv = eptdev;
^^^^^^^^^^^^^^^^^^^^^^^^^
If "rpdev->ept" is NULL this will Oops. If "eptdev" is an error pointer
that seems wrong as well.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 430
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 431 return 0;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 432 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31103 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device
@ 2021-02-18 12:33 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:33 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: riscv-randconfig-m031-20210215 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/rpmsg_char.c:429 rpmsg_chrdev_probe() error: we previously assumed 'rpdev->ept' could be null (see line 423)
vim +429 drivers/rpmsg/rpmsg_char.c
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 413 static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 414 {
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 415 struct rpmsg_channel_info chinfo;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 416 struct rpmsg_eptdev *eptdev;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 417
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 418 memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 419 chinfo.src = rpdev->src;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 420 chinfo.dst = rpdev->dst;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 421
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 422 eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @423 if (IS_ERR(eptdev) && rpdev->ept) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This condition is strange.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 424 rpmsg_destroy_ept(rpdev->ept);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What? Why are we undoing this when it's not something that we created?
This seems like a layering violation...
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 425 return PTR_ERR(eptdev);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 426 }
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 427
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 428 /* Set the private field of the default endpoint to retrieve context on callback. */
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @429 rpdev->ept->priv = eptdev;
^^^^^^^^^^^^^^^^^^^^^^^^^
If "rpdev->ept" is NULL this will Oops. If "eptdev" is an error pointer
that seems wrong as well.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 430
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 431 return 0;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 432 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31103 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device
@ 2021-02-18 12:33 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:33 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]
Hi Arnaud,
url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: riscv-randconfig-m031-20210215 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/rpmsg/rpmsg_char.c:429 rpmsg_chrdev_probe() error: we previously assumed 'rpdev->ept' could be null (see line 423)
vim +429 drivers/rpmsg/rpmsg_char.c
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 413 static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 414 {
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 415 struct rpmsg_channel_info chinfo;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 416 struct rpmsg_eptdev *eptdev;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 417
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 418 memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 419 chinfo.src = rpdev->src;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 420 chinfo.dst = rpdev->dst;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 421
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 422 eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @423 if (IS_ERR(eptdev) && rpdev->ept) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This condition is strange.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 424 rpmsg_destroy_ept(rpdev->ept);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What? Why are we undoing this when it's not something that we created?
This seems like a layering violation...
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 425 return PTR_ERR(eptdev);
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 426 }
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 427
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 428 /* Set the private field of the default endpoint to retrieve context on callback. */
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @429 rpdev->ept->priv = eptdev;
^^^^^^^^^^^^^^^^^^^^^^^^^
If "rpdev->ept" is NULL this will Oops. If "eptdev" is an error pointer
that seems wrong as well.
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 430
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 431 return 0;
7337f30f7a4426 Arnaud Pouliquen 2021-02-17 432 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31103 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device
2021-02-18 12:33 ` Dan Carpenter
(?)
(?)
@ 2021-02-18 17:52 ` Arnaud POULIQUEN
-1 siblings, 0 replies; 26+ messages in thread
From: Arnaud POULIQUEN @ 2021-02-18 17:52 UTC (permalink / raw)
To: Dan Carpenter, kbuild, Bjorn Andersson, Ohad Ben-Cohen,
Mathieu Poirier, Andy Gross
Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
linux-arm-msm
Hi Dan,
On 2/18/21 1:33 PM, Dan Carpenter wrote:
> Hi Arnaud,
>
> url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
> config: riscv-randconfig-m031-20210215 (attached as .config)
> compiler: riscv32-linux-gcc (GCC) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/rpmsg/rpmsg_char.c:429 rpmsg_chrdev_probe() error: we previously assumed 'rpdev->ept' could be null (see line 423)
>
> vim +429 drivers/rpmsg/rpmsg_char.c
>
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 413 static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 414 {
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 415 struct rpmsg_channel_info chinfo;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 416 struct rpmsg_eptdev *eptdev;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 417
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 418 memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 419 chinfo.src = rpdev->src;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 420 chinfo.dst = rpdev->dst;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 421
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 422 eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @423 if (IS_ERR(eptdev) && rpdev->ept) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This condition is strange.
>
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 424 rpmsg_destroy_ept(rpdev->ept);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> What? Why are we undoing this when it's not something that we created?
> This seems like a layering violation...
Right,something is not clean here, I need to crosscheck, but should be
if (IS_ERR(eptdev) && ) {
return PTR_ERR(eptdev);
}
The endpoint is already destroyed by rpmsg_dev_probe on error.
>
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 425 return PTR_ERR(eptdev);
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 426 }
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 427
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 428 /* Set the private field of the default endpoint to retrieve context on callback. */
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @429 rpdev->ept->priv = eptdev;
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> If "rpdev->ept" is NULL this will Oops. If "eptdev" is an error pointer
> that seems wrong as well.
rpdev->ept is set in rpmsg_dev_probe as the callback is defined so can not be
null, so probably a false positive here.
eptdev can not be an error pointer here for the same reason.
Anyway adding a check on the pointer, is not a big work and can prevent from
future issue.
As consequence of you multi-reports I have installed your smatch tool on my PC
and added it in my compilation chain. :)
Thanks for the review and the tool,
Arnaud
>
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 430
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 431 return 0;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 432 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 15/16] rpmsg: char: no dynamic endpoint management for the default one
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (13 preceding siblings ...)
2021-02-17 13:29 ` [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 16/16] rpmsg: char: return an error if device already open Arnaud Pouliquen
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
Do not dynamically manage the default endpoint. The ept address must
not change.
This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
case a default endpoint is used and it's address must not change or
been reused by another service.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index d5aa874865f7..0b0a6b7c0c9a 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
struct rpmsg_endpoint *ept;
struct rpmsg_device *rpdev = eptdev->rpdev;
struct device *dev = &eptdev->dev;
+ u32 addr = eptdev->chinfo.src;
get_device(dev);
- ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
- if (!ept) {
- dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
- put_device(dev);
- return -EINVAL;
+ /*
+ * The ept device can has been created by a ns announcement. In this
+ * case a default endpoint has been created. Reuse it to avoid to manage
+ * a new address on each open close.
+ */
+ ept = rpdev->ept;
+ if (!ept || addr != ept->addr) {
+ ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+ if (!ept) {
+ dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
+ put_device(dev);
+ return -EINVAL;
+ }
}
eptdev->ept = ept;
@@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
{
struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
+ struct rpmsg_device *rpdev = eptdev->rpdev;
struct device *dev = &eptdev->dev;
- /* Close the endpoint, if it's not already destroyed by the parent */
+ /*
+ * Close the endpoint, if it's not already destroyed by the parent and it is not the
+ * default one.
+ */
mutex_lock(&eptdev->ept_lock);
if (eptdev->ept) {
- rpmsg_destroy_ept(eptdev->ept);
+ if (eptdev->ept != rpdev->ept)
+ rpmsg_destroy_ept(eptdev->ept);
eptdev->ept = NULL;
}
mutex_unlock(&eptdev->ept_lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 16/16] rpmsg: char: return an error if device already open
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
` (14 preceding siblings ...)
2021-02-17 13:29 ` [PATCH v4 15/16] rpmsg: char: no dynamic endpoint management for the default one Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
15 siblings, 0 replies; 26+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
arnaud.pouliquen
The rpmsg_create_ept function is invoked when the device is opened.
As only one endpoint must be created per device. It is not possible to
open the same device twice. But there is nothing to prevent multi open.
Return -EBUSY when device is already opened to have a generic error
instead of relying on the back-end to potentially detect the error.
Without this patch for instance the GLINK driver return -EBUSY while
the virtio bus return -ENOSPC.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/rpmsg/rpmsg_char.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 0b0a6b7c0c9a..2eacddb83e29 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -116,6 +116,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
struct device *dev = &eptdev->dev;
u32 addr = eptdev->chinfo.src;
+ if (eptdev->ept)
+ return -EBUSY;
+
get_device(dev);
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread