On Tue, Nov 23, 2021 at 05:49:02PM +0100, Wolfram Sang 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. ... > +Result is a .sr file to be consumed with PulseView or sigrok-cli from the free > +`sigrok`_ project. It is a zip file which also contains the binary sample data > +which may be consumed by other software. The filename is the logic analyzer > +instance name plus a since-epoch timestamp. > + > +.. _sigrok: https://sigrok.org/ Alas, yet another tool required... (Sad thoughts since recently has installed PicoScope software). ... > kgdb > kselftest > kunit/index > + gpio-sloppy-logic-analyzer Above looks like ordered, do we need some groups here or so? ... > + mutex_lock(&priv->lock); > + if (priv->blob_dent) { Redundant (i.e. duplicate). > + debugfs_remove(priv->blob_dent); > + priv->blob_dent = NULL; > + } ... > +gpio_err: A bit confusing name. What about enable_irq_and_free_data: ? > + preempt_enable_notrace(); > + local_irq_enable(); > + if (ret) > + dev_err(priv->dev, "couldn't read GPIOs: %d\n", ret); > + > + kfree(priv->trig_data); > + priv->trig_data = NULL; > + priv->trig_len = 0; ... > +static int gpio_la_poll_probe(struct platform_device *pdev) > +{ > + struct gpio_la_poll_priv *priv; > + struct device *dev = &pdev->dev; > + const char *devname = dev_name(dev); > + const char *gpio_names[GPIO_LA_MAX_PROBES]; > + char *meta = NULL; > + unsigned int i, meta_len = 0; > + int ret; Perhaps unsigned int i, meta_len = 0; char *meta = NULL; int ret; ... > + ret = device_property_read_string_array(dev, "probe-names", gpio_names, > + priv->descs->ndescs); > + if (ret >= 0 && ret != priv->descs->ndescs) > + ret = -ENODATA; Don't remember if we already discussed this error code, but data is there, it's not correct. EBADSLT? EBADR? ECHRNG? > + if (ret < 0) > + return dev_err_probe(dev, ret, "error naming the GPIOs"); ... > + for (i = 0; i < priv->descs->ndescs; i++) { > + unsigned int add_len; > + char *new_meta, *consumer_name; > + > + if (gpiod_cansleep(priv->descs->desc[i])) > + return -EREMOTE; > + > + consumer_name = kasprintf(GFP_KERNEL, "%s: %s", devname, gpio_names[i]); > + if (!consumer_name) > + return -ENOMEM; > + gpiod_set_consumer_name(priv->descs->desc[i], consumer_name); > + kfree(consumer_name); > + > + /* '10' is length of 'probe00=\n\0' */ > + add_len = strlen(gpio_names[i]) + 10; > + > + new_meta = devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL); > + if (!new_meta) > + return -ENOMEM; > + > + meta = new_meta; > + meta_len += snprintf(meta + meta_len, add_len, "probe%02u=%s\n", > + i + 1, gpio_names[i]); Do we really need the 'probe%02u=' part? It's redundant since it may be derived from the line number of the output (and it always in [1..ndescs+1]). > + } ... > + dev_info(dev, "initialized"); Is it useful? ... > +print_help() > +{ > + cat <<EOF cat << EOF is slightly easier to read. > +EOF > +} ... > +set_newmask() > +{ > + for f in $(find "$1" -iname "$2"); do echo "$newmask" > "$f" 2>/dev/null || true; done While here it's okay, the rule of thumb is never use `for` or `while` against the list of filenames. > +} ... > +init_cpu() > +{ > + isol_cpu="$1" > + [ -d $cpusetdir ] || mkdir $cpusetdir `mkdir -p` and drop needless test. > + mount | grep -q $cpusetdir || mount -t cpuset cpuset $cpusetdir > + [ -d "$lacpusetdir" ] || mkdir "$lacpusetdir" `mkdir -p` and drop needless test. > + cur_cpu="$(cat "$lacpusetdir"/cpus)" > + [ "$cur_cpu" = "$isol_cpu" ] && return > + [ -z "$cur_cpu" ] || fail "CPU$isol_cpu requested but CPU$cur_cpu already isolated" > + > + echo "$isol_cpu" > "$lacpusetdir"/cpus || fail "Could not isolate CPU$isol_cpu. Does it exist?" > + echo 1 > "$lacpusetdir"/cpu_exclusive > + echo 0 > "$lacpusetdir"/mems > + > + oldmask=$(cat /proc/irq/default_smp_affinity) > + val=$((0x$oldmask & ~(1 << isol_cpu))) > + newmask=$(printf "%x" $val) Can be on one line (in a single expression). > + set_newmask '/proc/irq' '*smp_affinity' > + set_newmask '/sys/devices/virtual/workqueue/' 'cpumask' > + > + # Move tasks away from isolated CPU > + for p in $(ps -o pid | tail -n +2); do > + mask=$(taskset -p "$p") || continue > + # Ignore tasks with a custom mask, i.e. not equal $oldmask > + [ "${mask##*: }" = "$oldmask" ] || continue > + taskset -p "$newmask" "$p" || continue > + done 2>/dev/null >/dev/null `> /dev/null 2>&1` is idiomatic. And I think there is actually a subtle difference between two. > + echo 1 > /sys/module/rcupdate/parameters/rcu_cpu_stall_suppress > + > + cpufreqgov="/sys/devices/system/cpu/cpu$isol_cpu/cpufreq/scaling_governor" > + [ -w "$cpufreqgov" ] && echo 'performance' > "$cpufreqgov" || true > +} ... > +parse_triggerdat() > +{ > + oldifs="$IFS" > + IFS=','; for trig in $1; do > + mask=0; val1=0; val2=0 > + IFS='+'; for elem in $trig; do > + chan=${elem%[lhfrLHFR]} > + mode=${elem#$chan} > + # Check if we could parse something and the channel number fits > + [ "$chan" != "$elem" ] && [ "$chan" -le $max_chans ] || fail "Trigger syntax error: $elem" No need to execute `test` twice: [ "$chan" != "$elem" -a "$chan" -le $max_chans ] || fail "Trigger syntax error: $elem" > + bit=$((1 << (chan - 1))) > + mask=$((mask | bit)) > + case $mode in > + [hH]) val1=$((val1 | bit)); val2=$((val2 | bit));; > + [fF]) val1=$((val1 | bit));; > + [rR]) val2=$((val2 | bit));; > + esac > + done > + trigger_bindat="$trigger_bindat$(printf '\\%o\\%o' $mask $val1)" > + [ $val1 -ne $val2 ] && trigger_bindat="$trigger_bindat$(printf '\\%o\\%o' $mask $val2)" `printf` with arguments may be split to a separate helper function. > + done > + IFS="$oldifs" > +} > + > +do_capture() > +{ > + taskset "$1" echo 1 > "$lasysfsdir"/capture || fail "Capture error! Check kernel log" Shouldn't this function setup signal TRAPs? > + srtmp=$(mktemp -d) > + echo 1 > "$srtmp"/version > + cp "$lasysfsdir"/sample_data "$srtmp"/logic-1-1 > + cat > "$srtmp"/metadata <<EOF cat > "$srtmp"/metadata << EOF > +[global] > +sigrok version=0.2.0 > + > +[device 1] > +capturefile=logic-1 > +total probes=$(wc -l < "$lasysfsdir"/meta_data) > +samplerate=${samplefreq}Hz > +unitsize=1 > +EOF > + cat "$lasysfsdir"/meta_data >> "$srtmp"/metadata > + > + zipname="$outputdir/${lasysfsdir##*/}-$(date +%s).sr" > + zip -jq "$zipname" "$srtmp"/* > + rm -rf "$srtmp" > + delay_ack=$(cat "$lasysfsdir"/delay_ns_acquisition) > + [ "$delay_ack" -eq 0 ] && delay_ack=1 > + echo "Logic analyzer done. Saved '$zipname'" > + echo "Max sample frequency this time: $((1000000000 / delay_ack))Hz." > +} > + > +rep=$(getopt -a -l cpu:,duration-us:,help,instance:,kernel-debug-dir:,num_samples:,output-dir:,sample_freq:,trigger: -o c:d:hi:k:n:o:s:t: -- "$@") || exit 1 > +eval set -- "$rep" > +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 command line: $*";; $@ is better, actually one should never use $*. > + esac > + shift > +done ... Wondering, shouldn't be a simple validator before start that we have commands present, such as zip? -- With Best Regards, Andy Shevchenko
