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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Concurrent sysfs writes to the 'enable' attribute cause unbalanced IRQ enables and desynchronize the hardware state.
- [High] The Quadrature X1_A and X1_B modes are fundamentally broken and form a directional feedback loop, failing to decode direction.
- [High] GPIO states are sampled outside the spinlock in ISRs, causing state machine desynchronization under rapid concurrent edges.
- [Medium] Negative error codes from gpiod_get_value() are masked and incorrectly converted into logical HIGH states.
- [Medium] The counter saturates at bounds instead of wrapping around, violating Counter subsystem `ceiling` semantics.
- [Medium] The previous GPIO states are not synchronized when the counter is re-enabled, causing missed edges or spurious counts.
- [Medium] The Quadrature X2_A and X2_B modes do not verify if the triggering signal actually changed state, leading to false counts on noisy lines.
--

commit 9297ee567bedc28c8a463a9a4d24d31a9d00cc62
Author: Wadim Mueller <wafgo01@gmail.com>

counter: add GPIO-based counter driver

Add a platform driver that turns plain GPIOs into a counter device.
Edge interrupts on the signal-a, signal-b (and optional index) lines
are decoded in software using a 2-bit Gray-code parity trick for the
quadrature X4 mode and direct edge checks for the other modes.

> diff --git a/drivers/counter/gpio-counter.c b/drivers/counter/gpio-counter.c
> new file mode 100644
> index 0000000000000..f50cec33ae366
> --- /dev/null
> +++ b/drivers/counter/gpio-counter.c
[ ... ]
> +static void gpio_counter_update(struct gpio_counter_priv *priv, int delta)
> +{
> +	if (delta > 0) {
> +		priv->direction = COUNTER_COUNT_DIRECTION_FORWARD;
> +		if (priv->count >= priv->ceiling)
> +			return;
> +		priv->count++;
> +	} else if (delta < 0) {
> +		priv->direction = COUNTER_COUNT_DIRECTION_BACKWARD;
> +		if (priv->count == 0)
> +			return;
> +		priv->count--;
> +	}
> +}

[Severity: Medium]
Will saturating the counter at its limits break compatibility with the Counter
subsystem semantics?

The Counter ABI defines 'ceiling' as a wraparound limit for cyclic counters,
where counting past the ceiling wraps to 0, and counting below 0 wraps to the
ceiling. Saturating at the limits might cause issues for userspace
applications relying on cyclic tracking.

[ ... ]
> +static int gpio_counter_a_delta(struct gpio_counter_priv *priv, int a, int b,
> +				int prev_a, int prev_b)
> +{
> +	enum counter_count_direction dir;
> +
> +	switch (priv->function) {
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		if (!GPIO_COUNTER_STATE_CHANGED(prev_a, prev_b, a, b))
> +			return 0;
> +		dir = GPIO_COUNTER_GET_DIRECTION(prev_b, a);
> +		return (dir == COUNTER_COUNT_DIRECTION_FORWARD) ? 1 : -1;
> +
> +	case COUNTER_FUNCTION_QUADRATURE_X2_A:
> +		return (a == b) ? -1 : 1;

[Severity: Medium]
How does the X2_A mode handle noisy lines or contact bounce?

Unlike the X4 mode which checks for a state change, this mode unconditionally
returns 1 or -1 based on (a == b). If the signal line bounces, causing the ISR
to fire twice for the same logical state (where a == prev_a), it appears the
ISR will incorrectly register an additional count in the same direction.

Should there be a check to verify that the interrupted signal has actually
toggled?

> +
> +	case COUNTER_FUNCTION_QUADRATURE_X1_A:
> +		if (a && priv->direction == COUNTER_COUNT_DIRECTION_FORWARD)
> +			return 1;
> +		if (!a && priv->direction == COUNTER_COUNT_DIRECTION_BACKWARD)
> +			return -1;
> +		return 0;

[Severity: High]
Is it possible for the X1_A and X1_B modes to correctly decode direction?

The logic relies on checking the state against priv->direction. Since
priv->direction is initialized to forward, and only flips to backward if
delta < 0, but delta can only be < 0 if the direction is already backward,
this looks like a positive feedback loop. It seems the counter will only
ever count upward.

Should the state of the opposite phase signal be used to determine direction
instead?

[ ... ]
> +static irqreturn_t gpio_counter_a_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +	int a, b, delta;
> +
> +	/* !! normalises away negative gpiod_get_value() errors. */
> +	a = !!gpiod_get_value(priv->gpio_a);

[Severity: Medium]
What happens if gpiod_get_value() returns a negative error code?

The comment explicitly states this normalizes away errors, but treating
hardware read failures (like -EINVAL) as valid logical HIGH states could
inject phantom edges into the quadrature decoder, corrupting the absolute
count value.

Should the ISR explicitly check for negative returns and exit instead of
masking the error?

> +	b = !!gpiod_get_value(priv->gpio_b);
> +
> +	spin_lock_irqsave(&priv->lock, flags);

[Severity: High]
If A and B edges occur in quick succession on an SMP system, could different
CPUs handle the interrupts concurrently and sample the GPIOs out-of-order
relative to when they acquire the spinlock?

This could cause the internal software state machine to evaluate transitions
that are out of sequence with the actual hardware, leading to missed counts or
interpreting forward motion as backwards counting.

Since these are non-sleeping GPIOs, should the reads be moved inside the lock
to ensure atomic transition evaluation?

[ ... ]
> +static int gpio_counter_enable_write(struct counter_device *counter,
> +				     struct counter_count *count, u8 enable)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +	bool want = enable;
> +	bool changed;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	changed = priv->enabled != want;
> +	if (changed)
> +		priv->enabled = want;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (!changed)
> +		return 0;
> +
> +	if (want) {
> +		enable_irq(priv->irq_a);

[Severity: High]
If two threads write to the 'enable' attribute concurrently, could the
hardware toggles be reordered relative to the lock acquisitions?

Thread A writes 0 and Thread B writes 1. Both check the state under the lock,
but Thread B could call enable_irq() before Thread A calls disable_irq(). This
would trigger an 'Unbalanced enable for IRQ' warning since enable_irq() is
called on an already-enabled IRQ.

Should the physical enable_irq() and disable_irq() calls be moved inside the
spinlock, or should another synchronization mechanism be used to prevent this
race?

[Severity: Medium]
Are the previous GPIO states synchronized when the counter is re-enabled?

priv->prev_a and priv->prev_b are initialized during probe(). If the hardware
moves while the counter is disabled, the first edge interrupt after re-enabling
will compare the new GPIO levels against stale 'prev' states. This could lead
to misinterpreting the first transition, resulting in false direction
evaluations or dropped counts.

Should priv->prev_a and priv->prev_b be sampled and updated inside this
function before enabling the IRQs?

> +		enable_irq(priv->irq_b);
> +		if (priv->irq_index)
> +			enable_irq(priv->irq_index);
> +	} else {
> +		disable_irq(priv->irq_a);
> +		disable_irq(priv->irq_b);
> +		if (priv->irq_index)
> +			disable_irq(priv->irq_index);
> +	}
> +
> +	return 0;
> +}

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