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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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