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