linux-kernel - Re: [PATCH v3 08/11] gpio: sim: new testing module

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: <CAMRc=MfeCWNnXwqBBu3CcdHXQ5QnNPBh8EJRTCtyZau+RqE-0w@mail.gmail.com>
Date:   Fri, 12 Mar 2021 09:54:58 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Joel Becker <jlbec@...lplan.org>, Christoph Hellwig <hch@....de>,
        Shuah Khan <shuah@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Kent Gibson <warthog618@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-doc <linux-doc@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v3 08/11] gpio: sim: new testing module

On Wed, Mar 10, 2021 at 1:28 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>

[snip]

> > +
> > +static ssize_t gpio_sim_sysfs_line_show(struct device *dev,
> > +                                     struct device_attribute *attr,
> > +                                     char *buf)
> > +{
> > +     struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
> > +     struct gpio_sim_chip *chip = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     mutex_lock(&chip->lock);
> > +     ret = sprintf(buf, "%u\n", !!test_bit(line_attr->offset, chip->values));
>
> Shouldn't we use sysfs_emit() in a new code?
>

TIL it exists. :) I'll use it.

[snip]

> > +
> > +static ssize_t gpio_sim_config_dev_name_show(struct config_item *item,
> > +                                          char *page)
> > +{
> > +     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> > +     struct platform_device *pdev;
> > +     int ret;
> > +
> > +     mutex_lock(&config->lock);
> > +     pdev = config->pdev;
> > +     if (pdev)
> > +             ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
> > +     else
> > +             ret = sprintf(page, "n/a\n");
>
> I dunno '/' (slash) is a good character to be handled in a shell.
> I would prefer 'none' or 'not available' (I think space is easier,
> because the rules to escape much simpler: need just to take it into
> quotes, while / needs to be escaped separately).
>

My test cases work fine with 'n/a' but I can change it to 'none' if
it's less controversial.

[snip]

>
> Also don't know what the rules about using s*printf() in the configfs.
> Maybe we have sysfs_emit() analogue or it doesn't applicable here at all.
> Greg?
>

There's no configfs_emit() or anything similar. Output for simple
attributes must simply not exceed 4096 bytes. It used to be PAGE_SIZE,
now it's defined in fs/configfs/file.c as SIMPLE_ATTR_SIZE. There's no
need to check the length of the string here though as we're only
showing what we received from the user-space anyway and configfs makes
sure we don't get more than SIMPLE_ATTR_SIZE in the store callback.

[snip]

> > +
> > +static int gpio_sim_config_commit_item(struct config_item *item)
> > +{
> > +     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> > +     struct property_entry properties[GPIO_SIM_MAX_PROP];
> > +     struct platform_device_info pdevinfo;
> > +     struct platform_device *pdev;
> > +     unsigned int prop_idx = 0;
> > +
> > +     memset(&pdevinfo, 0, sizeof(pdevinfo));
> > +     memset(properties, 0, sizeof(properties));
> > +
> > +     mutex_lock(&config->lock);
> > +
> > +     properties[prop_idx++] = PROPERTY_ENTRY_U32("gpio-sim,nr-gpios",
> > +                                                 config->num_lines);
>
> > +     if (config->label[0] != '\0')
>
> I'm wondering if we need this check. Isn't core taking care of it?
>
> > +             properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
> > +                                                            config->label);
>
> > +     if (config->line_names)
>
> Ditto.
>
> > +             properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> > +                                             "gpio-line-names",
> > +                                             config->line_names,
> > +                                             config->num_line_names);
> > +

But I would be creating empty properties for nothing. Better to just
not have them at all.

[snip]

Bartosz