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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB9PR04MB847785E1861525FC1E4AD97280B39@DB9PR04MB8477.eurprd04.prod.outlook.com>
Date:   Tue, 21 Jun 2022 14:56:58 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Mark Brown <broonie@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dongas86@...il.com" <dongas86@...il.com>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        Peng Fan <peng.fan@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>
Subject: RE: [PATCH RFC 1/2] regmap: add option to disable debugfs

> From: Mark Brown <broonie@...nel.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@...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.

    Signed-off-by: David Lechner <david@...hnology.com>
    Signed-off-by: Mark Brown <broonie@...nel.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.