linux-kernel - Re: [PATCH] gpio: Add Intel Granite Rapids-D vGPIO 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CACRpkdbSB+JTdhGXViWs-SmR3nUnm6dVXt3WzK-d4zFSz63XxQ@mail.gmail.com>
Date: Fri, 19 Apr 2024 15:11:23 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Aapo Vienamo <aapo.vienamo@...ux.intel.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Andy Shevchenko <andy@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] gpio: Add Intel Granite Rapids-D vGPIO driver

Hi Aapo,

thanks for your patch!

The code is impeccable, not much to say about that.
>From that PoV the driver is finished.

I have some technical review comments:

On Fri, Apr 19, 2024 at 10:07 AM Aapo Vienamo
<aapo.vienamo@...ux.intel.com> wrote:

> This driver provides a basic GPIO driver for the Intel Granite Rapids-D
> virtual GPIOs. On SoCs with limited physical pins on the package, the
> physical pins controlled by this driver would be exposed on an external
> device such as a BMC or CPLD.
>
> Signed-off-by: Aapo Vienamo <aapo.vienamo@...ux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

OK I get how it works, but not all the way right? We write these registers,
and somehow that results on pins on a completely different piece of
silicon in a different package driving some lines low/high?

So ... can we write something about how the signal gets over there
from where the driver is running? It needs to happen somehow, right?

> +config GPIO_GRANITERAPIDS
> +       tristate "Intel Granite Rapids-D vGPIO support"
> +       depends on X86 || COMPILE_TEST
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Select this to enable GPIO support on platforms with the following
> +         SoCs:
> +
> +         - Intel Granite Rapids-D
> +
> +         The driver enables basic GPIO functionality and implements interrupt
> +         support.
> +
> +         To compile this driver as a module, choose M here: the module will
> +         be called gpio-graniterapids.

This help text is not as informative as the commit log. Write something
about how the GPIO works here, too.

> +static int gnr_gpio_configure_pad(struct gpio_chip *gc, unsigned int gpio,
> +                                 u32 clear_mask, u32 set_mask)
> +{
> +       struct gnr_gpio *priv = gpiochip_get_data(gc);
> +       void __iomem *addr = gnr_gpio_get_padcfg_addr(priv, gpio);
> +       u32 dw;
> +
> +       if (test_bit(gpio, priv->ro_bitmap))
> +               return -EACCES;
> +
> +       guard(raw_spinlock_irqsave)(&priv->lock);
> +
> +       dw = readl(addr);
> +       dw &= ~clear_mask;
> +       dw |= set_mask;
> +       writel(dw, addr);
> +
> +       return 0;
> +}

Configure pad sounds like pin control so it's a bit of icky name.
What it really does is configure the direction (in or out) for this
GPIO pad. And it's not really the *pad* that is configured, right?
It is the hardware *driver* for the pad, i.e. what is reflected in
the GPIO line control register.

Can you rename this:
gnr_gpio_configure_direction()?

With the above stuff addressed:
Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Yours,
Linus Walleij