Commit 064c73af authored by Andy Shevchenko's avatar Andy Shevchenko Committed by Bartosz Golaszewski

gpio: pca953x: Synchronize interrupt handler properly

Since the commit aa58a21a ("gpio: pca953x: disable regmap locking")
the locking of regmap is disabled and that immediately introduces
a synchronization issue. It's easy to see when we try to monitor
more than one interrupt from the same chip.

It seems that the problem exists from the day one and even commit
6e20fb18 ("drivers/gpio/pca953x.c: add a mutex to fix race condition")
missed this.

Below are the traces and shell reproducers before and after proposed change.
Note duplicates in the IRQ events. /proc/interrupts also shows a deviation,
i.e. sum of children interrupts higher than parent's one.

When locking is disabled for regmap and no protection in IRQ handler
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ...
 gpioset-194          regmap_hw_write_start: i2c-INT3491:02 reg=2 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_start: i2c-INT3491:02 reg=4c count=2
 gpioset-194          regmap_hw_write_done: i2c-INT3491:02 reg=2 count=1
 gpioset-194          regmap_reg_read_cache: i2c-INT3491:02 reg=6 val=f5
 gpioset-194          regmap_reg_write: i2c-INT3491:02 reg=6 val=f5
 gpioset-194          regmap_hw_write_start: i2c-INT3491:02 reg=6 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_done: i2c-INT3491:02 reg=4c count=2
 ...

 % gpiomon gpiochip3 0 &
 % gpioset gpiochip3 1=0
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     302.782583765]
 % gpiomon gpiochip3 2 &
 % gpioset gpiochip3 1=0
 event:  RISING EDGE offset: 2 timestamp: [     312.033148829]
 event: FALLING EDGE offset: 0 timestamp: [     312.022757525]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 2 timestamp: [     316.201148473]
 event:  RISING EDGE offset: 0 timestamp: [     316.191759599]

When locking is disabled for regmap and protection in IRQ handler
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ...
 gpioset-202          regmap_hw_write_start: i2c-INT3491:02 reg=2 count=1
 gpioset-202          regmap_hw_write_done: i2c-INT3491:02 reg=2 count=1
 gpioset-202          regmap_reg_read_cache: i2c-INT3491:02 reg=6 val=fd
 gpioset-202          regmap_reg_write: i2c-INT3491:02 reg=6 val=fd
 gpioset-202          regmap_hw_write_start: i2c-INT3491:02 reg=6 count=1
 gpioset-202          regmap_hw_write_done: i2c-INT3491:02 reg=6 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_start: i2c-INT3491:02 reg=4c count=2
 irq/31-i2c-INT3-139  regmap_hw_read_done: i2c-INT3491:02 reg=4c count=2
 ...

 % gpiomon gpiochip3 0 &
 % gpioset gpiochip3 1=0
 event: FALLING EDGE offset: 0 timestamp: [     531.330078107]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     532.912239128]
 % gpiomon gpiochip3 2 &
 % gpioset gpiochip3 1=0
 event: FALLING EDGE offset: 0 timestamp: [     539.633669484]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     542.256978461]

Fixes: 6e20fb18 ("drivers/gpio/pca953x.c: add a mutex to fix race condition")
Depends-on: 35d13d94 ("gpio: pca953x: convert to use bitmap API")
Depends-on: 49427232 ("gpio: pca953x: Perform basic regmap conversion")
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Roland Stigge <stigge@antcom.de>
Signed-off-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: default avatarBartosz Golaszewski <bgolaszewski@baylibre.com>
parent b3a9e3b9
...@@ -734,14 +734,16 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid) ...@@ -734,14 +734,16 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
struct gpio_chip *gc = &chip->gpio_chip; struct gpio_chip *gc = &chip->gpio_chip;
DECLARE_BITMAP(pending, MAX_LINE); DECLARE_BITMAP(pending, MAX_LINE);
int level; int level;
bool ret;
if (!pca953x_irq_pending(chip, pending)) mutex_lock(&chip->i2c_lock);
return IRQ_NONE; ret = pca953x_irq_pending(chip, pending);
mutex_unlock(&chip->i2c_lock);
for_each_set_bit(level, pending, gc->ngpio) for_each_set_bit(level, pending, gc->ngpio)
handle_nested_irq(irq_find_mapping(gc->irq.domain, level)); handle_nested_irq(irq_find_mapping(gc->irq.domain, level));
return IRQ_HANDLED; return IRQ_RETVAL(ret);
} }
static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base) static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment