Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 19 Sep 2021 23:27:14 +0300 | 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@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
| |