Messages in this thread | | | From | Aisheng Dong <> | Subject | RE: [PATCH RFC 1/2] regmap: add option to disable debugfs | Date | Wed, 22 Jun 2022 08:18:52 +0000 |
| |
> 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 >
| |