| 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:
<PAXPR04MB9185AF0B5225CB8DD06F774289B5A@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Tue, 23 Dec 2025 19:47:31 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Linus Walleij <linusw@...nel.org>, Bartosz Golaszewski <brgl@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Mathieu Poirier
<mathieu.poirier@...aro.org>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
<s.hauer@...gutronix.de>, Jonathan Corbet <corbet@....net>, Pengutronix
Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Peng
Fan <peng.fan@....com>, "linux-gpio@...r.kernel.org"
<linux-gpio@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-remoteproc@...r.kernel.org"
<linux-remoteproc@...r.kernel.org>, "imx@...ts.linux.dev"
<imx@...ts.linux.dev>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-doc@...r.kernel.org"
<linux-doc@...r.kernel.org>, dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH v6 2/5] remoteproc: imx_rproc: Populate devices under
"rpmsg" subnode
> -----Original Message-----
> From: Bjorn Andersson <andersson@...nel.org>
> Sent: Thursday, December 18, 2025 8:24 PM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: Linus Walleij <linusw@...nel.org>; Bartosz Golaszewski <brgl@...nel.org>;
> Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>;
> Conor Dooley <conor+dt@...nel.org>; Mathieu Poirier
> <mathieu.poirier@...aro.org>; Shawn Guo <shawnguo@...nel.org>; Sascha
> Hauer <s.hauer@...gutronix.de>; Jonathan Corbet <corbet@....net>;
> Pengutronix Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> <festevam@...il.com>; Peng Fan <peng.fan@....com>; linux-
> gpio@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-remoteproc@...r.kernel.org; imx@...ts.linux.dev;
> linux-arm-kernel@...ts.infradead.org; linux-doc@...r.kernel.org; dl-linux-imx
> <linux-imx@....com>
> Subject: [EXT] Re: [PATCH v6 2/5] remoteproc: imx_rproc: Populate devices
> under "rpmsg" subnode
>
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
>
>
> On Fri, Dec 12, 2025 at 01:43:38PM -0600, Shenwei Wang wrote:
> > Register the RPMsg channel driver and populate remote devices defined
> > under the "rpmsg" subnode upon receiving their notification messages.
>
> Please provide a proper description of what "problem" this patch solves.
>
> >
> > 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>;
>
> Surely there needs to be some "gpio-controller" and "#gpio-cells" here?
> Would be useful if the example is somewhat complete, to give a picture of what's
> actually going on.
>
Okay. Will add those in next version.
> > };
> >
> > gpio@1 {
> > compatible = "fsl,imx-rpmsg-gpio";
> > reg = <1>;
> > };
> >
> > ...
> > };
> >
> > ...
> > };
> > };
> >
> > + drvdata->rpdev = rpdev;
> > + auxdata[0].compatible = devm_kstrdup(dev, imx_rpdrv->compat,
> GFP_KERNEL);
> > + auxdata[0].platform_data = drvdata;
> > + dev_set_drvdata(dev, drvdata);
> > +
> > + of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
>
> auxiliary_bus would be a better choice, but I don't understand why you probe a
> rpmsg_device for each "gpio channel" and then from that create a
> platform_device.
>
> Why don't you just make the rpmsg_device register the gpio controller directly?
>
The "GPIO channel" is just one example-there are also "PWM channel", "I2C channel", and other channels.
The goal is to manage all these channels under a common logic, which helps avoid redundant code and keeps
the implementation consistent.
> > +
> > + return 0;
> > +}
> > +
> > + rp_driver->rpdrv.drv.name = name;
> > + 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;
> > + rp_driver->compat = compat;
> > +
> > + register_rpmsg_driver(&rp_driver->rpdrv);
>
> This would then also imply that it's the gpio driver that registers the
> rpmsg_driver.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int rproc_of_rpmsg_node_init(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + const char *compat;
> > + int ret;
> > +
> > + struct device_node *np __free(device_node) = of_get_child_by_name(dev-
> >of_node, "rpmsg");
> > + if (!np)
> > + return 0;
> > +
> > + for_each_child_of_node_scoped(np, child) {
> > + compat = imx_of_rpmsg_is_in_map(child->name);
> > + if (!compat)
> > + ret = of_platform_default_populate(child, NULL,
> > + dev);
>
> So if you don't recognize the child device node name you just register
> platform_devices for each of the children?
>
Yes. That would register platform_devices without the platform_data.
> > + else
> > + ret = imx_of_rpmsg_register_rpdriver(child, dev,
> > + child->name, compat);
> > +
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int imx_rproc_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev; @@ -1114,6 +1253,10 @@ static
> > int imx_rproc_probe(struct platform_device *pdev)
> > goto err_put_pm;
> > }
> >
> > + ret = rproc_of_rpmsg_node_init(pdev);
> > + if (ret < 0)
> > + dev_info(dev, "populating 'rpmsg' node failed\n");
> > +
> > return 0;
> >
> > err_put_pm:
> > diff --git a/include/linux/rpmsg/rpdev_info.h
> > b/include/linux/rpmsg/rpdev_info.h
> > new file mode 100644
> > index 000000000000..13e020cd028b
> > --- /dev/null
> > +++ b/include/linux/rpmsg/rpdev_info.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright 2025 NXP */
> > +
> > +/*
> > + * @file linux/rpdev_info.h
> > + *
> > + * @brief Global header file for RPDEV Info
> > + *
> > + * @ingroup RPMSG
> > + */
> > +#ifndef __LINUX_RPDEV_INFO_H__
> > +#define __LINUX_RPDEV_INFO_H__
> > +
> > +#define MAX_DEV_PER_CHANNEL 10
> > +
> > +/**
> > + * rpdev_platform_info - store the platform information of rpdev
> > + * @rproc_name: the name of the remote proc.
> > + * @rpdev: rpmsg channel device
> > + * @device_node: pointer to the device node of the rpdev.
> > + * @rx_callback: rx callback handler of the rpdev.
> > + * @channel_devices: an array of the devices related to the rpdev.
> > + */
> > +struct rpdev_platform_info {
>
> I don't understand what this structure represents. Why is this glue between the
> rpmsg_device and a made up platform_device needed?
>
The purpose is to have a shared array that can be accessed by all devices within
the same channel.
> > + const char *rproc_name;
>
> You don't need this, because you can rproc_get_by_child(&self) and then get the
> remoteproc name from that.
>
Good to know. Will try it in the next version.
> > + 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];
>
> Why 10? What does it mean?
>
This is based on practical experience. For example, on the i.MX platform, we typically don't have
more than eight same devices on the remote system.
> I think this becomes the list of the 10 grandchildren of the remoteproc (per child
> node). So what happens if those matches against two different drivers, what will
> rx_callback point to?
>
This is the limitation. That's why I used the map to populate the known child device for one specific channel.
Thanks,
Shenwei
> > +};
>
> Regards,
> Bjorn
>
> > +
> > +#endif /* __LINUX_RPDEV_INFO_H__ */
> > --
> > 2.43.0
> >
Powered by blists - more mailing lists