GPIO chips often have data input and output fields aliased to the same
offset. Since gpio-regmap performs a value update before the direction
update (to prevent glitches), a pin currently configured as input may
cause regmap_update_bits() to not perform a write.
This may cause unexpected line states when the current input state
equals the requested output state:
OUT IN OUT
DIR ''''''\...|.../''''''
pin ....../'''|'''\......
(1) (2) (3)
1. Line was configurad as out-low, but is reconfigured to input.
External logic results in high value.
2. Set output value high. regmap_update_bits() sees the value is
already high and discards the register write.
3. Line is switched to output, maintaining the stale output config
(low) instead of the requested config (high).
By switching to regmap_write_bits(), a write of the requested output
value can be forced, irrespective of the read state. Do this only for
aliased registers, so the more efficient regmap_update_bits() can still
be used for distinct registers.
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
drivers/gpio/gpio-regmap.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index ab9e4077fa60..ba3c19206ccf 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -93,7 +93,7 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
{
struct gpio_regmap *gpio = gpiochip_get_data(chip);
unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
- unsigned int reg, mask;
+ unsigned int reg, mask, mask_val;
int ret;
ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &mask);
@@ -101,9 +101,15 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
return ret;
if (val)
- ret = regmap_update_bits(gpio->regmap, reg, mask, mask);
+ mask_val = mask;
else
- ret = regmap_update_bits(gpio->regmap, reg, mask, 0);
+ mask_val = 0;
+
+ /* ignore input values which shadow the old output value */
+ if (gpio->reg_dat_base == gpio->reg_set_base)
+ ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
+ else
+ ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
return ret;
}
--
2.51.0Hi,
On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote:
> GPIO chips often have data input and output fields aliased to the same
> offset. Since gpio-regmap performs a value update before the direction
> update (to prevent glitches), a pin currently configured as input may
> cause regmap_update_bits() to not perform a write.
>
> This may cause unexpected line states when the current input state
> equals the requested output state:
>
> OUT IN OUT
> DIR ''''''\...|.../''''''
>
> pin ....../'''|'''\......
> (1) (2) (3)
>
> 1. Line was configurad as out-low, but is reconfigured to input.
> External logic results in high value.
> 2. Set output value high. regmap_update_bits() sees the value is
> already high and discards the register write.
> 3. Line is switched to output, maintaining the stale output config
> (low) instead of the requested config (high).
>
> By switching to regmap_write_bits(), a write of the requested output
> value can be forced, irrespective of the read state. Do this only for
> aliased registers, so the more efficient regmap_update_bits() can still
> be used for distinct registers.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> drivers/gpio/gpio-regmap.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index ab9e4077fa60..ba3c19206ccf 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -93,7 +93,7 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
> {
> struct gpio_regmap *gpio = gpiochip_get_data(chip);
> unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> - unsigned int reg, mask;
> + unsigned int reg, mask, mask_val;
> int ret;
>
> ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &mask);
> @@ -101,9 +101,15 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
> return ret;
>
> if (val)
> - ret = regmap_update_bits(gpio->regmap, reg, mask, mask);
> + mask_val = mask;
> else
> - ret = regmap_update_bits(gpio->regmap, reg, mask, 0);
> + mask_val = 0;
> +
> + /* ignore input values which shadow the old output value */
> + if (gpio->reg_dat_base == gpio->reg_set_base)
> + ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
> + else
> + ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
I wonder if we should just switch to regmap_write_bits() entirely.
In patch 2, you've wrote:
> The generic gpiochip implementation stores a shadow value of the
> pin output data, which is updated and written to hardware on output
> data changes. Pin input values are always obtained by reading the
> aliased data register from hardware.
I couldn't find that in the code though. But if the gpiolib only
updates the output register on changes, the write part in
regmap_update_bits() would always occur.
In any case, feel free to add.
Reviewed-by: Michael Walle <mwalle@kernel.org>
-michael
Hi Michael, On Tue, 2025-10-21 at 09:33 +0200, Michael Walle wrote: > > + /* ignore input values which shadow the old output value */ > > + if (gpio->reg_dat_base == gpio->reg_set_base) > > + ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val); > > + else > > + ret = regmap_update_bits(gpio->regmap, reg, mask, > > mask_val); > > I wonder if we should just switch to regmap_write_bits() entirely. It would certainly make the code simpler, but it may impact performance a bit. E.g. a bit-banged I2C bus doesn't need to update the output value (only the direction), so using regmap_update_bits() saves half the writes when the output data register can be properly cached. Similar to gpio-mmio.c, gpio-regmap.c could also provide multiple setters, so the branch(es) in this call would only occur once at init, to sacrifice code size for a bit of performance. Feel free to let me know what your preference is, otherwise I'll keep the patch as-is. > > In patch 2, you've wrote: > > > The generic gpiochip implementation stores a shadow value of the > > pin output data, which is updated and written to hardware on output > > data changes. Pin input values are always obtained by reading the > > aliased data register from hardware. > > I couldn't find that in the code though. But if the gpiolib only > updates the output register on changes, the write part in > regmap_update_bits() would always occur. I was referring to bgpio_set(). AFAICT gpiod_direction_input() and gpiod_direction_output() call the driver unconditionally, without checking if the gpiolib state would change for this pin. > > In any case, feel free to add. > > Reviewed-by: Michael Walle <mwalle@kernel.org> Thanks! Best, Sander
Hi Sander, On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote: > GPIO chips often have data input and output fields aliased to the same > offset. Since gpio-regmap performs a value update before the direction > update (to prevent glitches), a pin currently configured as input may > cause regmap_update_bits() to not perform a write. > > This may cause unexpected line states when the current input state > equals the requested output state: > > OUT IN OUT > DIR ''''''\...|.../'''''' > > pin ....../'''|'''\...... > (1) (2) (3) > > 1. Line was configurad as out-low, but is reconfigured to input. > External logic results in high value. > 2. Set output value high. regmap_update_bits() sees the value is > already high and discards the register write. > 3. Line is switched to output, maintaining the stale output config > (low) instead of the requested config (high). > > By switching to regmap_write_bits(), a write of the requested output > value can be forced, irrespective of the read state. Do this only for > aliased registers, so the more efficient regmap_update_bits() can still > be used for distinct registers. Have you looked at the .volatile_reg callback of the regmap api? You might use the same heuristics, i.e. .reg_dat_base == .reg_set_base to implement that callback. That way you'd just have to (unconditionally) set that callback in gpio_regmap_register() and regmap should take care of the rest. -michael
Hi Michael,
On Mon, 2025-10-20 at 15:02 +0200, Michael Walle wrote:
> Hi Sander,
>
> On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote:
> > GPIO chips often have data input and output fields aliased to the same
> > offset. Since gpio-regmap performs a value update before the direction
> > update (to prevent glitches), a pin currently configured as input may
> > cause regmap_update_bits() to not perform a write.
> >
> > This may cause unexpected line states when the current input state
> > equals the requested output state:
> >
> > OUT IN OUT
> > DIR ''''''\...|.../''''''
> >
> > pin ....../'''|'''\......
> > (1) (2) (3)
> >
> > 1. Line was configurad as out-low, but is reconfigured to input.
> > External logic results in high value.
> > 2. Set output value high. regmap_update_bits() sees the value is
> > already high and discards the register write.
> > 3. Line is switched to output, maintaining the stale output config
> > (low) instead of the requested config (high).
> >
> > By switching to regmap_write_bits(), a write of the requested output
> > value can be forced, irrespective of the read state. Do this only for
> > aliased registers, so the more efficient regmap_update_bits() can still
> > be used for distinct registers.
>
> Have you looked at the .volatile_reg callback of the regmap api?
> You might use the same heuristics, i.e. .reg_dat_base == .reg_set_base
> to implement that callback. That way you'd just have to
> (unconditionally) set that callback in gpio_regmap_register() and
> regmap should take care of the rest.
Maybe I'm missing something here, but I'm not sure what difference that would
make. .volatile_reg is part of the regmap config, so when gpio_regmap_register()
is called, the regmap has already been created. We can't change the
.volatile_reg callback (and we shouldn't, it's up to the user to define it).
FWIW, I did test this with a regmap config that marks the aliased data registers
as volatile. The issue isn't that an invalid cache is being read. The problem is
that writes are being optimized away when they shouldn't:
1. Read register from hardware (volatile) or cache (non-volatile).
2. Update bits in mask to requested value
3. Write updated value to hardware if:
A. This is a forced write (i.e. regmap_write_bits), or
B. The updated value differs from the original.
Marking the register as volatile doesn't change the behavior, only the source of
the initial value _regmap_update_bits() uses. Step 3B is the problematic one
when using regmap_update_bits(). Per the diagram above, the comparison may
happen against an input value differing from the (invisible) output state, which
would hide the state change.
Best,
Sander
On Mon Oct 20, 2025 at 3:25 PM CEST, Sander Vanheule wrote: > Hi Michael, > > On Mon, 2025-10-20 at 15:02 +0200, Michael Walle wrote: >> Hi Sander, >> >> On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote: >> > GPIO chips often have data input and output fields aliased to the same >> > offset. Since gpio-regmap performs a value update before the direction >> > update (to prevent glitches), a pin currently configured as input may >> > cause regmap_update_bits() to not perform a write. >> > >> > This may cause unexpected line states when the current input state >> > equals the requested output state: >> > >> > OUT IN OUT >> > DIR ''''''\...|.../'''''' >> > >> > pin ....../'''|'''\...... >> > (1) (2) (3) >> > >> > 1. Line was configurad as out-low, but is reconfigured to input. >> > External logic results in high value. >> > 2. Set output value high. regmap_update_bits() sees the value is >> > already high and discards the register write. >> > 3. Line is switched to output, maintaining the stale output config >> > (low) instead of the requested config (high). >> > >> > By switching to regmap_write_bits(), a write of the requested output >> > value can be forced, irrespective of the read state. Do this only for >> > aliased registers, so the more efficient regmap_update_bits() can still >> > be used for distinct registers. >> >> Have you looked at the .volatile_reg callback of the regmap api? >> You might use the same heuristics, i.e. .reg_dat_base == .reg_set_base >> to implement that callback. That way you'd just have to >> (unconditionally) set that callback in gpio_regmap_register() and >> regmap should take care of the rest. > > Maybe I'm missing something here, but I'm not sure what difference that would > make. .volatile_reg is part of the regmap config, so when gpio_regmap_register() > is called, the regmap has already been created. We can't change the > .volatile_reg callback (and we shouldn't, it's up to the user to define it). Ha, yes ofc, you're right. It seems I really need some more sleep. > FWIW, I did test this with a regmap config that marks the aliased data registers > as volatile. The issue isn't that an invalid cache is being read. The problem is > that writes are being optimized away when they shouldn't: > > 1. Read register from hardware (volatile) or cache (non-volatile). > 2. Update bits in mask to requested value > 3. Write updated value to hardware if: > A. This is a forced write (i.e. regmap_write_bits), or > B. The updated value differs from the original. > > Marking the register as volatile doesn't change the behavior, only the source of > the initial value _regmap_update_bits() uses. Step 3B is the problematic one > when using regmap_update_bits(). Per the diagram above, the comparison may > happen against an input value differing from the (invisible) output state, which > would hide the state change. Ah, now I got it. Thanks for the explanation. Let me get back to your initial patch tomorrow. -michael
© 2016 - 2026 Red Hat, Inc.