| 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: <aPsmMruDxOil_wYQ@smile.fi.intel.com>
Date: Fri, 24 Oct 2025 10:09:38 +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 Thu, Oct 23, 2025 at 08:55:27PM +0200, Bartosz Golaszewski wrote:
> 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:
...
> > > + 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?
Something like
char suffix[6];
bool found = false;
for_each_gpio_property_name(suffix, "")
found = found || strends();
for_each_gpio_property_name(suffix, NULL)
found = found || (strcmp() == 0);
if (!found)
continue;
Of course with more thinking this may be optimized to avoid snprintf()
(probably with a new helper macro or so).
But see my next reply, I found something more interesting.
...
> > > + /* 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?
I dunno. Maybe something applies a default release in you case? Can you
investigate that?
...
> > > + 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".
OK.
> > > + return 0;
> > > +}
...
> > > +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().
Maybe, but as we noticed above my suggestion would make the code less error
prone to begin with.
> > > +}
...
> > > + 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.
scoped_guard() {
call_my_new_helper_which_is_easier to read();
}
ret = devm_add_...
OR
call_my_new_helper_which_is_easier to read()
{
guard()()
...
}
call_my_new_helper_which_is_easier to read();
ret = devm_add_...
> > > + 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;
> > > +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists