| 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: <bb5cb154-0f7a-4e21-afe3-453ff5ee9373@foss.st.com>
Date: Mon, 1 Sep 2025 09:26:47 +0200
From: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To: Linus Walleij <linus.walleij@...aro.org>,
Shenwei Wang
<shenwei.wang@....com>,
Bjorn Andersson <andersson@...nel.org>,
"Mathieu
Poirier" <mathieu.poirier@...aro.org>
CC: 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>,
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-imx@....com>
Subject: Re: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Hello,
On 8/21/25 11:01, Linus Walleij wrote:
> Hi Shenwei,
>
> thanks for your patch!
>
> On Mon, Aug 18, 2025 at 10:45 PM Shenwei Wang <shenwei.wang@....com> wrote:
>
>> On i.MX SoCs, the system may include two processors:
>> - An MCU running an RTOS
>> - An MPU running Linux
>>
>> 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.
>>
>> Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> Since this is a first RPMSG GPIO driver, I'd like if Björn and/or
> Mathieu have a look at it so I'm sure it is RPMSG-proper!
Could this driver be generic (platform independent) ?
Perhaps i missed something, but it seems to me that there is no IMX
specific code.
Making it generic would allow other platforms to reuse it instead of
duplicating it.
Thanks,
Arnaud
>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index a437fe652dbc..2ce4e9b5225e 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -402,6 +402,17 @@ config GPIO_ICH
>>
>> If unsure, say N.
>>
>> +config GPIO_IMX_RPMSG
>> + tristate "NXP i.MX SoC RPMSG GPIO support"
>> + depends on IMX_REMOTEPROC && RPMSG && GPIOLIB
>> + default IMX_REMOTEPROC
>> + help
>> + Say yes here to support the RPMSG GPIO functions on i.MX SoC based
>> + platform. Currently supported devices: i.MX7ULP, i.MX8ULP, i.MX8x,
>> + and i.MX9x.
>> +
>> + If unsure, say N.
> This is sorted under memory-mapped GPIO, but it isn't.
>
> Create a new submenu:
>
> menu "RPMSG GPIO drivers"
> depends on RPMSG
>
> And put it here as the first such driver.
>
> No need to have a dependency on RPMSG in the GPIO_IMX_RPMSG
> Kconfig entry after this.
>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/bitops.h>
> bitops.h or just bits.h? Check which one you actually use.
>
>> +#include <linux/err.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/imx_rpmsg.h>
>> +#include <linux/init.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_qos.h>
> Are you really using pm_qos?
>
>> +#include <linux/rpmsg.h>
>> +#include <linux/virtio.h>
>> +#include <linux/workqueue.h>
> (...)
>
>> +struct imx_rpmsg_gpio_port {
>> + struct gpio_chip gc;
>> + struct irq_chip chip;
> This irqchip doesn't look very immutable.
>
> Look at other patches rewriting irqchips to be immutable
> and break this out to a static const struct irq_chip with
> IRQCHIP_IMMUTABLE set instead.
>
>> +static int imx_rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>> + struct gpio_rpmsg_data *msg = NULL;
>> + int ret;
>> +
>> + mutex_lock(&port->info.lock);
> Please use guards for all the mutexes:
>
> #include <linux/cleanup.h>
>
> guard(mutex)(&port->info.lock);
>
> and it will be released as you exit the function.
>
>> +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;
>> + int ret;
>> +
>> + mutex_lock(&port->info.lock);
> Dito for all these instances.
> (Saves you a bunch of lines!)
>
>> +static void imx_rpmsg_irq_bus_lock(struct irq_data *d)
>> +{
>> + struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>> +
>> + mutex_lock(&port->info.lock);
>> +}
> Here you need to keep the classic mutex_lock() though,
> because of the irqchip locking abstraction helper.
>
>> +static struct irq_chip imx_rpmsg_irq_chip = {
> const
>
>> + .irq_mask = imx_rpmsg_mask_irq,
>> + .irq_unmask = imx_rpmsg_unmask_irq,
>> + .irq_set_wake = imx_rpmsg_irq_set_wake,
>> + .irq_set_type = imx_rpmsg_irq_set_type,
>> + .irq_shutdown = imx_rpmsg_irq_shutdown,
>> + .irq_bus_lock = imx_rpmsg_irq_bus_lock,
>> + .irq_bus_sync_unlock = imx_rpmsg_irq_bus_sync_unlock,
> .flags = IRQCHIP_IMMUTABLE,
>
> probably also:
>
> GPIOCHIP_IRQ_RESOURCE_HELPERS,
>
> ?
>
> I think you want to properly mark GPIO lines as used for
> IRQs!
>
>> +static int imx_rpmsg_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>> + int irq;
>> +
>> + irq = irq_find_mapping(port->domain, gpio);
>> + if (irq > 0) {
>> + irq_set_chip_data(irq, port);
>> + irq_set_chip_and_handler(irq, &port->chip, handle_level_irq);
>> + }
>> +
>> + return irq;
>> +}
> Ugh we try to to use custom to_irq() if we can...
>
> Do you have to?
>
> Can't you use
> select GPIOLIB_IRQCHIP
> and be inspired by other chips using the irqchip
> helper library?
>
> We almost always use that these days.
>
>> + /* create an irq domain */
>> + port->chip = imx_rpmsg_irq_chip;
>> + port->chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
>> + pltdata->rproc_name, port->idx);
>> + port->dev = &pdev->dev;
>> +
>> + irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, IMX_RPMSG_GPIO_PER_PORT,
>> + numa_node_id());
>> + if (irq_base < 0) {
>> + dev_err(&pdev->dev, "Failed to alloc irq_descs\n");
>> + return irq_base;
>> + }
>> +
>> + port->domain = irq_domain_create_legacy(of_node_to_fwnode(np),
>> + IMX_RPMSG_GPIO_PER_PORT,
>> + irq_base, 0,
>> + &irq_domain_simple_ops, port);
>> + if (!port->domain) {
>> + dev_err(&pdev->dev, "Failed to allocate IRQ domain\n");
>> + return -EINVAL;
>> + }
> This also looks unnecessarily custom.
>
> Try to use GPIOLIB_IRQCHIP.
>
>
>> +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,
>> +};
>> +
>> +static int __init gpio_imx_rpmsg_init(void)
>> +{
>> + return platform_driver_register(&imx_rpmsg_gpio_driver);
>> +}
>> +
>> +device_initcall(gpio_imx_rpmsg_init);
> No please just do:
>
> module_platform_driver(imx_rpmsg_gpio_driver);
>
> Fix up these things to begin with and then we can
> look at details!
>
> Yours,
> Linus Walleij
>
Powered by blists - more mailing lists