| 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
| ||
|
Date: Fri, 4 Aug 2023 18:03:47 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Kent Gibson <warthog618@...il.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [RFC PATCH] gpio: consumer: new virtual driver
On Thu, Aug 3, 2023 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
[snip]
>
> > +#include <linux/of_platform.h>
>
> Wrong header. Use mod_devicetable.h.
>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/timer.h>
>
> And general recommendation is to revisit this block and refine it accordingly.
>
I kept track of the interfaces I used for most part, so it should be
mostly fine.
[snip]
> ...
>
> > + flags = function == GPIO_CONSUMER_FUNCTION_MONITOR ?
> > + GPIOD_IN : GPIOD_OUT_HIGH;
> > + for (i = 0; i < num_lines; i++) {
> > + desc = devm_gpiod_get(dev, lines[i], flags);
> > + if (IS_ERR(desc))
> > + return dev_err_probe(dev, PTR_ERR(desc),
> > + "Failed to get GPIO '%s'\n",
> > + lines[i]);
>
> Would it make sense to request GPIOs via devm_gpiod_get_array() and then try
> the rest on them in a loop?
>
No it would not. gpiod_get_array() works for properties represented in DT as:
foo-gpios = <&chip ...>, <&chip ...>, <&chip ...>;
while what we have here is:
foo-gpios = <&chip ...>;
bar-gpios = <&chip ...>;
Which makes me think that I need to add proper documentation for this module.
[snip]
>
> > +static ssize_t
> > +gpio_consumer_lookup_config_offset_store(struct config_item *item,
> > + const char *page, size_t count)
> > +{
> > + struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
> > + struct gpio_consumer_device *dev = lookup->parent;
> > + int offset, ret;
> > +
> > + ret = kstrtoint(page, 0, &offset);
> > + if (ret)
> > + return ret;
> > +
> > + /* Use -1 to indicate lookup by name. */
> > + if (offset > (U16_MAX - 1))
> > + return -EINVAL;
>
> So, offset here may be negative. Is it okay?
>
Yes. If negative - lookup line by name, if positive, by chip and
offset. I will document this properly for v2.
> > + mutex_lock(&dev->lock);
> > +
> > + if (gpio_consumer_device_is_live_unlocked(dev)) {
> > + mutex_unlock(&dev->lock);
> > + return -EBUSY;
> > + }
> > +
> > + lookup->offset = offset;
> > +
> > + mutex_unlock(&dev->lock);
> > +
> > + return count;
> > +}
>
> ...
>
> > + if (flags & GPIO_OPEN_DRAIN)
> > + repr = "open-drain";
> > + else if (flags & GPIO_OPEN_SOURCE)
> > + repr = "open-source";
>
> Can it be both flags set?
>
No!
> > + else
> > + repr = "push-pull";
>
> ...
>
> > + if (sysfs_streq(page, "push-pull")) {
> > + lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
> > + } else if (sysfs_streq(page, "open-drain")) {
> > + lookup->flags &= ~GPIO_OPEN_SOURCE;
> > + lookup->flags |= GPIO_OPEN_DRAIN;
> > + } else if (sysfs_streq(page, "open-source")) {
> > + lookup->flags &= ~GPIO_OPEN_DRAIN;
> > + lookup->flags |= GPIO_OPEN_SOURCE;
> > + } else {
> > + count = -EINVAL;
> > + }
>
> I prefer to see some kind of the array of constant string literals and do
> sysfs_match_string() here
>
I would generally agree but if the flag values ever change to ones
that make the resulting string array have holes in it, match_string()
will suddenly stop working. I think that with bit flags defined
elsewhere it's safer and more readable to do the above.
> lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
> flag = sysfs_match_string(...);
> if (flag < 0)
> count = flag
> else
> lookup->flags |= flag;
>
> (or something similar). And respectively indexed access above.
>
> ...
>
> ...
>
> > + if (list_empty(&dev->lookup_list))
> > + return -ENODATA;
>
> Instead you may count nodes here and if 0, return an error, otherwise pass it
> to the callee.
I'm not following, please rephrase.
>
> > + swnode = gpio_consumer_make_device_swnode(dev);
> > + if (IS_ERR(swnode))
> > + return PTR_ERR(swnode);
>
> ...
>
> > +static ssize_t
> > +gpio_consumer_device_config_live_store(struct config_item *item,
> > + const char *page, size_t count)
> > +{
> > + struct gpio_consumer_device *dev = to_gpio_consumer_device(item);
> > + bool live;
> > + int ret;
> > +
> > + ret = kstrtobool(page, &live);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&dev->lock);
> > +
> > + if ((!live && !gpio_consumer_device_is_live_unlocked(dev)) ||
> > + (live && gpio_consumer_device_is_live_unlocked(dev)))
>
> if (live ^ gpio_consumer_device_is_live_unlocked(dev))
>
> ?
Nah, let's not use bitwise operators for boolean logic.
[snip]
I commented on the ones that needed it, for others, I'll fix them for v2.
Bart
Powered by blists - more mailing lists