| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PAXPR04MB91855348A84594B68F05ECA689DEA@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Wed, 26 Nov 2025 22:43:38 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
Jonathan Corbet <corbet@....net>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, Pengutronix Kernel Team
<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Peng Fan
<peng.fan@....com>, "linux-remoteproc@...r.kernel.org"
<linux-remoteproc@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-doc@...r.kernel.org"
<linux-doc@...r.kernel.org>, dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH v5 2/5] remoteproc: imx_rproc: Populate devices under
"rpmsg" subnode
> -----Original Message-----
> From: Mathieu Poirier <mathieu.poirier@...aro.org>
> Sent: Tuesday, November 25, 2025 11:05 AM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: Bjorn Andersson <andersson@...nel.org>; Rob Herring <robh@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Shawn Guo <shawnguo@...nel.org>; Sascha Hauer
> <s.hauer@...gutronix.de>; Jonathan Corbet <corbet@....net>; Linus Walleij
> <linus.walleij@...aro.org>; Bartosz Golaszewski <brgl@...ev.pl>; Pengutronix
> Kernel Team <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>;
> Peng Fan <peng.fan@....com>; linux-remoteproc@...r.kernel.org;
> devicetree@...r.kernel.org; imx@...ts.linux.dev; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> doc@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: [EXT] Re: [PATCH v5 2/5] remoteproc: imx_rproc: Populate devices
> under "rpmsg" subnode
> > The following illustrates the expected DTS layout structure:
> >
> > cm33: remoteproc-cm33 {
> > compatible = "fsl,imx8ulp-cm33";
> >
> > rpmsg {
> > rpmsg-io-channel {
> > gpio@0 {
> > compatible = "fsl,imx-rpmsg-gpio";
> > reg = <0>;
> > };
> >
> > gpio@1 {
> > compatible = "fsl,imx-rpmsg-gpio";
> > reg = <1>;
> > };
> >
> > ...
> > };
> >
> > rpmsg-i2c-channel {
> > i2c@0 {
> > compatible = "fsl,imx-rpmsg-i2c";
> > reg = <0>;
> > };
> > };
> >
> > ...
> > };
> > };
>
> 2 observations here:
>
> (1) Why are you trying to introduce both i2c and GPIO in the same patchset?
The patch is to populate the "rpmsg" node under the remoteproc. This is a common function for the remoteproc.
Listing two examples (GPIO and I2C) here could give us an overview of the dts structure.
> (2) My first impression is that i2c, GPIO and others should be bundled with the
> platform definitions for i2c and GPIO, with a phandle to the remote processor
> they hang off from. My opinion may change once I have reviewed the other
> patches in this set.
>
The design is to let the remoteproc to populate the "rpmsg" node, and separate the rpmsg endpoint driver and its above function driver like GPIO.
Bunding the GPIO and rpdev driver together would make the driver become a multi-function driver. I am not sure if it is still suitable to put it in the directory of "drivers/gpio".
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> > ---
> > drivers/remoteproc/imx_rproc.c | 146
> > ++++++++++++++++++++++++++++++++ include/linux/rpmsg/imx_rpmsg.h |
> > 48 +++++++++++
> > 2 files changed, 194 insertions(+)
> > create mode 100644 include/linux/rpmsg/imx_rpmsg.h
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index a6eef0080ca9..e21a7980c490
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -8,6 +8,7 @@
> > #include <linux/clk.h>
> > #include <linux/err.h>
> > #include <linux/firmware/imx/sci.h>
> > +#include <linux/rpmsg/imx_rpmsg.h>
> > #include <linux/interrupt.h>
> > #include <linux/kernel.h>
> > #include <linux/mailbox_client.h>
> > @@ -15,6 +16,8 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > #include <linux/of_reserved_mem.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_domain.h>
> > @@ -22,6 +25,7 @@
> > #include <linux/reboot.h>
> > #include <linux/regmap.h>
> > #include <linux/remoteproc.h>
> > +#include <linux/rpmsg.h>
> > #include <linux/workqueue.h>
> >
> > #include "imx_rproc.h"
> > @@ -1084,6 +1088,144 @@ static int imx_rproc_sys_off_handler(struct
> sys_off_data *data)
> > return NOTIFY_DONE;
> > }
> >
> > +struct imx_rpmsg_driver {
> > + struct rpmsg_driver rpdrv;
> > + void *driver_data;
> > +};
> > +
> > +static char *channel_device_map[][2] = {
> > + {"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
> > + {"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"}, };
> > +
> > +static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev, void *data,
> > + int len, void *priv, u32 src) {
> > + struct imx_rpmsg_driver_data *drvdata;
> > +
> > + drvdata = dev_get_drvdata(&rpdev->dev);
> > + if (drvdata && drvdata->rx_callback)
> > + return drvdata->rx_callback(rpdev, data, len, priv,
> > + src);
> > +
> > + return 0;
> > +}
> > +
> > +static void imx_rpmsg_endpoint_remove(struct rpmsg_device *rpdev) {
> > + of_platform_depopulate(&rpdev->dev);
> > +}
> > +
> > +static int imx_rpmsg_endpoint_probe(struct rpmsg_device *rpdev) {
> > + struct imx_rpmsg_driver_data *drvdata;
> > + struct imx_rpmsg_driver *imx_rpdrv;
> > + struct device *dev = &rpdev->dev;
> > + struct of_dev_auxdata *auxdata;
> > + struct rpmsg_driver *rpdrv;
> > + int i;
> > +
> > + rpdrv = container_of(dev->driver, struct rpmsg_driver, drv);
> > + imx_rpdrv = container_of(rpdrv, struct imx_rpmsg_driver, rpdrv);
> > +
> > + if (!imx_rpdrv->driver_data)
> > + return -EINVAL;
> > +
> > + drvdata = devm_kmemdup(dev, imx_rpdrv->driver_data,
> > + sizeof(*drvdata), GFP_KERNEL);
>
> Something with the overall design of this feature doesn't work if you need to do
> this.
>
Could you elaborate further?
> > + if (!drvdata)
> > + return -ENOMEM;
> > +
> > + i = drvdata->map_idx;
> > + if (i >= ARRAY_SIZE(channel_device_map))
> > + return -ENODEV;
> > +
> > + auxdata = devm_kzalloc(dev, sizeof(*auxdata) * 2, GFP_KERNEL);
> > + if (!auxdata)
> > + return -ENOMEM;
> > +
> > + drvdata->rpdev = rpdev;
> > + auxdata[0].compatible = channel_device_map[i][1];
> > + auxdata[0].platform_data = drvdata;
> > + dev_set_drvdata(dev, drvdata);
> > +
> > + of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
> > + of_node_put(drvdata->channel_node);
>
> Why is there a need for of_node_put() when imx_of_rprmsg_node_init() is using
> for_each_child_of_node_scoped()?
>
You are right. This "put" should be deleted.
> > +
> > + return 0;
> > +}
> > +
> > +static int imx_of_rpmsg_is_in_map(const char *name) {
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(channel_device_map); i++) {
> > + if (strcmp(name, channel_device_map[i][0]) == 0)
> > + return i;
> > + }
> > +
> > + return -1;
>
> Please return a real error code.
>
> > +}
> > +
> > +static int imx_of_rpmsg_register_rpdriver(struct device_node *channel,
> > + struct device *dev, int idx) {
> > + struct imx_rpmsg_driver_data *driver_data;
> > + struct imx_rpmsg_driver *rp_driver;
> > + struct rpmsg_device_id *rpdev_id;
> > +
> > + rpdev_id = devm_kzalloc(dev, sizeof(*rpdev_id) * 2, GFP_KERNEL);
>
> I had a really good time figuring out why 2 instances were needed. Reviewing
> this work without a single line of comment isn't motivating.
>
> > + if (!rpdev_id)
> > + return -ENOMEM;
> > +
> > + strscpy(rpdev_id[0].name, channel_device_map[idx][0],
> > + RPMSG_NAME_SIZE);
> > +
> > + rp_driver = devm_kzalloc(dev, sizeof(*rp_driver), GFP_KERNEL);
> > + if (!rp_driver)
> > + return -ENOMEM;
> > +
> > + driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
> > + if (!driver_data)
> > + return -ENOMEM;
> > +
> > + driver_data->rproc_name = dev->of_node->name;
> > + driver_data->channel_node = channel;
> > + driver_data->map_idx = idx;
>
> struct rpmsg_device_id already contains a @driver_data. As with my other
> comment above, something in the design of this feature is not working if you need
> to do something like this.
>
The purpose is to use the existing driver_data pointer inside the rpmsg driver to pass the instance relating data.
If you think there is another standard method, please let me know.
> > +
> > + rp_driver->rpdrv.drv.name = channel_device_map[idx][0];
> > + rp_driver->rpdrv.id_table = rpdev_id;
> > + rp_driver->rpdrv.probe = imx_rpmsg_endpoint_probe;
> > + rp_driver->rpdrv.remove = imx_rpmsg_endpoint_remove;
> > + rp_driver->rpdrv.callback = imx_rpmsg_endpoint_cb;
> > + rp_driver->driver_data = driver_data;
> > +
> > + register_rpmsg_driver(&rp_driver->rpdrv);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx_of_rpmsg_node_init(struct platform_device *pdev) {
> > + struct device_node *np __free(device_node);
> > + struct device *dev = &pdev->dev;
> > + int idx, ret;
> > +
> > + np = of_get_child_by_name(dev->of_node, "rpmsg");
> > + if (!np)
> > + return 0;
> > +
> > + for_each_child_of_node_scoped(np, child) {
> > + idx = imx_of_rpmsg_is_in_map(child->name);
> > + if (idx < 0)
> > + ret = of_platform_default_populate(child, NULL,
> > + dev);
>
> Why is this called?
>
This is to populate the rpmsg channels not in the defined map under the "rpmsg" node in a default method.
> > + else
> > + ret = imx_of_rpmsg_register_rpdriver(child, dev,
> > + idx);
> > +
> > + if (ret < 0)
> > + return ret;
>
> In case of an error, the reference to @np is not released.
It will be released when the function returns, because it is defined as
struct device_node *np __free(device_node);
Thanks,
Shenwei
>
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> If we were to keep the current design, all of the above should be in
> drivers/rpmsg/imx_rpmsg.c.
>
> > static int imx_rproc_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev; @@ -1177,6 +1319,10 @@ static
> > int imx_rproc_probe(struct platform_device *pdev)
> > goto err_put_clk;
> > }
> >
> > + ret = imx_of_rpmsg_node_init(pdev);
> > + if (ret < 0)
> > + dev_info(dev, "populating 'rpmsg' node failed\n");
> > +
> > return 0;
> >
> > err_put_clk:
> > diff --git a/include/linux/rpmsg/imx_rpmsg.h
> > b/include/linux/rpmsg/imx_rpmsg.h new file mode 100644 index
> > 000000000000..04a5ad2d4a1d
> > --- /dev/null
> > +++ b/include/linux/rpmsg/imx_rpmsg.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright 2025 NXP */
> > +
> > +/*
> > + * @file linux/imx_rpmsg.h
> > + *
> > + * @brief Global header file for iMX RPMSG
> > + *
> > + * @ingroup RPMSG
> > + */
> > +#ifndef __LINUX_IMX_RPMSG_H__
> > +#define __LINUX_IMX_RPMSG_H__
> > +
> > +/* Category define */
> > +#define IMX_RMPSG_LIFECYCLE 1
> > +#define IMX_RPMSG_PMIC 2
> > +#define IMX_RPMSG_AUDIO 3
> > +#define IMX_RPMSG_KEY 4
> > +#define IMX_RPMSG_GPIO 5
> > +#define IMX_RPMSG_RTC 6
> > +#define IMX_RPMSG_SENSOR 7
> > +
> > +/* rpmsg version */
> > +#define IMX_RMPSG_MAJOR 1
> > +#define IMX_RMPSG_MINOR 0
> > +
> > +#define MAX_DEV_PER_CHANNEL 10
> > +
> > +struct imx_rpmsg_head {
> > + u8 cate; /* Category */
> > + u8 major; /* Major version */
> > + u8 minor; /* Minor version */
> > + u8 type; /* Message type */
> > + u8 cmd; /* Command code */
> > + u8 reserved[5];
> > +} __packed;
>
> This structure is not used in this file, please remove.
>
> > +
> > +struct imx_rpmsg_driver_data {
> > + int map_idx;
> > + const char *rproc_name;
> > + struct rpmsg_device *rpdev;
> > + struct device_node *channel_node;
> > + int (*rx_callback)(struct rpmsg_device *rpdev, void *data,
> > + int len, void *priv, u32 src);
> > + void *channel_devices[MAX_DEV_PER_CHANNEL];
> > +};
>
> For the above 2 struct, proper documentation is needed.
>
> Thanks,
> Mathieu
>
> > +
> > +#endif /* __LINUX_IMX_RPMSG_H__ */
> > --
> > 2.43.0
> >
Powered by blists - more mailing lists