linux-kernel - Re: [PATCH v6 4/5] gpio: rpmsg: add generic rpmsg GPIO driver

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <xiqftcgga24bypbux276u56t2lxjy4oxzggixrhy5h7woc4aon@2zbcfxxrzqig>
Date: Tue, 23 Dec 2025 18:09:44 -0600
From: Bjorn Andersson <andersson@...nel.org>
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" <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>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v6 4/5] gpio: rpmsg: add generic rpmsg GPIO driver

On Tue, Dec 23, 2025 at 08:20:49PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Bjorn Andersson <andersson@...nel.org>
> > Sent: Thursday, December 18, 2025 9:58 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>; 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
> > On Fri, Dec 12, 2025 at 01:43:40PM -0600, Shenwei Wang wrote:
> > > On an AMP platform, the system may include two processors:
[..]
> > > +static int rpmsg_gpio_callback(struct rpmsg_device *rpdev,
> > > +                            void *data, int len, void *priv, u32 src)
> > > +{
[..]
> > > +     init_completion(&port->info.cmd_complete);
> > > +     port->info.rpdev = pltdata->rpdev;
> > > +     port->info.port_store = pltdata->channel_devices;
> > > +     port->info.port_store[port->idx] = port;
> > > +     if (!pltdata->rx_callback)
> > > +             pltdata->rx_callback = rpmsg_gpio_callback;
> > 
> > What happens if you rmmod your rpmsg gpio driver and then trigger an interrupt?
> > 
> 
> The driver has a rpmsg_gpio_remove_action which will clear the devices in the pltdata->channel_devices[].
> In the rpmsg callback function, it will just return -NODEV error.

When you rmmod the rpmsg_gpio driver, the code will go away, so at the
address of rpmsg_gpio_callback there's going to be some undefined data.

> 
>      if (msg)
>              port = drvdata->channel_devices[msg->port_idx];

So this code is no longer going to be there, or maybe there's some
remnants of it? But jumping to the address of rpmsg_gpio_callback (not
the actual rpmsg_gpio_callback) is best case going to result in a crash.

> 
>      if (!port)
>              return -ENODEV;
> 
> > > +
[..]
> > > +static const struct of_device_id rpmsg_gpio_dt_ids[] = {
> > > +     { .compatible = "rpmsg-gpio" },
> > > +     { /* sentinel */ }
> > > +};
> > > +
> > > +static struct platform_driver rpmsg_gpio_driver = {
> > 
> > It's an "rpmsg gpio driver", but it's a platform_driver...
> > 
> > I don't think this is the correct design, but if it is then this needs to be well
> > documented.
> > 
> > Same thing as platform_data forms a strong ABI between some other driver and
> > this platform_driver, this needs to be well documented (but should be avoided).
> > 
> 
> Are you suggesting to use "rpmsg_driver" framework here?
> 

Yes, making the functional driver bind to the particular rpmsg channel
is exactly what I would expect from a "rpmsg gpio driver".

Regards,
Bjorn

> Thank you very much for the review.
> Shenwei
> 
> > Regards,
> > Bjorn
> > 
> > > +     .driver = {
> > > +             .name = "gpio-rpmsg",
> > > +             .of_match_table = rpmsg_gpio_dt_ids,
> > > +     },
> > > +     .probe = rpmsg_gpio_probe,
> > > +};
> > > +
> > > +module_platform_driver(rpmsg_gpio_driver);
> > > +
> > > +MODULE_AUTHOR("Shenwei Wang <shenwei.wang@....com>");
> > > +MODULE_DESCRIPTION("generic rpmsg gpio driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.43.0
> > >