Message-ID: <a059f69470e39cf8928e53773ace6a97bb600821.camel@pengutronix.de>
Date:   Wed, 22 Jun 2022 10:35:17 +0200
From:   Lucas Stach <l.stach@...gutronix.de>
To:     Aisheng Dong <aisheng.dong@....com>,
        Mark Brown <broonie@...nel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dongas86@...il.com" <dongas86@...il.com>,
        Peng Fan <peng.fan@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>
Subject: Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

Am Mittwoch, dem 22.06.2022 um 08:18 +0000 schrieb Aisheng Dong:
> > From: Lucas Stach <l.stach@...gutronix.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@...nel.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@...hnology.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

Powered by blists - more mailing lists