linux-hardening - Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

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: <CAHp75VfFnd616FiG8XP_oW4MeMBrqj_nmi0xCOGUj-LG1ru-qw@mail.gmail.com>
Date: Wed, 7 May 2025 09:57:00 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Geert Uytterhoeven <geert+renesas@...der.be>, Kees Cook <kees@...nel.org>, 
	Andy Shevchenko <andy@...nel.org>, linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	thomas.petazzoni@...tlin.com, DanieleCleri@...on.eu, GaryWang@...on.com.tw, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@...tlin.com> wrote:
>
> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
>
> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> header, and also as a mux or switch.
>
> +---------+          +--------------+             +---+
>           |          |              |             |   |
>           | PWM0     |       \      |             | H |
>           |----------|------  \-----|-------------| E |
>           | I2C0_SDA |              |             | A |
> Intel SoC |----------|------\       |             | D |
>           | GPIO0    |       \------|-------------| E |
>           |----------|------        |             | R |
>           |          |     FPGA     |             |   |
> ----------+          +--------------+             +---+
>
> For most of the pins, the FPGA opens/closes a switch to enable/disable
> the access to the SoC pin from a pin header.
> Each switch, has a direction flag that is set depending the status of the
> SoC pin.
>
> For some other pins, the FPGA acts as a mux, and routes one pin (or the
> other one) to the header.
>
> The driver also provides a GPIO chip. It requests SoC pins in GPIO mode,
> and drives them in tandem with FPGA pins (switch/mux direction).
>
> This commit adds support only for UP Squared board.

...

> +#define UPBOARD_UP_PIN_MUX(bit, data)                          \
> +       {                                                       \
> +               .number = UPBOARD_UP_BIT_##bit,                 \
> +               .name = "PINMUX_"#bit,                          \
> +               .drv_data = (void *)(data),                     \

Wondering why we need to cast here and there if currently we all use
constant driver data. Perhaps enable const for now and if we ever need
that to be modified by the consumer we add that.

> +       }
> +
> +#define UPBOARD_UP_PIN_FUNC(id, data)                          \
> +       {                                                       \
> +               .number = UPBOARD_UP_BIT_##id,                  \
> +               .name = #id,                                    \
> +               .drv_data = (void *)(data),                     \

Ditto.

> +       }

...

> +static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
> +{
> +       const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
> +       size_t ngroups = pctrl->pctrl_data->ngroups;
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < ngroups; i++) {
> +               ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
> +                                               groups[i].grp.pins, groups[i].grp.npins, pctrl);

> +               if (ret < 0)

Why ' < 0' ?

> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
> +{
> +       const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
> +       size_t nfuncs = pctrl->pctrl_data->nfuncs;
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < nfuncs ; i++) {
> +               ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
> +                                                 funcs[i].groups, funcs[i].ngroups, NULL);
> +               if (ret < 0)

Ditto.

> +                       return ret;
> +       }
> +
> +       return 0;
> +}

...

> +       board_id = (enum upboard_board_id)(unsigned long)dmi_id->driver_data;
> +

Unneeded blank line.

> +       switch (board_id) {
> +       case UPBOARD_APL01:
> +               pctrl->maps = upboard_pinctrl_mapping_apl01;
> +               pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
> +               break;
> +       default:
> +               return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
> +       }

And actually we can get rid of that train of castings by switching to
use the info type of the structure

(namings are just constructed without checking for the better or
existing ones, choose another if you think they fit)

struct upboard_pinctrl_map {
  ... *maps;
  ... nmaps;
};

static const struct upboard_pinctrl_map apl01 = {
  ...
};

...dmi... = {
  ...
  .data = (void *)&apl01,
  ...
};

board_map = (const ...) dmi_id->driver_data;

...

But since DMI will require castings, it's up to you as I don't like
the idea of having that driver data not to be const in the first
place.

-- 
With Best Regards,
Andy Shevchenko