lkml.org 
[lkml]   [2021]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2021-09-19 22:28    [W:0.064 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site