Messages in this thread | | | Subject | Re: [PATCH RFC 1/2] regmap: add option to disable debugfs | From | Lucas Stach <> | Date | Wed, 22 Jun 2022 10:35:17 +0200 |
| |
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
| |