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

[Aisheng Dong]

> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxx>
> > > > 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
>