| 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: <aPkVjoWkP04Q-2xP@smile.fi.intel.com>
Date: Wed, 22 Oct 2025 20:34:06 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kees Cook <kees@...nel.org>, Mika Westerberg <westeri@...nel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Walleij <linus.walleij@...aro.org>,
Manivannan Sadhasivam <mani@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Saravana Kannan <saravanak@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Shevchenko <andy@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Srinivas Kandagatla <srini@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, linux-hardening@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sound@...r.kernel.org,
linux-arm-msm@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 03/10] gpiolib: implement low-level, shared GPIO
support
On Wed, Oct 22, 2025 at 03:10:42PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> This module scans the device tree (for now only OF nodes are supported
> but care is taken to make other fwnode implementations easy to
> integrate) and determines which GPIO lines are shared by multiple users.
> It stores that information in memory. When the GPIO chip exposing shared
> lines is registered, the shared GPIO descriptors it exposes are marked
> as shared and virtual "proxy" devices that mediate access to the shared
> lines are created. When a consumer of a shared GPIO looks it up, its
> fwnode lookup is redirected to a just-in-time machine lookup that points
> to this proxy device.
>
> This code can be compiled out on platforms which don't use shared GPIOs.
...
> + if (!strends(prop->name, "-gpios") &&
> + !strends(prop->name, "-gpio") &&
> + strcmp(prop->name, "gpios") != 0 &&
> + strcmp(prop->name, "gpio") != 0)
We have gpio_suffixes for a reason (also refer to for_each_gpio_property_name()
implementation, and yes I understand the difference, this is just a reference
for an example of use of the existing list of suffixes).
> + continue;
...
> + if (strends(prop->name, "gpios"))
> + suffix = "-gpios";
> + else if (strends(prop->name, "gpio"))
> + suffix = "-gpio";
> + else
> + suffix = NULL;
> + if (!suffix)
> + continue;
In a similar way.
> + /* We only set con_id if there's actually one. */
> + if (strcmp(prop->name, "gpios") && strcmp(prop->name, "gpio")) {
And again...
> + ref->con_id = kstrdup(prop->name, GFP_KERNEL);
> + if (!ref->con_id)
> + return -ENOMEM;
> +
> + con_id_len = strlen(ref->con_id);
> + suffix_len = strlen(suffix);
> +
> + ref->con_id[con_id_len - suffix_len] = '\0';
> + }
...
> + adev->dev.parent = gdev->dev.parent;
> + /* No need to dev->release() anything. */
And is it okay?
See drivers/base/core.c:2567
WARN(1, KERN_ERR "Device '%s' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
...
> + pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
> + dev_name(&adev->dev), gpio_device_get_label(gdev));
Are you expecting dev_name() to be NULL in some cases? Otherwise why is this
not a dev_dbg() call?
> + return 0;
> +}
...
> + char *key __free(kfree) =
> + kasprintf(GFP_KERNEL,
> + KBUILD_MODNAME ".proxy.%u",
> + ref->adev.id);
This looks awful. Just allow a bit longer line
> + if (!key)
> + return -ENOMEM;
...
> +static void gpio_shared_remove_adev(struct auxiliary_device *adev)
> +{
> + lockdep_assert_held(&gpio_shared_lock);
> +
> + auxiliary_device_uninit(adev);
> + auxiliary_device_delete(adev);
_destroy() ? Esp. taking into account the (wrong?) ordering.
> +}
...
> + set_bit(GPIOD_FLAG_SHARED, flags);
Do you need this to be atomic?
> + /*
> + * Shared GPIOs are not requested via the normal path. Make
> + * them inaccessible to anyone even before we register the
> + * chip.
> + */
> + set_bit(GPIOD_FLAG_REQUESTED, flags);
Ditto.
...
> +struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct gpio_shared_desc *shared_desc;
> + struct gpio_shared_entry *entry;
> + struct gpio_shared_ref *ref;
> + struct gpio_device *gdev;
> + int ret;
> + + scoped_guard(mutex, &gpio_shared_lock) {
Much better to split the below to a helper and make it run under a
scoped_guard() here or call a guard()() there.
> + list_for_each_entry(entry, &gpio_shared_list, list) {
> + list_for_each_entry(ref, &entry->refs, list) {
> + if (adev != &ref->adev)
> + continue;
> +
> + if (entry->shared_desc) {
> + kref_get(&entry->ref);
> + shared_desc = entry->shared_desc;
> + break;
> + }
> +
> + shared_desc = kzalloc(sizeof(*shared_desc), GFP_KERNEL);
> + if (!shared_desc)
> + return ERR_PTR(-ENOMEM);
> +
> + gdev = gpio_device_find_by_fwnode(entry->fwnode);
> + if (!gdev) {
> + kfree(shared_desc);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + shared_desc->desc = &gdev->descs[entry->offset];
> + shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc);
> + if (shared_desc->can_sleep)
> + mutex_init(&shared_desc->mutex);
> + else
> + spin_lock_init(&shared_desc->spinlock);
> +
> + kref_init(&entry->ref);
> + entry->shared_desc = shared_desc;
> +
> + pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
> + dev_name(dev), desc_to_gpio(shared_desc->desc),
> + gpio_device_get_label(gdev));
> + break;
> + }
> + }
> + }
> +
> + ret = devm_add_action_or_reset(dev, gpiod_shared_put, entry);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return shared_desc;
> +}
...
> +/*
> + * This is only called if gpio_shared_init() fails so it's in fact __init and
> + * not __exit.
> + */
Fine. Have you checked if there are any section mismatch warnings during kernel
(post)build?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists