| 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:
<PAXPR04MB9185297C477DCD1929A78E4F8962A@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Tue, 10 Feb 2026 20:17:48 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Andrew Lunn <andrew@...n.ch>
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>, Bjorn Andersson <andersson@...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>,
"arnaud.pouliquen@...s.st.com" <arnaud.pouliquen@...s.st.com>, Bartosz
Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v7 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Tuesday, February 10, 2026 11:48 AM
> 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>; Bjorn Andersson
> <andersson@...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>;
> arnaud.pouliquen@...s.st.com; Bartosz Golaszewski <brgl@...ev.pl>
> Subject: [EXT] Re: [PATCH v7 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> > +#define GPIOS_PER_PORT 32
>
> Maybe this should be from DT, using "ngpios". The Documentation says:
>
> Optionally, a GPIO controller may have a "ngpios" property. This
> property indicates the number of in-use slots of available slots for
> GPIOs. The typical example is something like this: the hardware
> register is 32 bits wide, but only 18 of the bits have a physical
> counterpart. The driver is generally written so that all 32 bits can
> be used, but the IP block is reused in a lot of designs, some using
> all 32 bits, some using 18 and some using 12. In this case, setting
> "ngpios = <18>;" informs the driver that only the first 18 GPIOs, at
> local offset 0 .. 17, are in use.
>
> Just because your hardware has 32 does not mean every vendor does.
>
32 just represents the maximum number of GPIO lines that the driver can
support, so there's no need to declare all of them as in use.
Adding support for the ngpios property is a good suggestion, and I'll include
this property in the next version.
> > +struct gpio_rpmsg_head {
> > + u8 id; /* Message ID Code */
> > + u8 vendor; /* Vendor ID number */
> > + u8 version; /* Vendor-specific version number */
> > + u8 type; /* Message type */
> > + u8 cmd; /* Command code */
> > + u8 reserved[5];
> > +} __packed;
>
> I still think this should be a clean design from scratch, and you modify your
> firmware.
>
I do need to take the existing constraints into account. It's always ideal to start with
a clean design, but I also have to maintain compatibility with the current products in
the field. The approach should break what already exists.
However, as the companion firmware is updated over time, the driver can be refined
accordingly.
> This data structure is 10 bytes. Are these all needed for a generic GPIO
> controller? version, type, command and one reserved byte seems like enough,
> and it is then 4 bytes, so there is no need for __packed.
>
> > +struct gpio_rpmsg_packet {
> > + struct gpio_rpmsg_head header;
> > + u8 pin_idx;
> > + u8 port_idx;
> > + union {
> > + u8 event;
> > + u8 retcode;
> > + u8 value;
> > + } out;
> > + union {
> > + u8 wakeup;
> > + u8 value;
> > + } in;
> > +} __packed __aligned(8);
>
> This then becomes 8 bytes, so there is no need for __packed or __aligned(8).
>
Even though the struct currently evaluates to 8 bytes, that doesn't make __packed or __aligned(8)
unnecessary. Because struct layout and default alignment vary between compilers and architectures,
these attributes still serve specific purposes-__packed ensures there's no internal padding,
and __aligned(8) enforces the external alignment. Without them, the layout may not be consistent
across toolchains or build configurations.
> I don't want to force this, it is something i think which should be discussed. Do we
> adopt your design, which is not so nice, but at least has one working
> implementation, or do we do a clean design?
>
I'd lean toward getting a working solution in place first and then improving the design
over time. This approach lets us ensure functionality for current users while still giving
us room to evolve toward a cleaner, more consistent design as the code and firmware mature.
> > +static int gpio_send_message(struct rpmsg_gpio_port *port,
> > + struct gpio_rpmsg_packet *msg,
> > + bool sync)
> > +{
> > + struct gpio_rpmsg_info *info = &port->info;
> > + int err;
> > +
> > + reinit_completion(&info->cmd_complete);
> > + err = rpmsg_send(info->rpdev->ept, msg, sizeof(struct gpio_rpmsg_packet));
> > + if (err) {
> > + dev_err(&info->rpdev->dev, "rpmsg_send failed: %d\n", err);
> > + return err;
> > + }
> > +
> > + if (sync) {
> > + err = wait_for_completion_timeout(&info->cmd_complete,
> > + msecs_to_jiffies(RPMSG_TIMEOUT));
> > + if (!err) {
> > + dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
> > + return -ETIMEDOUT;
> > + }
>
> I _think_ you need to handle negative values of err. It looks like
> do_wait_for_common() can return -ERESTARTSYS;
>
> > +static struct gpio_rpmsg_packet *gpio_setup_msg_header(struct
> rpmsg_gpio_port *port,
> > + unsigned int offset,
> > + u8 cmd) {
> > + struct gpio_rpmsg_packet *msg = &port->gpio_pins[offset].msg;
> > +
> > + memset(msg, 0, sizeof(struct gpio_rpmsg_packet));
> > + msg->header.id = RPMSG_GPIO_ID;
> > + msg->header.vendor = RPMSG_VENDOR;
> > + msg->header.version = RPMSG_VERSION;
> > + msg->header.type = GPIO_RPMSG_SETUP;
> > + msg->header.cmd = cmd;
> > + msg->pin_idx = offset;
> > + msg->port_idx = port->idx;
>
> Why is a function called gpio_setup_msg_header() setting things outside of the
> header?
>
How about change to gpio_setup_msg_common()?
Thanks,
Shenwei
> > +static int rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio) {
> > + struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
> > + struct gpio_rpmsg_packet *msg;
> > + int ret;
> > +
> > + guard(mutex)(&port->info.lock);
> > +
> > + msg = gpio_setup_msg_header(port, gpio, GPIO_RPMSG_INPUT_GET);
> > +
> > + ret = gpio_send_message(port, msg, true);
>
> If gpio_setup_msg_header() does what it sounds like it should do, what is setting
> up the message body before you send the message?
>
> Andrew
Powered by blists - more mailing lists