Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race

[Jean Delvare]

Hi Ray,

On Thu, 2024-06-20 at 07:29 +0300, Ian Ray wrote:
> Ensure that `i2c_lock' is held when setting interrupt latch and mask in
> pca953x_irq_bus_sync_unlock() in order to avoid races.
> 
> The other (non-probe) call site pca953x_gpio_set_multiple() ensures the
> lock is held before calling pca953x_write_regs().
> 
> The problem occurred when a request raced against irq_bus_sync_unlock()
> approximately once per thousand reboots on an i.MX8MP based system.
> 
>  * Normal case
> 
>    0-0022: write register AI|3a {03,02,00,00,01} Input latch P0
>    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
>    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
>    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
> 
>  * Race case
> 
>    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
>    0-0022: write register AI|08 {03,02,00,00,01} *** Wrong register ***
>    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
>    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
> 

I have more questions on this. Where does the above log come from?
Specifically, at which layer (bus driver, regmap, gpio device drier)?
What do these values represent exactly? Which GPIO chip was used on
your system? Which i2c bus driver is being used on that system? What
are the "requests" you mention in the description above?

I'm asking because I do not understand how writing to the wrong
register can happen, even without holding i2c_lock in
pca953x_irq_bus_sync_unlock(). The i2c layer has a per-i2c_adapter lock
which is taken before any bus transfer, so it isn't possible that two
transfers collide at the bus level. So the lack of locking at the
device driver level could lead to data corruption (for example read-
modify-write cycles overlapping), but not to data being written to the
wrong register.

As a side note, I dug through the history of the gpio-pca953x driver
and found that i2c_lock was introduced before the driver was converted
to regmap by:

commit 6e20fb18054c179d7e64c0af43d855b9310a3394
Author: Roland Stigge
Date:   Thu Feb 10 15:01:23 2011 -0800

    drivers/gpio/pca953x.c: add a mutex to fix race condition

The fix added locking around read-modify-write cycles (which was indeed
needed) and also around simple register reads (which I don't think was
needed).

It turns out that regmap has its own protection around read-modify-
write cycles (see regmap_update_bits_base) so I think several uses of
i2c_lock should have been removed from the gpio-pca953x driver when it
was converted to regmap as they became redundant then. This driver-side
lock is still needed in a number of functions though, where the read-
modify-write is handled outside of regmap (for example in
pca953x_gpio_set_multiple).

Thanks,
-- 
Jean Delvare
SUSE L3 Support