linux-kernel - Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller

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] [day] [month] [year] [list]

Date:   Fri, 21 Feb 2020 17:26:13 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Asmaa Mnebhi <Asmaa@...lanox.com>
Cc:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller

Hi Asmaa,

thanks for your patch!

On Fri, Feb 21, 2020 at 4:40 PM Asmaa Mnebhi <Asmaa@...lanox.com> wrote:

> This patch adds support for the GPIO controller used by
> Mellanox BlueField 2 SOCs.
>
> Signed-off-by: Asmaa Mnebhi <Asmaa@...lanox.com>
(...)

> +config GPIO_MLXBF2
> +       tristate "Mellanox BlueField 2 SoC GPIO"
> +       depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
> +       select GPIO_GENERIC

This selects GPIO_GENERIC but does not make use of it?

> +/*
> + * gpio[x] block registers and their offset
> + */
> +#define YU_GPIO_DATAOUT                        0x00
> +#define YU_GPIO_DATAIN                 0x04
> +#define YU_GPIO_MODE1                  0x08
> +#define YU_GPIO_MODE0                  0x0c
> +#define YU_GPIO_DATASET                        0x14
> +#define YU_GPIO_DATACLEAR              0x18
> +#define YU_GPIO_MODE1_CLEAR            0x50
> +#define YU_GPIO_MODE0_SET              0x54
> +#define YU_GPIO_MODE0_CLEAR            0x58

This however looks a lot like it could use GPIO_GENERIC.

> +       /* Must hold this lock to modify shared data. */
> +       spinlock_t lock;

Incidentally GPIO_GENERIC accessors also contains
a lock so you don't need to implement that either.

> +       /* All 3 YU GPIO block address */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, GPIO_BLOCKS);
> +       if (!res)
> +               return -ENODEV;

If there are 3 GPIO blocks, simply spawn 3 struct gpio_chip's so you
can use the generic accessors?

> +       /* YU ARM GPIO Lock address */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, ARM_GPIO_LOCK);
> +       if (!res)
> +               return -ENOMEM;

If the direction setting needs some special lock/unlock to happen, why not
just override the .set/get_direction() callbacks?

I would recommend:

- Split in one gpio_chip per instance
- Look at drivers/gpio/gpio-ftgpio010.c on how to populate callbacks
  for each with bgpio_init()
- In struct mlxbf2_gpio_state create some custom direction
  callbacks to store what comes back in direction_input/output
  and indirect the calls from direction_input/output to these
  with locking folded in.

Yours,
Linus Walleij