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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing direction state update on enable for PULSE_DIRECTION mode.
- [High] Reading GPIO pin states outside the spinlock in the ISRs allows out-of-order state evaluation, leading to spurious counter steps.
- [Medium] Enabling a count overwrites shared quadrature state, corrupting the active state machine of the other running count.
--

commit 002c8f1bc700b17784c2bbc34b75aada303eee61
Author: Wadim Mueller <wafgo01@gmail.com>

counter: add GPIO-based counter driver

This adds a platform driver to decode 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..8d9fa0b616d5c
> --- /dev/null
> +++ b/drivers/counter/gpio-counter.c

[ ... ]

> +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);
> +	struct gpio_counter_count_priv *cp =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_1];
> +	unsigned long flags;
> +	bool enabled;
> +	int a, b;
> +
> +	/* !! drops negative gpiod_get_value() errors. */
> +	a = !!gpiod_get_value(priv->gpio_a);
> +	b = !!gpiod_get_value(priv->gpio_b);
> +
> +	spin_lock_irqsave(&priv->lock, flags);

[Severity: High]
Does reading the GPIO pin states outside the spinlock here and in
gpio_counter_b_isr() create a data race that allows out-of-order state
evaluation?

If high-frequency edges on A and B pins trigger interrupts that are serviced
concurrently on different CPUs, CPU 0 could read the states locklessly, get
preempted, and then apply a stale state after CPU 1 has already acquired the
lock and updated it. This could rewind the Gray-code state machine and
generate spurious backward steps or drop valid transitions.

Would moving the gpiod_get_value() reads inside the lock ensure late-arriving
ISRs read the most current state and safely drop redundant edges?

[ ... ]

> +static irqreturn_t gpio_counter_b_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *c1 =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_1];
> +	struct gpio_counter_count_priv *c2 =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_2];
> +	bool c1_enabled, c2_enabled;
> +	unsigned long flags;
> +	int a, b;
> +
> +	a = !!gpiod_get_value(priv->gpio_a);
> +	b = !!gpiod_get_value(priv->gpio_b);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	c1_enabled = c1->enabled;
> +	c2_enabled = c2->enabled;
> +
> +	/* Count 1: A+B decoding on this B edge */
> +	if (c1_enabled) {
> +		switch (c1->function) {
> +		case COUNTER_FUNCTION_QUADRATURE_X4:
> +		case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		case COUNTER_FUNCTION_QUADRATURE_X1_B:
> +			if (!GPIO_COUNTER_STATE_CHANGED(priv->prev_a,
> +							priv->prev_b, a, b))
> +				break;
> +			c1->direction =
> +				GPIO_COUNTER_GET_DIRECTION(priv->prev_b, a);
> +			if (c1->function == COUNTER_FUNCTION_QUADRATURE_X1_B)
> +				gpio_counter_quadrature_x1_step(c1, b);
> +			else
> +				gpio_counter_value_step(c1);
> +			break;
> +
> +		case COUNTER_FUNCTION_PULSE_DIRECTION:
> +			/* B is the direction line */
> +			c1->direction = b ? COUNTER_COUNT_DIRECTION_BACKWARD
> +					  : COUNTER_COUNT_DIRECTION_FORWARD;
> +			break;

[Severity: High]
Is a direction state update missing on enable for PULSE_DIRECTION mode?

Since the direction is only updated here when the count is enabled, any B pin
toggles that happen while the counter is disabled are not tracked. When the
counter is subsequently enabled, gpio_counter_enable_write() does not
synchronize cp->direction with the current B pin level.

Could this cause the counter to step in the wrong direction until the B pin
toggles again?

[ ... ]

> +static int gpio_counter_enable_write(struct counter_device *counter,
> +				     struct counter_count *count, u8 enable)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +	bool current_state;
> +
> +	guard(mutex)(&priv->enable_lock);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	current_state = cp->enabled;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (current_state == enable)
> +		return 0;
> +
> +	/* disable_irq() may sleep, so it can not run under the spinlock. */
> +	if (enable) {
> +		spin_lock_irqsave(&priv->lock, flags);
> +		priv->prev_a = !!gpiod_get_value(priv->gpio_a);
> +		priv->prev_b = !!gpiod_get_value(priv->gpio_b);

[Severity: Medium]
Does enabling a count unconditionally overwrite the shared quadrature state,
corrupting the active state machine of the other running count?

Because priv->prev_a and priv->prev_b are updated here whenever any count is
enabled, enabling Count 2 while Count 1 is actively running will overwrite
Count 1's state tracker.

If an edge interrupt was pending for Count 1, its ISR might observe no state
change and incorrectly drop the step.

> +		cp->enabled = true;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		gpio_counter_enable_irqs(priv, count->id);
> +	} else {

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