[PATCH RFC 0/2] regmap: option to disable debugfs

[PATCH RFC 0/2] regmap: option to disable debugfs

[Dong Aisheng]

Current regmap core will create debugfs by default for each instance.
However, it's fairly possible that some devices may not work properly
with regmap registers dump via debugfs due to it may be in suspend
state (e.g. Power domain is off).

This patchset allows user to claim no debugfs support due to platform
limitations.

Dong Aisheng (2):
  regmap: add option to disable debugfs
  soc: imx8m-blk-ctrl: do not export debugfs

 drivers/base/regmap/regmap-debugfs.c | 3 ++-
 drivers/base/regmap/regmap.c         | 3 +++
 drivers/soc/imx/imx8m-blk-ctrl.c     | 1 +
 include/linux/regmap.h               | 2 ++
 4 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

The regmap core will create debugfs by default for each instance.
However, it's fairly possible that some devices may not work properly
with regmap registers dump via debugfs due to it may be in suspend
state (e.g. Power domain is off).

Current regmap core does not support runtime pm for MMIO bus.
Although there have been several retries [1] in community but finally
didn't get accepted.

This patch adds an option to allow drivers to claim no debugfs support
due to possible platform limitations.

1. Link: https://lkml.iu.edu/hypermail/linux/kernel/1204.0/01646.html

Cc: Mark Brown <broonie@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/regmap/regmap-debugfs.c | 3 ++-
 drivers/base/regmap/regmap.c         | 3 +++
 include/linux/regmap.h               | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 817eda2075aa..82082a5f1729 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -556,9 +556,10 @@ void regmap_debugfs_init(struct regmap *map)
 	 * a mutex or a spinlock, but if the regmap owner decided to disable
 	 * all locking mechanisms, this is no longer the case. For safety:
 	 * don't create the debugfs entries if locking is disabled.
+	 * Or disabled explicitly in driver.
 	 */
 	if (map->debugfs_disable) {
-		dev_dbg(map->dev, "regmap locking disabled - not creating debugfs entries\n");
+		dev_dbg(map->dev, "regmap debugfs disabled - not creating debugfs entries\n");
 		return;
 	}
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index cb0be5e7b100..bb603b4271ef 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -812,6 +812,9 @@ struct regmap *__regmap_init(struct device *dev,
 		map->lock_arg = map;
 	}
 
+	if (config->disable_debugfs && !config->disable_locking)
+		regmap_debugfs_disable(map);
+
 	/*
 	 * When we write in fast-paths with regmap_bulk_write() don't allocate
 	 * scratch buffers with sleeping allocations.
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index d5b08f4f0dc0..db967a331d36 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -282,6 +282,7 @@ typedef void (*regmap_unlock)(void *);
  * @disable_locking: This regmap is either protected by external means or
  *                   is guaranteed not to be accessed from multiple threads.
  *                   Don't use any locking mechanisms.
+ * @disable_debugfs: Optional, don't create debugfs entries for this regmap.
  * @lock:	  Optional lock callback (overrides regmap's default lock
  *		  function, based on spinlock or mutex).
  * @unlock:	  As above for unlocking.
@@ -383,6 +384,7 @@ struct regmap_config {
 	bool (*readable_noinc_reg)(struct device *dev, unsigned int reg);
 
 	bool disable_locking;
+	bool disable_debugfs;
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg;
-- 
2.25.1

[PATCH RFC 2/2] soc: imx8m-blk-ctrl: do not export debugfs

[Dong Aisheng]

imx8m blkctl does not support registers dumping via regmap debugfs
due to power domain required which is not supported by current regmap
core. So let's claim no debugfs support, otherwise the system may
hang when dumping registers through regmap debugfs.

Cc: Mark Brown <broonie@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/imx8m-blk-ctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 7ebc28709e94..a064670576e8 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -179,6 +179,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
 		.reg_bits	= 32,
 		.val_bits	= 32,
 		.reg_stride	= 4,
+		.disable_debugfs = true,
 	};
 
 	bc = devm_kzalloc(dev, sizeof(*bc), GFP_KERNEL);
-- 
2.25.1

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Mon, Jun 20, 2022 at 09:47:57PM +0800, Dong Aisheng wrote:
> The regmap core will create debugfs by default for each instance.
> However, it's fairly possible that some devices may not work properly
> with regmap registers dump via debugfs due to it may be in suspend
> state (e.g. Power domain is off).
> 
> Current regmap core does not support runtime pm for MMIO bus.
> Although there have been several retries [1] in community but finally
> didn't get accepted.
> 
> This patch adds an option to allow drivers to claim no debugfs support
> due to possible platform limitations.
> 
> 1. Link: https://lkml.iu.edu/hypermail/linux/kernel/1204.0/01646.html

As indicated in the message you're linking to here if the device can't
be accessed it should be being put in cache only mode which will ensure
that nothing can do any physical accesses.

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

RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Aisheng Dong]

> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, June 20, 2022 11:06 PM
> 
> On Mon, Jun 20, 2022 at 09:47:57PM +0800, Dong Aisheng wrote:
> > The regmap core will create debugfs by default for each instance.
> > However, it's fairly possible that some devices may not work properly
> > with regmap registers dump via debugfs due to it may be in suspend
> > state (e.g. Power domain is off).
> >
> > Current regmap core does not support runtime pm for MMIO bus.
> > Although there have been several retries [1] in community but finally
> > didn't get accepted.
> >
> > This patch adds an option to allow drivers to claim no debugfs support
> > due to possible platform limitations.
> >
> > 1. Link: https://lkml.iu.edu/hypermail/linux/kernel/1204.0/01646.html
> 
> As indicated in the message you're linking to here if the device can't be
> accessed it should be being put in cache only mode which will ensure that
> nothing can do any physical accesses.

I wonder that's not a stable solution assuming there're possible volatile registers.
Isn't that?

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Mon, Jun 20, 2022 at 03:47:05PM +0000, Aisheng Dong wrote:

> > As indicated in the message you're linking to here if the device can't be
> > accessed it should be being put in cache only mode which will ensure that
> > nothing can do any physical accesses.

> I wonder that's not a stable solution assuming there're possible volatile registers.
> Isn't that?

The driver is going to need to power the device back up to access the
volatile registers so it can take the device out of cache only mode when
it's doing that can't it?

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

RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Aisheng Dong]

> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, June 20, 2022 11:50 PM
> 
> On Mon, Jun 20, 2022 at 03:47:05PM +0000, Aisheng Dong wrote:
> 
> > > As indicated in the message you're linking to here if the device
> > > can't be accessed it should be being put in cache only mode which
> > > will ensure that nothing can do any physical accesses.
> 
> > I wonder that's not a stable solution assuming there're possible volatile
> registers.
> > Isn't that?
> 
> The driver is going to need to power the device back up to access the volatile
> registers so it can take the device out of cache only mode when it's doing that
> can't it?

Sorry, I didn't quite get it.
There's no problem in driver to access volatile registers as it usually will power up
device first by rpm.
But for debugfs, from what I saw in code, if there's a volatile register, _regmap_read()
will bypass cache and try to read the register value from HW.
Then system may hang as no one powered up the device before.
Anything I missed?

static int _regmap_read(struct regmap *map, unsigned int reg,
                        unsigned int *val)
{
        int ret;
        void *context = _regmap_map_get_context(map);

        if (!map->cache_bypass) {
                ret = regcache_read(map, reg, val);
                if (ret == 0)
                        return 0;
        }

        ret = map->reg_read(context, reg, val);
        ...
}

Or you mean simply forgetting about volatile registers and let debugfs
to read the stale value from cache?

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Mon, Jun 20, 2022 at 04:15:40PM +0000, Aisheng Dong wrote:

> > The driver is going to need to power the device back up to access the volatile
> > registers so it can take the device out of cache only mode when it's doing that
> > can't it?

> Sorry, I didn't quite get it.
> There's no problem in driver to access volatile registers as it usually will power up
> device first by rpm.

So the runtime power managment seems like a good place to manage cache
only mode.

> But for debugfs, from what I saw in code, if there's a volatile register, _regmap_read()
> will bypass cache and try to read the register value from HW.
> Then system may hang as no one powered up the device before.
> Anything I missed?

> static int _regmap_read(struct regmap *map, unsigned int reg,
>                         unsigned int *val)
> {
>         int ret;
>         void *context = _regmap_map_get_context(map);
> 
>         if (!map->cache_bypass) {
>                 ret = regcache_read(map, reg, val);
>                 if (ret == 0)
>                         return 0;
>         }
> 
>         ret = map->reg_read(context, reg, val);

That's not what the code is upstream, upstream between the cache_bypass
check and the reg_read we have 

	if (map->cache_only)
		return -EBUSY;

	if (!regmap_readable(map, reg))
		return -EIO;

so if we can't satisfy the read from the cache then we'll hit the
cache_only check and return -EBUSY before we start trying to do any
physical I/O.  The debugfs code will handle that gracefully, indicating
that it couldn't get a value for the volatile register by showing all Xs
for the value.  If none of the registers are cached then the file won't
be terribly useful but it at least shouldn't cause any errors with
accessing the device when it's powered down.

> Or you mean simply forgetting about volatile registers and let debugfs
> to read the stale value from cache?

We shouldn't cache anything for volatile registers, if we are then
that's an issue.

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

RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Aisheng Dong]

> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, June 21, 2022 1:51 AM
> 
> On Mon, Jun 20, 2022 at 04:15:40PM +0000, Aisheng Dong wrote:
> 
> > > The driver is going to need to power the device back up to access
> > > the volatile registers so it can take the device out of cache only
> > > mode when it's doing that can't it?
> 
> > Sorry, I didn't quite get it.
> > There's no problem in driver to access volatile registers as it
> > usually will power up device first by rpm.
> 
> So the runtime power managment seems like a good place to manage cache
> only mode.
> 
> > But for debugfs, from what I saw in code, if there's a volatile
> > register, _regmap_read() will bypass cache and try to read the register value
> from HW.
> > Then system may hang as no one powered up the device before.
> > Anything I missed?
> 
> > static int _regmap_read(struct regmap *map, unsigned int reg,
> >                         unsigned int *val) {
> >         int ret;
> >         void *context = _regmap_map_get_context(map);
> >
> >         if (!map->cache_bypass) {
> >                 ret = regcache_read(map, reg, val);
> >                 if (ret == 0)
> >                         return 0;
> >         }
> >
> >         ret = map->reg_read(context, reg, val);
> 
> That's not what the code is upstream, upstream between the cache_bypass
> check and the reg_read we have
> 
> 	if (map->cache_only)
> 		return -EBUSY;
> 
> 	if (!regmap_readable(map, reg))
> 		return -EIO;
> 

Yes, I removed them during copy&paste for a more clean code before.

> so if we can't satisfy the read from the cache then we'll hit the cache_only
> check and return -EBUSY before we start trying to do any physical I/O.  The
> debugfs code will handle that gracefully, indicating that it couldn't get a value
> for the volatile register by showing all Xs for the value.  If none of the registers
> are cached then the file won't be terribly useful but it at least shouldn't cause
> any errors with accessing the device when it's powered down.
> 

That means we have to use cache_only mode to workaround the issue, right?
I saw that cache_only mode has to be explicated enable/disable by driver,
commonly used in device rpm in kernel right now.

However, things are a little bit complicated on i.MX that 1) imx blkctl
needs follow strict registers r/w flow interleaved with handshakes with upstream gpc
power operations and delay may be needed between registers writing
2) blkctl itself does not enable runtime pm, it simply call rpm to resume upstream power
domains devices and necessary clocks before r/w registers.

Furthermore, current imx blkctl is a common driver used by many devices [1].
Introducing volatile registers and cache may bloat the driver a lot unnecessarily.

The simplest way for i.MX case may be just disabling debugfs as it can't reflect the actually
HW state without power. So we would wish the regmap core could provide an option to users.

And I noticed that syscon [2] seems have the same issue since the following commit:

commit 9b947a13e7f6017f18470f665992dbae267852b3
Author: David Lechner <david@lechnology.com>
Date:   Mon Feb 19 15:43:02 2018 -0600

    regmap: use debugfs even when no device

    This registers regmaps with debugfs even when they do not have an
    associated device. For example, this is common for syscon regmaps.

    Signed-off-by: David Lechner <david@lechnology.com>
    Signed-off-by: Mark Brown <broonie@kernel.org>


1. drivers/soc/imx/imx8m-blk-ctrl.c
2. drivers/mfd/syscon.c

Regards
Aisheng

> > Or you mean simply forgetting about volatile registers and let debugfs
> > to read the stale value from cache?
> 
> We shouldn't cache anything for volatile registers, if we are then that's an
> issue.

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:

> > so if we can't satisfy the read from the cache then we'll hit the cache_only
> > check and return -EBUSY before we start trying to do any physical I/O.  The
> > debugfs code will handle that gracefully, indicating that it couldn't get a value
> > for the volatile register by showing all Xs for the value.  If none of the registers
> > are cached then the file won't be terribly useful but it at least shouldn't cause
> > any errors with accessing the device when it's powered down.

> That means we have to use cache_only mode to workaround the issue, right?
> I saw that cache_only mode has to be explicated enable/disable by driver,
> commonly used in device rpm in kernel right now.

Yes.

> However, things are a little bit complicated on i.MX that 1) imx blkctl
> needs follow strict registers r/w flow interleaved with handshakes with upstream gpc
> power operations and delay may be needed between registers writing
> 2) blkctl itself does not enable runtime pm, it simply call rpm to resume upstream power
> domains devices and necessary clocks before r/w registers.

I'm not sure I fully understand the issue here?  If the driver can
safely manage the hardware surely it can safely manage cache only mode
too?  If there are duplicate resumes then cache only enable/disable is a
boolean flag rather than refcounted so that shouldn't be a problem.

> Furthermore, current imx blkctl is a common driver used by many devices [1].
> Introducing volatile registers and cache may bloat the driver a lot unnecessarily.

You don't actually need to have a cache to use cache only mode, if there
are no cached registers then you'll just get -EBUSY on any access to the
registers but that's hopefully fine since at the minute things will just
fall over anyway.  You shouldn't even need to flag registers as volatile
if there's no cache since it's not really relevant without a cache.

> The simplest way for i.MX case may be just disabling debugfs as it can't reflect the actually
> HW state without power. So we would wish the regmap core could provide an option to users.

The goal here is to describe the regmap itself so that there's less
fragility as things change and we don't needlessly disable anything else
that happens to be added to debugfs in the future due to having an
overly generic flag, and we get the ability to directly catch access to
the hardware that misses doing power management properly while we're at
it.

We already have a way to describe it being unsafe to access the
hardware, we may as well use it.

> And I noticed that syscon [2] seems have the same issue since the following commit:

> commit 9b947a13e7f6017f18470f665992dbae267852b3
> Author: David Lechner <david@lechnology.com>
> Date:   Mon Feb 19 15:43:02 2018 -0600

>     regmap: use debugfs even when no device

>     This registers regmaps with debugfs even when they do not have an
>     associated device. For example, this is common for syscon regmaps.

That's a different thing, that's due to us naming the directory after
the struct device but syscons being created before we have that struct
device available.

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

RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Aisheng Dong]

> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, June 21, 2022 11:32 PM
> 
> On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:
> 
> > > so if we can't satisfy the read from the cache then we'll hit the
> > > cache_only check and return -EBUSY before we start trying to do any
> > > physical I/O.  The debugfs code will handle that gracefully,
> > > indicating that it couldn't get a value for the volatile register by
> > > showing all Xs for the value.  If none of the registers are cached
> > > then the file won't be terribly useful but it at least shouldn't cause any
> errors with accessing the device when it's powered down.
> 
> > That means we have to use cache_only mode to workaround the issue, right?
> > I saw that cache_only mode has to be explicated enable/disable by
> > driver, commonly used in device rpm in kernel right now.
> 
> Yes.
> 
> > However, things are a little bit complicated on i.MX that 1) imx
> > blkctl needs follow strict registers r/w flow interleaved with
> > handshakes with upstream gpc power operations and delay may be needed
> > between registers writing
> > 2) blkctl itself does not enable runtime pm, it simply call rpm to
> > resume upstream power domains devices and necessary clocks before r/w
> registers.
> 
> I'm not sure I fully understand the issue here?  If the driver can safely manage
> the hardware surely it can safely manage cache only mode too?  If there are
> duplicate resumes then cache only enable/disable is a boolean flag rather than
> refcounted so that shouldn't be a problem.
> 

I still can't see an easy and safe to way to do it.
What I'm wondering is whether it's worth to do it if need to introducing considerable
complexities in driver to overcome this issue if it can be simply disabled.
Anyway, I will try to investigate it.

> > Furthermore, current imx blkctl is a common driver used by many devices
> [1].
> > Introducing volatile registers and cache may bloat the driver a lot
> unnecessarily.
> 
> You don't actually need to have a cache to use cache only mode, if there are
> no cached registers then you'll just get -EBUSY on any access to the registers
> but that's hopefully fine since at the minute things will just fall over anyway.
> You shouldn't even need to flag registers as volatile if there's no cache since it's
> not really relevant without a cache.
> 

After a quick try initially, I found two issues:
1. There's a warning dump if using cache_only without cache
void regcache_cache_only(struct regmap *map, bool enable)
{
        map->lock(map->lock_arg);
        WARN_ON(map->cache_bypass && enable);
        ...
}
2. It seems _regmap_write() did not handle cache_only case well without cache.
It may still writes HW even for cache_only mode?

I guess we may need fix those two issues first before we can safely use it?

> > The simplest way for i.MX case may be just disabling debugfs as it
> > can't reflect the actually HW state without power. So we would wish the
> regmap core could provide an option to users.
> 
> The goal here is to describe the regmap itself so that there's less fragility as
> things change and we don't needlessly disable anything else that happens to
> be added to debugfs in the future due to having an overly generic flag, and we
> get the ability to directly catch access to the hardware that misses doing
> power management properly while we're at it.
> 
> We already have a way to describe it being unsafe to access the hardware, we
> may as well use it.
> 
> > And I noticed that syscon [2] seems have the same issue since the following
> commit:
> 
> > commit 9b947a13e7f6017f18470f665992dbae267852b3
> > Author: David Lechner <david@lechnology.com>
> > Date:   Mon Feb 19 15:43:02 2018 -0600
> 
> >     regmap: use debugfs even when no device
> 
> >     This registers regmaps with debugfs even when they do not have an
> >     associated device. For example, this is common for syscon regmaps.
> 
> That's a different thing, that's due to us naming the directory after the struct
> device but syscons being created before we have that struct device available.

Yes, but syscon has the same issue that the system may hang if dump registers
through debugfs without power on.
How would you suggest for this case as syscon is also a common driver?

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Lucas Stach]

Hi Aisheng, hi Mark,

Am Dienstag, dem 21.06.2022 um 18:16 +0000 schrieb Aisheng Dong:
> > From: Mark Brown <broonie@kernel.org>
> > Sent: Tuesday, June 21, 2022 11:32 PM
> > 
> > On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:
> > 
> > > > so if we can't satisfy the read from the cache then we'll hit
> > > > the
> > > > cache_only check and return -EBUSY before we start trying to do
> > > > any
> > > > physical I/O.  The debugfs code will handle that gracefully,
> > > > indicating that it couldn't get a value for the volatile
> > > > register by
> > > > showing all Xs for the value.  If none of the registers are
> > > > cached
> > > > then the file won't be terribly useful but it at least
> > > > shouldn't cause any
> > errors with accessing the device when it's powered down.
> > 
> > > That means we have to use cache_only mode to workaround the
> > > issue, right?
> > > I saw that cache_only mode has to be explicated enable/disable by
> > > driver, commonly used in device rpm in kernel right now.
> > 
> > Yes.
> > 
> > > However, things are a little bit complicated on i.MX that 1) imx
> > > blkctl needs follow strict registers r/w flow interleaved with
> > > handshakes with upstream gpc power operations and delay may be
> > > needed
> > > between registers writing
> > > 2) blkctl itself does not enable runtime pm, it simply call rpm
> > > to
> > > resume upstream power domains devices and necessary clocks before
> > > r/w
> > registers.
> > 
> > I'm not sure I fully understand the issue here?  If the driver can
> > safely manage
> > the hardware surely it can safely manage cache only mode too?  If
> > there are
> > duplicate resumes then cache only enable/disable is a boolean flag
> > rather than
> > refcounted so that shouldn't be a problem.
> > 
> 
> I still can't see an easy and safe to way to do it.
> What I'm wondering is whether it's worth to do it if need to
> introducing considerable
> complexities in driver to overcome this issue if it can be simply
> disabled.
> Anyway, I will try to investigate it.
> 
> > > Furthermore, current imx blkctl is a common driver used by many
> > > devices
> > [1].
> > > Introducing volatile registers and cache may bloat the driver a
> > > lot
> > unnecessarily.
> > 
> > You don't actually need to have a cache to use cache only mode, if
> > there are
> > no cached registers then you'll just get -EBUSY on any access to
> > the registers
> > but that's hopefully fine since at the minute things will just fall
> > over anyway.
> > You shouldn't even need to flag registers as volatile if there's no
> > cache since it's
> > not really relevant without a cache.
> > 
> 
> After a quick try initially, I found two issues:
> 1. There's a warning dump if using cache_only without cache
> void regcache_cache_only(struct regmap *map, bool enable)
> {
>         map->lock(map->lock_arg);
>         WARN_ON(map->cache_bypass && enable);
>         ...
> }
> 2. It seems _regmap_write() did not handle cache_only case well
> without cache.
> It may still writes HW even for cache_only mode?
> 
> I guess we may need fix those two issues first before we can safely
> use it?
> 
Why would you write to a cache only regmap. The debugfs interface only
allows to dump the registers, no?

I think it wouldn't be too hard to fix this for the blk-ctrl drivers.
I'll give it a try today.

> > > The simplest way for i.MX case may be just disabling debugfs as
> > > it
> > > can't reflect the actually HW state without power. So we would
> > > wish the
> > regmap core could provide an option to users.
> > 
> > The goal here is to describe the regmap itself so that there's less
> > fragility as
> > things change and we don't needlessly disable anything else that
> > happens to
> > be added to debugfs in the future due to having an overly generic
> > flag, and we
> > get the ability to directly catch access to the hardware that
> > misses doing
> > power management properly while we're at it.
> > 
> > We already have a way to describe it being unsafe to access the
> > hardware, we
> > may as well use it.
> > 
> > > And I noticed that syscon [2] seems have the same issue since the
> > > following
> > commit:
> > 
> > > commit 9b947a13e7f6017f18470f665992dbae267852b3
> > > Author: David Lechner <david@lechnology.com>
> > > Date:   Mon Feb 19 15:43:02 2018 -0600
> > 
> > >     regmap: use debugfs even when no device
> > 
> > >     This registers regmaps with debugfs even when they do not
> > > have an
> > >     associated device. For example, this is common for syscon
> > > regmaps.
> > 
> > That's a different thing, that's due to us naming the directory
> > after the struct
> > device but syscons being created before we have that struct device
> > available.
> 
> Yes, but syscon has the same issue that the system may hang if dump
> registers
> through debugfs without power on.
> How would you suggest for this case as syscon is also a common
> driver?
> 
This is a general issue. If something uses a syscon that is inside a
power-domain where the runtime PM is controlled by some other entity,
how is it safe to use the syscon at all? Every access might end up
locking up the system. So those syscons really need to learn some kind
of runtime PM handling.

Regards,
Lucas

RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Aisheng Dong]

> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, June 22, 2022 4:08 PM
> 
> Hi Aisheng, hi Mark,
> 
> Am Dienstag, dem 21.06.2022 um 18:16 +0000 schrieb Aisheng Dong:
> > > From: Mark Brown <broonie@kernel.org>
> > > Sent: Tuesday, June 21, 2022 11:32 PM
> > >
> > > On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:
> > >
> > > > > so if we can't satisfy the read from the cache then we'll hit
> > > > > the cache_only check and return -EBUSY before we start trying to
> > > > > do any physical I/O.  The debugfs code will handle that
> > > > > gracefully, indicating that it couldn't get a value for the
> > > > > volatile register by showing all Xs for the value.  If none of
> > > > > the registers are cached then the file won't be terribly useful
> > > > > but it at least shouldn't cause any
> > > errors with accessing the device when it's powered down.
> > >
> > > > That means we have to use cache_only mode to workaround the issue,
> > > > right?
> > > > I saw that cache_only mode has to be explicated enable/disable by
> > > > driver, commonly used in device rpm in kernel right now.
> > >
> > > Yes.
> > >
> > > > However, things are a little bit complicated on i.MX that 1) imx
> > > > blkctl needs follow strict registers r/w flow interleaved with
> > > > handshakes with upstream gpc power operations and delay may be
> > > > needed between registers writing
> > > > 2) blkctl itself does not enable runtime pm, it simply call rpm to
> > > > resume upstream power domains devices and necessary clocks before
> > > > r/w
> > > registers.
> > >
> > > I'm not sure I fully understand the issue here?  If the driver can
> > > safely manage the hardware surely it can safely manage cache only
> > > mode too?  If there are duplicate resumes then cache only
> > > enable/disable is a boolean flag rather than refcounted so that
> > > shouldn't be a problem.
> > >
> >
> > I still can't see an easy and safe to way to do it.
> > What I'm wondering is whether it's worth to do it if need to
> > introducing considerable complexities in driver to overcome this issue
> > if it can be simply disabled.
> > Anyway, I will try to investigate it.
> >
> > > > Furthermore, current imx blkctl is a common driver used by many
> > > > devices
> > > [1].
> > > > Introducing volatile registers and cache may bloat the driver a
> > > > lot
> > > unnecessarily.
> > >
> > > You don't actually need to have a cache to use cache only mode, if
> > > there are no cached registers then you'll just get -EBUSY on any
> > > access to the registers but that's hopefully fine since at the
> > > minute things will just fall over anyway.
> > > You shouldn't even need to flag registers as volatile if there's no
> > > cache since it's not really relevant without a cache.
> > >
> >
> > After a quick try initially, I found two issues:
> > 1. There's a warning dump if using cache_only without cache void
> > regcache_cache_only(struct regmap *map, bool enable) {
> >         map->lock(map->lock_arg);
> >         WARN_ON(map->cache_bypass && enable);
> >         ...
> > }
> > 2. It seems _regmap_write() did not handle cache_only case well
> > without cache.
> > It may still writes HW even for cache_only mode?
> >
> > I guess we may need fix those two issues first before we can safely
> > use it?
> >
> Why would you write to a cache only regmap. The debugfs interface only
> allows to dump the registers, no?

I mean the _regmap_write() called in driver even we claim it's cache only.
Not dumping registers from debugfs.

> 
> I think it wouldn't be too hard to fix this for the blk-ctrl drivers.
> I'll give it a try today.
> 

Great, looking forward to see it.

> > > > The simplest way for i.MX case may be just disabling debugfs as it
> > > > can't reflect the actually HW state without power. So we would
> > > > wish the
> > > regmap core could provide an option to users.
> > >
> > > The goal here is to describe the regmap itself so that there's less
> > > fragility as things change and we don't needlessly disable anything
> > > else that happens to be added to debugfs in the future due to having
> > > an overly generic flag, and we get the ability to directly catch
> > > access to the hardware that misses doing power management properly
> > > while we're at it.
> > >
> > > We already have a way to describe it being unsafe to access the
> > > hardware, we may as well use it.
> > >
> > > > And I noticed that syscon [2] seems have the same issue since the
> > > > following
> > > commit:
> > >
> > > > commit 9b947a13e7f6017f18470f665992dbae267852b3
> > > > Author: David Lechner <david@lechnology.com>
> > > > Date:   Mon Feb 19 15:43:02 2018 -0600
> > >
> > > >     regmap: use debugfs even when no device
> > >
> > > >     This registers regmaps with debugfs even when they do not have
> > > > an
> > > >     associated device. For example, this is common for syscon
> > > > regmaps.
> > >
> > > That's a different thing, that's due to us naming the directory
> > > after the struct device but syscons being created before we have
> > > that struct device available.
> >
> > Yes, but syscon has the same issue that the system may hang if dump
> > registers through debugfs without power on.
> > How would you suggest for this case as syscon is also a common driver?
> >
> This is a general issue. If something uses a syscon that is inside a
> power-domain where the runtime PM is controlled by some other entity, how
> is it safe to use the syscon at all? Every access might end up locking up the
> system. So those syscons really need to learn some kind of runtime PM
> handling.

The regmap core does not support runtime pm.
It may be unsafe to dumping registers through debugfs.

Regards
Aisheng

> 
> Regards,
> Lucas
> 

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Lucas Stach]

Am Mittwoch, dem 22.06.2022 um 08:18 +0000 schrieb Aisheng Dong:
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: Wednesday, June 22, 2022 4:08 PM
> > 
> > Hi Aisheng, hi Mark,
> > 
> > Am Dienstag, dem 21.06.2022 um 18:16 +0000 schrieb Aisheng Dong:
> > > > From: Mark Brown <broonie@kernel.org>
> > > > Sent: Tuesday, June 21, 2022 11:32 PM
> > > > 
> > > > On Tue, Jun 21, 2022 at 02:56:58PM +0000, Aisheng Dong wrote:
> > > > 
> > > > > > so if we can't satisfy the read from the cache then we'll
> > > > > > hit
> > > > > > the cache_only check and return -EBUSY before we start
> > > > > > trying to
> > > > > > do any physical I/O.  The debugfs code will handle that
> > > > > > gracefully, indicating that it couldn't get a value for the
> > > > > > volatile register by showing all Xs for the value.  If none
> > > > > > of
> > > > > > the registers are cached then the file won't be terribly
> > > > > > useful
> > > > > > but it at least shouldn't cause any
> > > > errors with accessing the device when it's powered down.
> > > > 
> > > > > That means we have to use cache_only mode to workaround the
> > > > > issue,
> > > > > right?
> > > > > I saw that cache_only mode has to be explicated
> > > > > enable/disable by
> > > > > driver, commonly used in device rpm in kernel right now.
> > > > 
> > > > Yes.
> > > > 
> > > > > However, things are a little bit complicated on i.MX that 1)
> > > > > imx
> > > > > blkctl needs follow strict registers r/w flow interleaved
> > > > > with
> > > > > handshakes with upstream gpc power operations and delay may
> > > > > be
> > > > > needed between registers writing
> > > > > 2) blkctl itself does not enable runtime pm, it simply call
> > > > > rpm to
> > > > > resume upstream power domains devices and necessary clocks
> > > > > before
> > > > > r/w
> > > > registers.
> > > > 
> > > > I'm not sure I fully understand the issue here?  If the driver
> > > > can
> > > > safely manage the hardware surely it can safely manage cache
> > > > only
> > > > mode too?  If there are duplicate resumes then cache only
> > > > enable/disable is a boolean flag rather than refcounted so that
> > > > shouldn't be a problem.
> > > > 
> > > 
> > > I still can't see an easy and safe to way to do it.
> > > What I'm wondering is whether it's worth to do it if need to
> > > introducing considerable complexities in driver to overcome this
> > > issue
> > > if it can be simply disabled.
> > > Anyway, I will try to investigate it.
> > > 
> > > > > Furthermore, current imx blkctl is a common driver used by
> > > > > many
> > > > > devices
> > > > [1].
> > > > > Introducing volatile registers and cache may bloat the driver
> > > > > a
> > > > > lot
> > > > unnecessarily.
> > > > 
> > > > You don't actually need to have a cache to use cache only mode,
> > > > if
> > > > there are no cached registers then you'll just get -EBUSY on
> > > > any
> > > > access to the registers but that's hopefully fine since at the
> > > > minute things will just fall over anyway.
> > > > You shouldn't even need to flag registers as volatile if
> > > > there's no
> > > > cache since it's not really relevant without a cache.
> > > > 
> > > 
> > > After a quick try initially, I found two issues:
> > > 1. There's a warning dump if using cache_only without cache void
> > > regcache_cache_only(struct regmap *map, bool enable) {
> > >         map->lock(map->lock_arg);
> > >         WARN_ON(map->cache_bypass && enable);
> > >         ...
> > > }
> > > 2. It seems _regmap_write() did not handle cache_only case well
> > > without cache.
> > > It may still writes HW even for cache_only mode?
> > > 
> > > I guess we may need fix those two issues first before we can
> > > safely
> > > use it?
> > > 
> > Why would you write to a cache only regmap. The debugfs interface
> > only
> > allows to dump the registers, no?
> 
> I mean the _regmap_write() called in driver even we claim it's cache
> only.
> Not dumping registers from debugfs.

That should obviously never happen, as the regmap should only be marked
cache-only while the power domain is collapsed. Writing any register in
this state is invalid usage and the WARN_ON is totally warranted.

> 
> > 
> > I think it wouldn't be too hard to fix this for the blk-ctrl
> > drivers.
> > I'll give it a try today.
> > 
> 
> Great, looking forward to see it.
> 
> > > > > The simplest way for i.MX case may be just disabling debugfs
> > > > > as it
> > > > > can't reflect the actually HW state without power. So we
> > > > > would
> > > > > wish the
> > > > regmap core could provide an option to users.
> > > > 
> > > > The goal here is to describe the regmap itself so that there's
> > > > less
> > > > fragility as things change and we don't needlessly disable
> > > > anything
> > > > else that happens to be added to debugfs in the future due to
> > > > having
> > > > an overly generic flag, and we get the ability to directly
> > > > catch
> > > > access to the hardware that misses doing power management
> > > > properly
> > > > while we're at it.
> > > > 
> > > > We already have a way to describe it being unsafe to access the
> > > > hardware, we may as well use it.
> > > > 
> > > > > And I noticed that syscon [2] seems have the same issue since
> > > > > the
> > > > > following
> > > > commit:
> > > > 
> > > > > commit 9b947a13e7f6017f18470f665992dbae267852b3
> > > > > Author: David Lechner <david@lechnology.com>
> > > > > Date:   Mon Feb 19 15:43:02 2018 -0600
> > > > 
> > > > >     regmap: use debugfs even when no device
> > > > 
> > > > >     This registers regmaps with debugfs even when they do not
> > > > > have
> > > > > an
> > > > >     associated device. For example, this is common for syscon
> > > > > regmaps.
> > > > 
> > > > That's a different thing, that's due to us naming the directory
> > > > after the struct device but syscons being created before we
> > > > have
> > > > that struct device available.
> > > 
> > > Yes, but syscon has the same issue that the system may hang if
> > > dump
> > > registers through debugfs without power on.
> > > How would you suggest for this case as syscon is also a common
> > > driver?
> > > 
> > This is a general issue. If something uses a syscon that is inside
> > a
> > power-domain where the runtime PM is controlled by some other
> > entity, how
> > is it safe to use the syscon at all? Every access might end up
> > locking up the
> > system. So those syscons really need to learn some kind of runtime
> > PM
> > handling.
> 
> The regmap core does not support runtime pm.
> It may be unsafe to dumping registers through debugfs.

Right, that is a general issue, not only for debugfs. Any access to a
syscon inside a power-domain may hang the system, as the syscon has no
way to ensure that the power domain is up.

Regards,
Lucas

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

Hi Mark & Lucas,

[...]

> > > Furthermore, current imx blkctl is a common driver used by many devices
> > [1].
> > > Introducing volatile registers and cache may bloat the driver a lot
> > unnecessarily.
> >
> > You don't actually need to have a cache to use cache only mode, if there are
> > no cached registers then you'll just get -EBUSY on any access to the registers
> > but that's hopefully fine since at the minute things will just fall over anyway.
> > You shouldn't even need to flag registers as volatile if there's no cache since it's
> > not really relevant without a cache.
> >
>
> After a quick try initially, I found two issues:
> 1. There's a warning dump if using cache_only without cache
> void regcache_cache_only(struct regmap *map, bool enable)
> {
>         map->lock(map->lock_arg);
>         WARN_ON(map->cache_bypass && enable);
>         ...
> }
> 2. It seems _regmap_write() did not handle cache_only case well without cache.
> It may still writes HW even for cache_only mode?
>
> I guess we may need fix those two issues first before we can safely use it?
>

Below is a quick try fix which seems to work at my side by using cache_only
mode suggested by Mark. I set cache_only mode in the bus power notifier
rather than in blkctl power on/off callback in order to avoid possible cache
mode setting contention.

NOTE:  i didn't fix _regmap_write() as i.MX controls regmap write well in driver
with power enabled first, so don't have issues in reality.
It can be fixed in a separate patch later if needed.
You may check if it's as your expected solution.

For syscon, I still have no idea how to fix it if I can't disable it.

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 2eaffd3224c9..da1702fd57cc 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
 void regcache_cache_only(struct regmap *map, bool enable)
 {
        map->lock(map->lock_arg);
-       WARN_ON(map->cache_bypass && enable);
+//     WARN_ON(map->cache_bypass && enable);
        map->cache_only = enable;
        trace_regmap_cache_only(map, enable);
        map->unlock(map->lock_arg);
diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 7ebc28709e94..12f0f9a24fad 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -199,6 +199,8 @@ static int imx8m_blk_ctrl_probe(struct
platform_device *pdev)
                return dev_err_probe(dev, PTR_ERR(bc->regmap),
                                     "failed to init regmap\n");

+       regcache_cache_only(bc->regmap, true);
+
        bc->domains = devm_kcalloc(dev, bc_data->num_domains,
                                   sizeof(struct imx8m_blk_ctrl_domain),
                                   GFP_KERNEL);
@@ -408,6 +410,8 @@ static int imx8mm_vpu_power_notifier(struct
notifier_block *nb,
                 */
                udelay(5);

+               regcache_cache_only(bc->regmap, false);
+
                /* set "fuse" bits to enable the VPUs */
                regmap_set_bits(bc->regmap, 0x8, 0xffffffff);
                regmap_set_bits(bc->regmap, 0xc, 0xffffffff);
@@ -415,6 +419,9 @@ static int imx8mm_vpu_power_notifier(struct
notifier_block *nb,
                regmap_set_bits(bc->regmap, 0x14, 0xffffffff);
        }

+       if (action == GENPD_NOTIFY_PRE_OFF)
+               regcache_cache_only(bc->regmap, true);
+
        return NOTIFY_OK;
 }

@@ -461,6 +468,9 @@ static int imx8mm_disp_power_notifier(struct
notifier_block *nb,
        if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
                return NOTIFY_OK;

+       if (action == GENPD_NOTIFY_ON)
+               regcache_cache_only(bc->regmap, false);
+
        /* Enable bus clock and deassert bus reset */
        regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(12));
        regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(6));
@@ -473,6 +483,8 @@ static int imx8mm_disp_power_notifier(struct
notifier_block *nb,
        if (action == GENPD_NOTIFY_ON)
                udelay(5);

+       if (action == GENPD_NOTIFY_PRE_OFF)
+               regcache_cache_only(bc->regmap, true);

        return NOTIFY_OK;
 }
@@ -531,6 +543,9 @@ static int imx8mn_disp_power_notifier(struct
notifier_block *nb,
        if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
                return NOTIFY_OK;

+       if (action == GENPD_NOTIFY_ON)
+               regcache_cache_only(bc->regmap, false);
+
        /* Enable bus clock and deassert bus reset */
        regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(8));
        regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(8));
@@ -543,6 +558,8 @@ static int imx8mn_disp_power_notifier(struct
notifier_block *nb,
        if (action == GENPD_NOTIFY_ON)
                udelay(5);

+       if (action == GENPD_NOTIFY_PRE_OFF)
+               regcache_cache_only(bc->regmap, true);

        return NOTIFY_OK;
 }
@@ -716,6 +733,9 @@ static int imx8mq_vpu_power_notifier(struct
notifier_block *nb,
        if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
                return NOTIFY_OK;

+       if (action == GENPD_NOTIFY_ON)
+               regcache_cache_only(bc->regmap, false);
+
        /*
         * The ADB in the VPUMIX domain has no separate reset and clock
         * enable bits, but is ungated and reset together with the VPUs. The
@@ -740,6 +760,9 @@ static int imx8mq_vpu_power_notifier(struct
notifier_block *nb,
                regmap_set_bits(bc->regmap, 0x10, 0xffffffff);
        }

+       if (action == GENPD_NOTIFY_PRE_OFF)
+               regcache_cache_only(bc->regmap, true);
+
        return NOTIFY_OK;
 }

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Wed, Jun 22, 2022 at 10:08:10AM +0200, Lucas Stach wrote:
> Am Dienstag, dem 21.06.2022 um 18:16 +0000 schrieb Aisheng Dong:

> > 1. There's a warning dump if using cache_only without cache
> > void regcache_cache_only(struct regmap *map, bool enable)
> > {
> >         map->lock(map->lock_arg);
> >         WARN_ON(map->cache_bypass && enable);
> >         ...
> > }
> > 2. It seems _regmap_write() did not handle cache_only case well
> > without cache.
> > It may still writes HW even for cache_only mode?
> > 
> > I guess we may need fix those two issues first before we can safely
> > use it?

> Why would you write to a cache only regmap. The debugfs interface only
> allows to dump the registers, no?

One of the use cases is where you've got settings that can be changed
while the device is idle and just map those directly onto the registers,
syncing the cache to the device when it gets powered up for actual use.
This can only be done when there's a cache, and won't apply to a lot of
devices.

There is debugfs code to do writes but you have to modify the kernel to
enable it, there's no config option for it upstream.

> I think it wouldn't be too hard to fix this for the blk-ctrl drivers.
> I'll give it a try today.

The other thing is that even with the bodge to just turn debugfs
presumably any case where the driver would try to write to a powered off
device is still a bug that needs fixing anyway - having a regmap in
cache only mode will translate it into a warning rather than a write
that goes AWOL or otherwise fails.

> > > That's a different thing, that's due to us naming the directory
> > > after the struct
> > > device but syscons being created before we have that struct device
> > > available.

> > Yes, but syscon has the same issue that the system may hang if dump
> > registers
> > through debugfs without power on.
> > How would you suggest for this case as syscon is also a common
> > driver?

> This is a general issue. If something uses a syscon that is inside a
> power-domain where the runtime PM is controlled by some other entity,
> how is it safe to use the syscon at all? Every access might end up
> locking up the system. So those syscons really need to learn some kind
> of runtime PM handling.

Right.  If you can interact with the device safely there's no need to
put it into cache only mode and you don't need to worry about managing
things (this should be the normal case for system controllers).  If you
can't interact with the device without powering it up then you still
have to worry about doing that regardless of what regmap is doing, if
anything I'd guess the warnings from regmap might be easier to debug
than the hardware misbehaviour.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Wed, Jun 22, 2022 at 06:12:49PM +0800, Dong Aisheng wrote:

> NOTE:  i didn't fix _regmap_write() as i.MX controls regmap write well in driver
> with power enabled first, so don't have issues in reality.

I can't tell what you think the problem is with _regmap_write()?

> It can be fixed in a separate patch later if needed.
> You may check if it's as your expected solution.

> For syscon, I still have no idea how to fix it if I can't disable it.
> 
> diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> index 2eaffd3224c9..da1702fd57cc 100644
> --- a/drivers/base/regmap/regcache.c
> +++ b/drivers/base/regmap/regcache.c
> @@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
>  void regcache_cache_only(struct regmap *map, bool enable)
>  {
>         map->lock(map->lock_arg);
> -       WARN_ON(map->cache_bypass && enable);
> +//     WARN_ON(map->cache_bypass && enable);
>         map->cache_only = enable;
>         trace_regmap_cache_only(map, enable);
>         map->unlock(map->lock_arg);

What is the purpose of this change?  Why would the combination of cache
only and bypass modes work be a good idea, and how should things behave
in that case?

> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 7ebc28709e94..12f0f9a24fad 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c

The changes in here look reasonable, though I'm not familiar with this
driver so might be missing something.

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

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

On Wed, Jun 22, 2022 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 22, 2022 at 06:12:49PM +0800, Dong Aisheng wrote:
>
> > NOTE:  i didn't fix _regmap_write() as i.MX controls regmap write well in driver
> > with power enabled first, so don't have issues in reality.
>
> I can't tell what you think the problem is with _regmap_write()?
>

Because from what I see,  _regmap_write() seems still can write to HW
register even with cache_only mode set theoretically.

> > It can be fixed in a separate patch later if needed.
> > You may check if it's as your expected solution.
>
> > For syscon, I still have no idea how to fix it if I can't disable it.
> >
> > diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> > index 2eaffd3224c9..da1702fd57cc 100644
> > --- a/drivers/base/regmap/regcache.c
> > +++ b/drivers/base/regmap/regcache.c
> > @@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
> >  void regcache_cache_only(struct regmap *map, bool enable)
> >  {
> >         map->lock(map->lock_arg);
> > -       WARN_ON(map->cache_bypass && enable);
> > +//     WARN_ON(map->cache_bypass && enable);
> >         map->cache_only = enable;
> >         trace_regmap_cache_only(map, enable);
> >         map->unlock(map->lock_arg);
>
> What is the purpose of this change?  Why would the combination of cache
> only and bypass modes work be a good idea, and how should things behave
> in that case?
>

Because without this change, there will be a kernel dump caused by
WARN_ON when drivers call regcache_cache_only(map, true) after power
is off. There's no cache used in the imx blkctl driver. So map->cache_bypass
is default to true.

I did this by following your previous suggestion as follows:
"You don't actually need to have a cache to use cache only mode, if there
are no cached registers then you'll just get -EBUSY on any access to the
registers but that's hopefully fine since at the minute things will just
fall over anyway.  You shouldn't even need to flag registers as volatile
if there's no cache since it's not really relevant without a cache."

Am I misunderstanding your suggestions?

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Thu, Jun 23, 2022 at 12:05:46AM +0800, Dong Aisheng wrote:
> On Wed, Jun 22, 2022 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:

> > On Wed, Jun 22, 2022 at 06:12:49PM +0800, Dong Aisheng wrote:
> >
> > > NOTE:  i didn't fix _regmap_write() as i.MX controls regmap write well in driver
> > > with power enabled first, so don't have issues in reality.

> > I can't tell what you think the problem is with _regmap_write()?

> Because from what I see,  _regmap_write() seems still can write to HW
> register even with cache_only mode set theoretically.

Ah, I see - we don't enforce cache_only if bypass is enabled somehow,
but we will complain if you try to enable both at the same time so I'm
not sure that's an issue?

> > > -       WARN_ON(map->cache_bypass && enable);
> > > +//     WARN_ON(map->cache_bypass && enable);

> > What is the purpose of this change?  Why would the combination of cache
> > only and bypass modes work be a good idea, and how should things behave
> > in that case?

> Because without this change, there will be a kernel dump caused by
> WARN_ON when drivers call regcache_cache_only(map, true) after power
> is off. There's no cache used in the imx blkctl driver. So map->cache_bypass
> is default to true.

cache_bypass is only going to be true if something enabled bypass, why
would a device that doesn't use a cache enable bypass?  It does get
turned on transiently by things like patching but those only make sense
if the device can be accessed so caceh_only shouldn't be on then.

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

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

On Thu, Jun 23, 2022 at 12:27 AM Mark Brown <broonie@kernel.org> wrote:

> > > > -       WARN_ON(map->cache_bypass && enable);
> > > > +//     WARN_ON(map->cache_bypass && enable);
>
> > > What is the purpose of this change?  Why would the combination of cache
> > > only and bypass modes work be a good idea, and how should things behave
> > > in that case?
>
> > Because without this change, there will be a kernel dump caused by
> > WARN_ON when drivers call regcache_cache_only(map, true) after power
> > is off. There's no cache used in the imx blkctl driver. So map->cache_bypass
> > is default to true.
>
> cache_bypass is only going to be true if something enabled bypass, why
> would a device that doesn't use a cache enable bypass?  It does get
> turned on transiently by things like patching but those only make sense
> if the device can be accessed so caceh_only shouldn't be on then.

It was enabled by default according to the code:
__regmap_init -> regcache_init

int regcache_init(struct regmap *map, const struct regmap_config *config)
{
        int ret;
        int i;
        void *tmp_buf;

        if (map->cache_type == REGCACHE_NONE) {
                if (config->reg_defaults || config->num_reg_defaults_raw)
                        dev_warn(map->dev,
                                 "No cache used with register defaults set!\n");

                map->cache_bypass = true;
                return 0;
        }
}

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Thu, Jun 23, 2022 at 12:42:51AM +0800, Dong Aisheng wrote:
> On Thu, Jun 23, 2022 at 12:27 AM Mark Brown <broonie@kernel.org> wrote:

> > cache_bypass is only going to be true if something enabled bypass, why
> > would a device that doesn't use a cache enable bypass?  It does get
> > turned on transiently by things like patching but those only make sense
> > if the device can be accessed so caceh_only shouldn't be on then.

> It was enabled by default according to the code:
> __regmap_init -> regcache_init

Ah, right.  That makes sense - we should relax the check to only apply
if there is actually a cache.

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

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

On Thu, Jun 23, 2022 at 12:48 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jun 23, 2022 at 12:42:51AM +0800, Dong Aisheng wrote:
> > On Thu, Jun 23, 2022 at 12:27 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > cache_bypass is only going to be true if something enabled bypass, why
> > > would a device that doesn't use a cache enable bypass?  It does get
> > > turned on transiently by things like patching but those only make sense
> > > if the device can be accessed so caceh_only shouldn't be on then.
>
> > It was enabled by default according to the code:
> > __regmap_init -> regcache_init
>
> Ah, right.  That makes sense - we should relax the check to only apply
> if there is actually a cache.

If we don't remove the WARN_ON in regcache_cache_only(), how
would you suggest the fix?
Otherwise we can't call regcache_cache_only() for imx blkctl which does not
have a cache.

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 2eaffd3224c9..da1702fd57cc 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
 void regcache_cache_only(struct regmap *map, bool enable)
 {
        map->lock(map->lock_arg);
-       WARN_ON(map->cache_bypass && enable);
+//     WARN_ON(map->cache_bypass && enable);
        map->cache_only = enable;
        trace_regmap_cache_only(map, enable);
        map->unlock(map->lock_arg);

Regards
Aisheng

Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Mark Brown]

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

On Thu, Jun 23, 2022 at 01:01:12AM +0800, Dong Aisheng wrote:
> On Thu, Jun 23, 2022 at 12:48 AM Mark Brown <broonie@kernel.org> wrote:

> > Ah, right.  That makes sense - we should relax the check to only apply
> > if there is actually a cache.

> If we don't remove the WARN_ON in regcache_cache_only(), how
> would you suggest the fix?

In exactly the manner I suggested above.

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