linux-kernel - Re: [PATCH v2 1/1] gpio: add sloppy logic analyzer using polling

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: <YUhGkBdXJUI3XadP@ninjato>
Date:   Mon, 20 Sep 2021 10:30:08 +0200
From:   Wolfram Sang <wsa+renesas@...g-engineering.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-renesas-soc@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] gpio: add sloppy logic analyzer using polling

Hi Andy,

thanks for the prompt review again!

> > +       /* upper limit is arbitrary */
> 
> Not really. I believe if the upper limit is > PAGE_SIZE, you would get
> -ENOMEM with much higher chances. So, I think the comment should be
> amended,

? Dunno, maybe it is not arbitrary that it is < PAGE_SIZE but other than
that the value I chose is arbitrary. There is no technical reason for
2048.

> 
> > +       if (count > 2048 || count & 1)
> > +               return -EINVAL;
> 
> ...
> 
> > +       ret = device_property_read_string_array(dev, "probe-names", gpio_names,
> > +                                               priv->descs->ndescs);
> > +       if (ret >= 0 && ret != priv->descs->ndescs)
> > +               ret = -ENODATA;
> > +       if (ret < 0) {
> 
> > +               dev_err(dev, "error naming the GPIOs: %d\n", ret);
> > +               return ret;
> > +       }
> 
> Perhaps
> 
>   return dev_err_probe() ?

Reading strings from DT can be deferred? I don't think so.

> And I think it might be split into two conditionals with
> distinguishable error messages.

I think "something is wrong with the names" is helpful enough for the
user.


> > +       dev_info(dev, "initialized");
> 
> Unneeded noise.

Nope, I added it because when I was adding more instances, this proved
useful. I'd agree if this is a regular driver. But this is a
only-for-special-case-debugging one.

> > +       [ -n "$cur_cpu" ] && fail "CPU$isol_cpu requested but CPU$cur_cpu already isolated"
> 
> For the sake of style (handle errors on the error) I would use
> 
> [ -z "..." ] || fail ...

I'll think about it. On first glimpse, this doesn't look more readable
to me. "if this is true then do that" is super readable in my book. But
yes, when calling external programs, I need '||' anyhow, true.

> > +       # Move tasks away from isolated CPU
> > +       for p in $(ps -o pid | tail -n +2); do
> > +               mask=$(taskset -p "$p") || continue
> > +               [ "${mask##*: }" != "$oldmask" ] && continue
> 
> Perhaps
> 
>   [ ... = ... ] || continue
> 
> to be in alignment with the rest of the similar lines here?

Yes.

> > +       *) fail "error parsing commandline: $*";;
> 
> command line

OK.

> > +if [ -n "$lainstance" ]; then
> 
> Shouldn't be rather '-d' ?

Nope, this is just a string for now. '-d' comes later with $lasysfsdir.

> > +[ ! -d "$lacpusetdir" ] && echo "Auto-Isolating CPU1" && init_cpu 1
> 
> This ! along with double && is hard to read. Simply

Same as above, will think about it. But "if there is not this directory,
then do a) and b)" is not hard to read in my book.

Happy hacking,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)