[RFC PATCH 0/2] gpio: regmap: Ensure writes for aliased data values

[RFC PATCH 0/2] gpio: regmap: Ensure writes for aliased data values

[Sander Vanheule]

These patches aim to fix an issue with aliased data registers, where the
input values are read from an offset and the output values are written
to the same offset. The current use of regmap_update_bits() may cause an
input value to be used to decide not to write a new output value.

The first patch in this RFC aims to fix the above issue. The second
patch is optional, but allows to recover some performance by leveraging
the regmap cache.

I am currently revisiting an older patch series of mine [1]. I am
submitting these as an RFC to see if they are acceptable, but would
include the patch(es) in the respin of the series.

[1] https://lore.kernel.org/linux-gpio/cover.1623532208.git.sander@svanheule.net/

Sander Vanheule (2):
  gpio: regmap: Force writes for aliased data regs
  gpio: regmap: Bypass cache for aliased outputs

 drivers/gpio/gpio-regmap.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.51.0

[RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Sander Vanheule]

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, &reg, &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.0

Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Sander Vanheule]

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

Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

Hi,

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, &reg, &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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

[Sander Vanheule]

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

[RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Sander Vanheule]

GPIO chips often have data input and output registers aliased to the
same offset. The output register is non-valitile and could in theory be
cached. The input register however is volatile by nature and hence
should not be cached, resulting in different requirements for reads and
writes.

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.

For gpio-regmap the output data could be in multiple registers, but we
can use the regmap cache support to shadow the output values by marking
the data registers as non-volatile. By using regmap_read_bypassed() we
can still treat the input values as volatile, irrespective of the regmap
config. This ensures proper functioning of writing the output register
with regmap_write_bits(), which will then use and update the cache only
on data writes, gaining some performance from the cached output values.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/gpio/gpio-regmap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index ba3c19206ccf..afecacf7607f 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -81,7 +81,11 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 	if (ret)
 		return ret;
 
-	ret = regmap_read(gpio->regmap, reg, &val);
+	/* ensure we don't spoil any register cache with pin input values */
+	if (gpio->reg_dat_base == gpio->reg_set_base)
+		ret = regmap_read_bypassed(gpio->regmap, reg, &val);
+	else
+		ret = regmap_read(gpio->regmap, reg, &val);
 	if (ret)
 		return ret;
 
-- 
2.51.0

Re: [RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Linus Walleij]

On Mon, Oct 20, 2025 at 1:56 PM Sander Vanheule <sander@svanheule.net> wrote:

> GPIO chips often have data input and output registers aliased to the
> same offset. The output register is non-valitile and could in theory be
> cached. The input register however is volatile by nature and hence
> should not be cached, resulting in different requirements for reads and
> writes.
>
> 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.
>
> For gpio-regmap the output data could be in multiple registers, but we
> can use the regmap cache support to shadow the output values by marking
> the data registers as non-volatile. By using regmap_read_bypassed() we
> can still treat the input values as volatile, irrespective of the regmap
> config. This ensures proper functioning of writing the output register
> with regmap_write_bits(), which will then use and update the cache only
> on data writes, gaining some performance from the cached output values.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

That looks good to me for sure!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Sander Vanheule]

Hi Linus, Michael,

On Tue, 2025-10-21 at 09:18 +0200, Linus Walleij wrote:
> On Mon, Oct 20, 2025 at 1:56 PM Sander Vanheule <sander@svanheule.net> wrote:
> 
> > GPIO chips often have data input and output registers aliased to the
> > same offset. The output register is non-valitile and could in theory be
> > cached. The input register however is volatile by nature and hence
> > should not be cached, resulting in different requirements for reads and
> > writes.
> > 
> > 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.
> > 
> > For gpio-regmap the output data could be in multiple registers, but we
> > can use the regmap cache support to shadow the output values by marking
> > the data registers as non-volatile. By using regmap_read_bypassed() we
> > can still treat the input values as volatile, irrespective of the regmap
> > config. This ensures proper functioning of writing the output register
> > with regmap_write_bits(), which will then use and update the cache only
> > on data writes, gaining some performance from the cached output values.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> 
> That looks good to me for sure!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for the reviews, I'll prepare the full respin for the RTL8231 patches and
send them later today or tomorrow.

Best,
Sander

Re: [RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Bartosz Golaszewski]

On Tue, Oct 21, 2025 at 11:01 AM Sander Vanheule <sander@svanheule.net> wrote:
>
> Thanks for the reviews, I'll prepare the full respin for the RTL8231 patches and
> send them later today or tomorrow.

I take it, you'll include these patches in that series?

Bart

Re: [RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Sander Vanheule]

Hi,

On Tue, 2025-10-21 at 14:21 +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 11:01 AM Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > Thanks for the reviews, I'll prepare the full respin for the RTL8231 patches
> > and
> > send them later today or tomorrow.
> 
> I take it, you'll include these patches in that series?

Yes, I will.

Since this was just an RFC, I don't know if everybody pays as much attention. I
will include Linus' and Michael's tags when I send the series.

Best,
Sander

Re: [RFC PATCH 2/2] gpio: regmap: Bypass cache for aliased outputs

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote:
> GPIO chips often have data input and output registers aliased to the
> same offset. The output register is non-valitile and could in theory be
> cached. The input register however is volatile by nature and hence
> should not be cached, resulting in different requirements for reads and
> writes.
>
> 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.
>
> For gpio-regmap the output data could be in multiple registers, but we
> can use the regmap cache support to shadow the output values by marking
> the data registers as non-volatile. By using regmap_read_bypassed() we
> can still treat the input values as volatile, irrespective of the regmap
> config. This ensures proper functioning of writing the output register
> with regmap_write_bits(), which will then use and update the cache only
> on data writes, gaining some performance from the cached output values.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

Reviewed-by: Michael Walle <mwalle@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]