| 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:
<PAXPR04MB9185924ED129E87C77F34DCC89E4A@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Fri, 3 Oct 2025 18:41:41 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>, Bjorn Andersson
<andersson@...nel.org>, Mathieu Poirier <mathieu.poirier@...aro.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>, Linus Walleij <linus.walleij@...aro.org>, Bartosz
Golaszewski <brgl@...ev.pl>
CC: 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>, dl-linux-imx <linux-imx@....com>,
"openamp-rp@...ts.openampproject.org" <openamp-rp@...ts.openampproject.org>
Subject: Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Hi Arnaud,
> -----Original Message-----
> From: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
> Sent: Friday, October 3, 2025 2:40 AM
> To: Shenwei Wang <shenwei.wang@....com>; Bjorn Andersson
> <andersson@...nel.org>; Mathieu Poirier <mathieu.poirier@...aro.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>; Linus Walleij <linus.walleij@...aro.org>;
> Bartosz Golaszewski <brgl@...ev.pl>
> Cc: 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;
> > These processors communicate via the RPMSG protocol.
> > The driver implements the standard GPIO interface, allowing the Linux
> > side to control GPIO controllers which reside in the remote processor
> > via RPMSG protocol.
>
> What about my request in previous version to make this driver generic?
>
The only platform-dependent part of this driver is the layout of the message packet, which defines the
communication protocol between the host and the remote processor. It would be challenging to require
other vendors to follow our protocol and conform to the expected behavior.
> In ST we have similar driver in downstream, we would be interested in reusing it if
> generic. Indeed we need some rpmsg mechanism for gpio-interrupt. Today we
> have a downstream rpmsg driver [1][2] that could migrate on a generic rpmsg-
> gpio driver.
>
> > +
> > +#include <linux/err.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/rpmsg/imx_rpmsg.h>
> > +#include <linux/init.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/rpmsg.h>
> > +
> > +#define IMX_RPMSG_GPIO_PER_PORT 32
> > +#define RPMSG_TIMEOUT 1000
> > +
> > +enum gpio_input_trigger_type {
> > + GPIO_RPMSG_TRI_IGNORE,
> > + GPIO_RPMSG_TRI_RISING,
> > + GPIO_RPMSG_TRI_FALLING,
> > + GPIO_RPMSG_TRI_BOTH_EDGE,
> > + GPIO_RPMSG_TRI_LOW_LEVEL,
> > + GPIO_RPMSG_TRI_HIGH_LEVEL,
> > +};
> What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|?
> For instance:
> GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING |
> GPIO_RPMSG_TRI_RISING,
Yes, the suggestion is better.
> > +
> > +enum gpio_rpmsg_header_type {
> > + GPIO_RPMSG_SETUP,
> > + GPIO_RPMSG_REPLY,
> > + GPIO_RPMSG_NOTIFY,
> > +};
> > +
> > +enum gpio_rpmsg_header_cmd {
> > + GPIO_RPMSG_INPUT_INIT,
> > + GPIO_RPMSG_OUTPUT_INIT,
> > + GPIO_RPMSG_INPUT_GET,
> > +};
> > +
> > +struct gpio_rpmsg_data {
> > + struct imx_rpmsg_head header;
> > + u8 pin_idx;
> > + u8 port_idx;
> > + union {
> > + u8 event;
> > + u8 retcode;
> > + u8 value;
> > + } out;
> > + union {
> > + u8 wakeup;
> > + u8 value;
> nitpicking put "value" field out of union as common
I'm afraid we can't make it common, as the two 'value' fields serve different purposes-one is used for outgoing messages and
the other for incoming messages-and they are located in different parts of the packet.
> > + } in;
> > +} __packed __aligned(8);
>
> Any reason to pack it an align it?
> This structure will be copied in the RPMSg payload, right?
>
Yes. The message will then be transmitted via the MU hardware to the remote processor, so proper alignment is required.
> I also wonder if that definition should not be in a header file with double licensing
> that the DT. Indeed this need to be common with the remote side
> implementation that can be non GPL implementation.
>
> > +
> > +struct imx_rpmsg_gpio_pin {
> > + u8 irq_shutdown;
> > + u8 irq_unmask;
> > + u8 irq_mask;
> > + u32 irq_wake_enable;
> > + u32 irq_type;
> > + struct gpio_rpmsg_data msg;
> > +};
> > +
> > +struct imx_gpio_rpmsg_info {
> > + struct rpmsg_device *rpdev;
> > + struct gpio_rpmsg_data *notify_msg;
> > + struct gpio_rpmsg_data *reply_msg;
> > + struct completion cmd_complete;
> > + struct mutex lock;
> > + msg->pin_idx = gpio;
> > + msg->port_idx = port->idx;
> > +
> > + ret = gpio_send_message(port, msg, true);
> > + if (!ret)
> > + ret = !!port->gpio_pins[gpio].msg.in.value;
> Does this code is save? !! return a boolean right?
> why not force to 1 if greater that 1?
>
This approach is intended to simplify the implementation. Forcing to 1 would introduce an additional condition check.
I'm open to changing it if that's considered standard practice.
> > +
> > + return ret;
> > +}
> > +
> > +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
> > + unsigned int gpio) {
> > + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> > + struct gpio_rpmsg_data *msg = NULL;
> > +
> > + guard(mutex)(&port->info.lock);
> > +
> > + msg = gpio_get_pin_msg(port, gpio);
> > + msg->header.cate = IMX_RPMSG_GPIO;
> Do you use a single rpmsg channel for several feature?
> Any reason to not use one rpmsg channel per feature category?
>
The current implementation on the remote side uses a single channel to handle all GPIO controllers.
However, this driver itself does not have that limitation.
> > + msg->header.major = IMX_RMPSG_MAJOR;
> > + msg->header.minor = IMX_RMPSG_MINOR;
> > + msg->header.type = GPIO_RPMSG_SETUP;
> > + msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
> > + msg->pin_idx = gpio;
> > + msg->port_idx = port->idx;
> > +
> > + msg->out.event = GPIO_RPMSG_TRI_IGNORE;
> > + msg->in.wakeup = 0;
> > +
> > + return gpio_send_message(port, msg, true); }
> > +
> > +static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
> > + unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
> > +
> > +static struct platform_driver imx_rpmsg_gpio_driver = {
> > + .driver = {
> > + .name = "gpio-imx-rpmsg",
> > + .of_match_table = imx_rpmsg_gpio_dt_ids,
> > + },
> > + .probe = imx_rpmsg_gpio_probe,
> > +};
> > +
> > +module_platform_driver(imx_rpmsg_gpio_driver);
> This implementation seems strange to me.
> You have a platform driver, but your RPMsg driver appears split between this
> driver and the remoteproc driver, especially regarding the
> imx_rpmsg_endpoint_probe() function.
>
See my reply below.
> From my point of view, this driver should declare both a platform_driver and an
> rpmsg_driver structures.
>
> Your imx_rpmsg_gpio_driver platform driver should be probed by your
> remoteproc platform.
> This platform driver would be responsible for:
> - Parsing the device tree node
> - Registering the RPMsg driver
>
> Then, the RPMsg device should be probed either by the remote processor using
> the name service announcement mechanism or if not possible by your
> remoteproc driver.
>
The idea is to probe the GPIO driver successfully only after the remote processor is online and has sent the name service announcement.
Until then, the GPIO driver will remain in a deferred state, ensuring that all consumers of the associated GPIOs are also deferred.
The implementation you provided below does not guarantee that the related consumers will be properly deferred. This is the most
important behavior for a GPIO/I2C controllers.
Thanks,
Shenwei
> To better understand my proposal you can have a look to [1]and [2].
> Here is another example for an rpmsg_i2c( ST downstream implementation):
> https://github.co/
> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
> stm32mp%2Fdrivers%2Fi2c%2Fbusses%2Fi2c-
> rpmsg.c&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b474e
> 391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 8950740622597592%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
> 3D%7C0%7C%7C%7C&sdata=6lCk20Qhb%2F0MTw0NFtto7tj7EFYwZ%2BlOR1F3
> Qk7kQn8%3D&reserved=0
> https://github.co/
> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
> stm32mp%2FDocumentation%2Fdevicetree%2Fbindings%2Fi2c%2Fi2c-
> rpmsg.yaml&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b4
> 74e391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C638950740622612512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
> ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
> %3D%7C0%7C%7C%7C&sdata=4Gva%2FpqP2u8T57XDxSDaoHhvDeJ%2Fo5HtAB
> L9TY5gbDI%3D&reserved=0
>
> Thanks and Regards,
> Arnaud
>
> > +
> > +MODULE_AUTHOR("NXP Semiconductor");
> > +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
> > +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists