Re: [PATCH v7 2/3] counter: add GPIO-based counter driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Sparse initialization of the `signals` array causes a NULL pointer dereference and missed registrations if `index1` is omitted but `index2` is present.
- [High] Counter values incorrectly saturate at 0 and ceiling instead of wrapping around, permanently breaking position tracking for quadrature encoders.
- [Medium] Torn reads of 64-bit state variables (`value`, `preset`, `ceiling`) on 32-bit architectures due to lockless access.
- [Medium] Missing `cp->enabled` check in index pulse ISR allows disabled counters to be modified and generate events.
--

--- Patch [2]: [PATCH v7 2/3] counter: add GPIO-based counter driver ---
commit 71f827659851f1023e187d6b98a7113441670997
Author: Wadim Mueller <wafgo01@gmail.com>

counter: add GPIO-based counter driver

Add a platform driver that turns plain GPIOs into a counter device
supporting quadrature, pulse-direction, and increase/decrease modes.

> diff --git a/drivers/counter/gpio-counter.c b/drivers/counter/gpio-counter.c
> new file mode 100644
> index 0000000000000..ceb79375d85b3
> --- /dev/null
> +++ b/drivers/counter/gpio-counter.c

[ ... ]

> +/* priv->lock held. */
> +static void gpio_counter_value_step(struct gpio_counter_count_priv *cp)
> +{
> +	if (cp->direction == COUNTER_COUNT_DIRECTION_FORWARD) {
> +		if (cp->value < cp->ceiling)
> +			cp->value++;
> +	} else {
> +		if (cp->value > 0)
> +			cp->value--;
> +	}
> +}

[Severity: High]
Will this clamp the counter instead of letting it wrap around?

If the hardware oscillates or moves backward past 0, the counter will stick
at 0. Moving forward again will increment from 0, which could destroy the
absolute position mapping for quadrature encoders. Should this wrap around
modulo the ceiling instead?

[ ... ]

> +static void gpio_counter_index_pulse(struct counter_device *counter,
> +				     enum gpio_counter_count_id count_id)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count_id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (cp->preset_enabled)
> +		cp->value = (cp->preset > cp->ceiling) ? cp->ceiling : cp->preset;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	counter_push_event(counter, COUNTER_EVENT_INDEX, count_id);
> +}

[Severity: Medium]
Is a check for cp->enabled needed here?

When disabling a counter in gpio_counter_enable_write(), cp->enabled is set
to false and the spinlock is released before disable_irq() is called. If an
index edge fires in this narrow window, it looks like this could update
cp->value and push a COUNTER_EVENT_INDEX event despite the driver state
being logically disabled.

[ ... ]

> +static int gpio_counter_count_read(struct counter_device *counter,
> +				   struct counter_count *count, u64 *val)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +
> +	*val = cp->value;
> +
> +	return 0;
> +}

[Severity: Medium]
Can this cause torn reads on 32-bit architectures?

Since cp->value is a 64-bit variable, reading it locklessly on a 32-bit
architecture requires two 32-bit loads. If the hardware ISR interrupts
these loads to increment the value across a 32-bit boundary, the reader
might fuse the old and new halves together resulting in a large, spurious
jump in the reported value.

This also seems to apply to the lockless reads of cp->ceiling and
cp->preset later in this file.

[ ... ]

> +static int gpio_counter_setup_signals(struct gpio_counter_priv *priv)
> +{
> +	priv->signals[GPIO_COUNTER_SIGNAL_A].id   = GPIO_COUNTER_SIGNAL_A;
> +	priv->signals[GPIO_COUNTER_SIGNAL_A].name = "Signal A";
> +	priv->signals[GPIO_COUNTER_SIGNAL_B].id   = GPIO_COUNTER_SIGNAL_B;
> +	priv->signals[GPIO_COUNTER_SIGNAL_B].name = "Signal B";
> +
> +	if (priv->count_priv[GPIO_COUNTER_COUNT_1].has_index) {
> +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX1].id =
> +			GPIO_COUNTER_SIGNAL_INDEX1;
> +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX1].name = "Index 1";
> +	}
> +	if (priv->count_priv[GPIO_COUNTER_COUNT_2].has_index) {
> +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX2].id =
> +			GPIO_COUNTER_SIGNAL_INDEX2;
> +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX2].name = "Index 2";
> +	}
> +
> +	return 2 + priv->count_priv[GPIO_COUNTER_COUNT_1].has_index +
> +	       priv->count_priv[GPIO_COUNTER_COUNT_2].has_index;
> +}

[Severity: High]
Does this setup handle the case where a device tree omits index1 but
includes index2?

If index1 is missing but index2 is present, the index 2 signal is mapped to
priv->signals[3] (GPIO_COUNTER_SIGNAL_INDEX2), leaving priv->signals[2]
zero-initialized.

Since this function returns a compressed count of 3, the counter framework
will iterate from index 0 to 2, causing a NULL pointer dereference on the
empty name at signals[2] and skipping the valid signal at signals[3].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714081709.17846-1-wafgo01@gmail.com?part=2