| 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
| ||
|
Date: Wed, 22 Jun 2022 18:12:49 +0800
From: Dong Aisheng <dongas86@...il.com>
To: Aisheng Dong <aisheng.dong@....com>
Cc: Mark Brown <broonie@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"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
Hi Mark & Lucas,
[...]
> > > 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?
>
Below is a quick try fix which seems to work at my side by using cache_only
mode suggested by Mark. I set cache_only mode in the bus power notifier
rather than in blkctl power on/off callback in order to avoid possible cache
mode setting contention.
NOTE: i didn't fix _regmap_write() as i.MX controls regmap write well in driver
with power enabled first, so don't have issues in reality.
It can be fixed in a separate patch later if needed.
You may check if it's as your expected solution.
For syscon, I still have no idea how to fix it if I can't disable it.
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 2eaffd3224c9..da1702fd57cc 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
void regcache_cache_only(struct regmap *map, bool enable)
{
map->lock(map->lock_arg);
- WARN_ON(map->cache_bypass && enable);
+// WARN_ON(map->cache_bypass && enable);
map->cache_only = enable;
trace_regmap_cache_only(map, enable);
map->unlock(map->lock_arg);
diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 7ebc28709e94..12f0f9a24fad 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -199,6 +199,8 @@ static int imx8m_blk_ctrl_probe(struct
platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(bc->regmap),
"failed to init regmap\n");
+ regcache_cache_only(bc->regmap, true);
+
bc->domains = devm_kcalloc(dev, bc_data->num_domains,
sizeof(struct imx8m_blk_ctrl_domain),
GFP_KERNEL);
@@ -408,6 +410,8 @@ static int imx8mm_vpu_power_notifier(struct
notifier_block *nb,
*/
udelay(5);
+ regcache_cache_only(bc->regmap, false);
+
/* set "fuse" bits to enable the VPUs */
regmap_set_bits(bc->regmap, 0x8, 0xffffffff);
regmap_set_bits(bc->regmap, 0xc, 0xffffffff);
@@ -415,6 +419,9 @@ static int imx8mm_vpu_power_notifier(struct
notifier_block *nb,
regmap_set_bits(bc->regmap, 0x14, 0xffffffff);
}
+ if (action == GENPD_NOTIFY_PRE_OFF)
+ regcache_cache_only(bc->regmap, true);
+
return NOTIFY_OK;
}
@@ -461,6 +468,9 @@ static int imx8mm_disp_power_notifier(struct
notifier_block *nb,
if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON)
+ regcache_cache_only(bc->regmap, false);
+
/* Enable bus clock and deassert bus reset */
regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(12));
regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(6));
@@ -473,6 +483,8 @@ static int imx8mm_disp_power_notifier(struct
notifier_block *nb,
if (action == GENPD_NOTIFY_ON)
udelay(5);
+ if (action == GENPD_NOTIFY_PRE_OFF)
+ regcache_cache_only(bc->regmap, true);
return NOTIFY_OK;
}
@@ -531,6 +543,9 @@ static int imx8mn_disp_power_notifier(struct
notifier_block *nb,
if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON)
+ regcache_cache_only(bc->regmap, false);
+
/* Enable bus clock and deassert bus reset */
regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(8));
regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(8));
@@ -543,6 +558,8 @@ static int imx8mn_disp_power_notifier(struct
notifier_block *nb,
if (action == GENPD_NOTIFY_ON)
udelay(5);
+ if (action == GENPD_NOTIFY_PRE_OFF)
+ regcache_cache_only(bc->regmap, true);
return NOTIFY_OK;
}
@@ -716,6 +733,9 @@ static int imx8mq_vpu_power_notifier(struct
notifier_block *nb,
if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON)
+ regcache_cache_only(bc->regmap, false);
+
/*
* The ADB in the VPUMIX domain has no separate reset and clock
* enable bits, but is ungated and reset together with the VPUs. The
@@ -740,6 +760,9 @@ static int imx8mq_vpu_power_notifier(struct
notifier_block *nb,
regmap_set_bits(bc->regmap, 0x10, 0xffffffff);
}
+ if (action == GENPD_NOTIFY_PRE_OFF)
+ regcache_cache_only(bc->regmap, true);
+
return NOTIFY_OK;
}
Regards
Aisheng
Powered by blists - more mailing lists