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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Sep 18, 2021 at 5:22 PM Wolfram Sang
<wsa+renesas@sang-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