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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vdv=0i05EitMi6JjbjML-jFD_1M0q7ps2KVHcN4UtFU-w@mail.gmail.com>
Date:   Sun, 19 Sep 2021 23:27:14 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Wolfram Sang <wsa+renesas@...g-engineering.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

On Sat, Sep 18, 2021 at 5:22 PM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
>
> This is a sloppy logic analyzer using GPIOs. It comes with a script to
> isolate a CPU for polling. While this is definitely not a production
> level analyzer, it can be a helpful first view when remote debugging.
> Read the documentation for details.

Thanks for update, my comments below.

...

> +       /* 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,

> +       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() ?

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

...

> +       dev_info(dev, "initialized");

Unneeded noise.

...

> +       [ -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 ...

...

> +       # 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?

> +               taskset -p "$newmask" "$p" || continue
> +       done 2>/dev/null >/dev/null

...

> +while true; do
> +       case "$1" in
> +       -c|--cpu) initcpu="$2"; shift;;
> +       -d|--duration-us) duration="$2"; shift;;
> +       -h|--help) print_help; exit 0;;
> +       -i|--instance) lainstance="$2"; shift;;
> +       -k|--kernel-debug-dir) debugdir="$2"; shift;;
> +       -n|--num_samples) numsamples="$2"; shift;;
> +       -o|--output-dir) outputdir="$2"; shift;;
> +       -s|--sample_freq) samplefreq="$2"; shift;;
> +       -t|--trigger) triggerdat="$2"; shift;;
> +       --) break;;
> +       *) fail "error parsing commandline: $*";;

command line

> +       esac
> +       shift
> +done

...

> +[ "$samplefreq" -eq 0 ] && fail "Invalid sample frequency"

Same as above, use '... || fail ...' approach.

...

> +if [ -n "$lainstance" ]; then

Shouldn't be rather '-d' ?

> +       lasysfsdir="$sysfsdir/$lainstance"
> +else
> +       lasysfsdir="$(find "$sysfsdir" -mindepth 1 -type d -print -quit)"
> +fi

...

> +[ ! -d "$lacpusetdir" ] && echo "Auto-Isolating CPU1" && init_cpu 1

This ! along with double && is hard to read. Simply

[ -d ... ] || {
  ...
}

no?

...

> +[ -z "$workcpu" ] && fail "No isolated CPU found"

As above, use ' ... || fail ...' approach.

-- 
With Best Regards,
Andy Shevchenko