[v2] regmap: Add debugfs file for forcing field writes

  `_regmap_update_bits()` checks if the current register value differs
from the new value, and only writes to the register if they differ. When
testing hardware drivers, it might be desirable to always force a
register write, for example when writing to a `regmap_field`. This
enables and simplifies testing and verification of the hardware
interaction. For example, when using a hardware mock/simulation model,
one can then more easily verify that the driver makes the correct
expected register writes during certain events.

Add a bool variable `force_write_field` and a corresponding debugfs
entry to enable this. Since this feature could interfere with driver
operation, guard it with a macro.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
Changes in v2:
* Add macro to guard the debugfs entry.
* Fix `Signed-off-by` in commit message to match actual email address.
* Link to v1: https://lore.kernel.org/all/pndttvcu3ut.fsf@axis.com/

 drivers/base/regmap/internal.h       |  3 +++
 drivers/base/regmap/regmap-debugfs.c | 11 +++++++++++
 drivers/base/regmap/regmap.c         |  2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)


base-commit: 858fd168a95c5b9669aac8db6c14a9aeab446375
  

On Tue, Jun 13, 2023 at 01:42:27PM +0200, Waqar Hameed wrote:
> `_regmap_update_bits()` checks if the current register value differs
> from the new value, and only writes to the register if they differ. When
> testing hardware drivers, it might be desirable to always force a
> register write, for example when writing to a `regmap_field`. This
> enables and simplifies testing and verification of the hardware
> interaction. For example, when using a hardware mock/simulation model,
> one can then more easily verify that the driver makes the correct
> expected register writes during certain events.
> 
> Add a bool variable `force_write_field` and a corresponding debugfs
> entry to enable this. Since this feature could interfere with driver
> operation, guard it with a macro.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---
> Changes in v2:
> * Add macro to guard the debugfs entry.
> * Fix `Signed-off-by` in commit message to match actual email address.
> * Link to v1: https://lore.kernel.org/all/pndttvcu3ut.fsf@axis.com/
> 
>  drivers/base/regmap/internal.h       |  3 +++
>  drivers/base/regmap/regmap-debugfs.c | 11 +++++++++++
>  drivers/base/regmap/regmap.c         |  2 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 9bd0dfd1e259..6472b3222b82 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -125,6 +125,9 @@ struct regmap {
>  	int reg_stride;
>  	int reg_stride_order;
>  
> +	/* If set, will always write field to HW. */
> +	bool force_write_field;
> +
>  	/* regcache specific members */
>  	const struct regcache_ops *cache_ops;
>  	enum regcache_type cache_type;
> diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
> index c491fabe3617..f36027591e1a 100644
> --- a/drivers/base/regmap/regmap-debugfs.c
> +++ b/drivers/base/regmap/regmap-debugfs.c
> @@ -636,6 +636,17 @@ void regmap_debugfs_init(struct regmap *map)
>  				    &regmap_cache_bypass_fops);
>  	}
>  
> +	/*
> +	 * This could interfere with driver operation. Therefore, don't provide
> +	 * any real compile time configuration option for this feature. One will
> +	 * have to modify the source code directly in order to use it.
> +	 */
> +#undef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> +#ifdef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> +	debugfs_create_bool("force_write_field", 0600, map->debugfs,
> +			    &map->force_write_field);
> +#endif

Please no, that means this will never ever ever get used, and if it
happens to break the build or runtime, no one will ever notice it.

If you need this for your device/tree/distro, great, just keep it as an
out-of-tree patch with the huge header "NEVER ENABLE THIS IN A REAL
SYSTEM" or something like that.

But as-is, we can't take this, sorry.

greg k-h
  

On Tue, 13 Jun 2023, Greg Kroah-Hartman wrote:

> On Tue, Jun 13, 2023 at 01:42:27PM +0200, Waqar Hameed wrote:
> > `_regmap_update_bits()` checks if the current register value differs
> > from the new value, and only writes to the register if they differ. When
> >
> > +	/*
> > +	 * This could interfere with driver operation. Therefore, don't provide
> > +	 * any real compile time configuration option for this feature. One will
> > +	 * have to modify the source code directly in order to use it.
> > +	 */
> > +#undef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> > +#ifdef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> > +	debugfs_create_bool("force_write_field", 0600, map->debugfs,
> > +			    &map->force_write_field);
> > +#endif
> 
> Please no, that means this will never ever ever get used, and if it
> happens to break the build or runtime, no one will ever notice it.
> 
> If you need this for your device/tree/distro, great, just keep it as an
> out-of-tree patch with the huge header "NEVER ENABLE THIS IN A REAL
> SYSTEM" or something like that.

The ordinary only-write-if-bits-have-changed behavior of 
_regmap_update_bits would seem to be mostly an optimization to minimize 
the number of writes to a device. The way it is normally used in the code, 
it's not easy to predict when it in fact will result in a write, because 
the whole idea of a function like this is that a driver doesn't have to 
keep track of which bits are actually set or not, or else the function 
would not be useful in the first place.

I can understand that enabling a write-always behavior results in a 
different behavior on the associated bus (e.g. I2C), but in the end it 
shouldn't affect the functionality of the peripheral device any more than 
rearranging driver code to perform regmap writes in a different order, 
which might also lead to fewer or more bus writes.

It seems I'm clearly in the wrong here, so there must be some scenario 
I've missed. It just doesn't seem that dangerous; it's a debug 
functionality after all.

/Ricard
  

On Tue, Jun 13, 2023 at 05:02:27PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 13, 2023 at 01:42:27PM +0200, Waqar Hameed wrote:

> > +#undef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> > +#ifdef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> > +	debugfs_create_bool("force_write_field", 0600, map->debugfs,
> > +			    &map->force_write_field);
> > +#endif

> Please no, that means this will never ever ever get used, and if it
> happens to break the build or runtime, no one will ever notice it.

> If you need this for your device/tree/distro, great, just keep it as an
> out-of-tree patch with the huge header "NEVER ENABLE THIS IN A REAL
> SYSTEM" or something like that.

We have an existing setting that parallels this which enables writes via
debugfs - there *is* demand for this sort of thing in development where
this is a viable configuration mechanism, having something in tree in a
way that requires an out of tree patch like you're suggesting to enable
it documents why the feature isn't something you can enable without code
changes whereas just not having anything means people send patches
adding the feature.

This one is very much safer than the general write thing, we probably
*could* have a Kconfig option but changing the guarantees underneath
drivers feels like it's going to explode if we just let it happen.
  

On Tue, Jun 13, 2023 at 05:15:10PM +0200, Ricard Wanderlof wrote:

> I can understand that enabling a write-always behavior results in a 
> different behavior on the associated bus (e.g. I2C), but in the end it 
> shouldn't affect the functionality of the peripheral device any more than 
> rearranging driver code to perform regmap writes in a different order, 
> which might also lead to fewer or more bus writes.

> It seems I'm clearly in the wrong here, so there must be some scenario 
> I've missed. It just doesn't seem that dangerous; it's a debug 
> functionality after all.

There are devices where the act of writing to a register can have side
effects even if the values in the register are the same (for example
triggering some kind of recalibration based on the "new" value).  It's
rare, and it's probably more likely to be annoying or disruptive than
actually dangerous, but it is something drivers might be relying on.
  

On Tue, 13 Jun 2023 13:42:27 +0200, Waqar Hameed wrote:
> `_regmap_update_bits()` checks if the current register value differs
> from the new value, and only writes to the register if they differ. When
> testing hardware drivers, it might be desirable to always force a
> register write, for example when writing to a `regmap_field`. This
> enables and simplifies testing and verification of the hardware
> interaction. For example, when using a hardware mock/simulation model,
> one can then more easily verify that the driver makes the correct
> expected register writes during certain events.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regmap: Add debugfs file for forcing field writes
      commit: b629c698eae745eb51b6d92395e2eee44bbf5f49

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark