| 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: <CAMRc=Mc165HSLdug1F+t3qcOoE52mR1e_zEh=rSTUKN_-dB5NA@mail.gmail.com>
Date: Thu, 23 Oct 2025 20:55:27 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
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 7:34 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> 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).
>
And how would you use them here - when you also need the hyphen -
without multiple dynamic allocations instead of static strings?
>
> > + 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",
>
Huh... you're not wrong but I haven't seen this warning. Do people
just use empty functions in this case?
> ...
>
> > + 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?
>
It's the low-level code saying: "I created device X for Y", not "Hey,
here's X, I'm here for Y".
>
> > + 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
>
Ok
> > + 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.
>
You're right about the order but if you _add() then you should
_delete(). You typically _destroy() if you earlier _create().
> > +}
>
> ...
>
> > + 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.
>
Ok
> ...
>
> > +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.
>
I'm not following, please rephrase.
> > + 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?
>
Yes, there are none.
Bartosz
Powered by blists - more mailing lists