[PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Jeffy Chen]

We are now allowing to register debugfs for syscon regmap, and not
having a valid name will end up using "dummy" to create debugfs dir.

Fixes: 9b947a13e7f6 ("regmap: use debugfs even when no device")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/mfd/syscon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 7eaa40bc703f..250d22f40c84 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	syscon_config.reg_stride = reg_io_width;
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
+	syscon_config.name = of_node_full_name(np);
 
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
-- 
2.11.0

[PATCH v2 2/3] regmap: debugfs: Fix kmemleak in regmap_debugfs_init

[Jeffy Chen]

Use map->debugfs_name to store allocated debugfs name, so it would be
freed in regmap_debugfs_exit().

Kmemleak reported:
unreferenced object 0xffffffc0cf78cf00 (size 128):
  comm "swapper/0", pid 1, jiffies 4294669168 (age 89.152s)
  hex dump (first 32 bytes):
    64 75 6d 6d 79 32 33 00 00 00 00 00 00 00 00 00  dummy23.........
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<0000000027429160>] kmemleak_alloc+0x58/0x8c
    [<0000000081861ddf>] __kmalloc_track_caller+0x1dc/0x2bc
    [<00000000258a341f>] kvasprintf+0xd0/0x168
    [<00000000f3243f27>] kasprintf+0xac/0xd8
    [<000000005f585642>] regmap_debugfs_init+0x1a0/0x3b0
    [<000000002c08b110>] __regmap_init+0x12b8/0x135c
    [<00000000f64bcddb>] __regmap_init_mmio_clk+0x70/0x84
    [<000000009c8f06b5>] of_syscon_register+0x278/0x378
    [<0000000087e6c121>] syscon_node_to_regmap+0x80/0xb0

Fixes: a430ab205d29 ("regmap: debugfs: Disambiguate dummy debugfs file name")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/base/regmap/regmap-debugfs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index e3e7b91cc421..5479a183248f 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -570,15 +570,15 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 		map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
 					      devname, name);
 		name = map->debugfs_name;
+	} else if (!map->dev) {
+		map->debugfs_name = kasprintf(GFP_KERNEL, "%s%d",
+					      devname, dummy_index);
+		name = map->debugfs_name;
+		dummy_index++;
 	} else {
 		name = devname;
 	}
 
-	if (!strcmp(name, "dummy")) {
-		name = kasprintf(GFP_KERNEL, "dummy%d", dummy_index);
-		dummy_index++;
-	}
-
 	map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
 	if (!map->debugfs) {
 		dev_warn(map->dev, "Failed to create debugfs directory\n");
-- 
2.11.0

Re: [PATCH v2 2/3] regmap: debugfs: Fix kmemleak in regmap_debugfs_init

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

On Tue, Mar 06, 2018 at 07:04:02PM +0800, Jeffy Chen wrote:
> Use map->debugfs_name to store allocated debugfs name, so it would be
> freed in regmap_debugfs_exit().

I'm missing patch 1 in this series and I think this collides with a fix
I already have locally.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] regmap: debugfs: Fix kmemleak in regmap_debugfs_init

[JeffyChen]

Hi Mark,

Thanks for your reply.

On 03/06/2018 08:59 PM, Mark Brown wrote:
> On Tue, Mar 06, 2018 at 07:04:02PM +0800, Jeffy Chen wrote:
>> Use map->debugfs_name to store allocated debugfs name, so it would be
>> freed in regmap_debugfs_exit().
>
> I'm missing patch 1 in this series and I think this collides with a fix
> I already have locally.
>
sorry for that, i should CC you the whole seires.

the patch 1 should be:
https://patchwork.kernel.org/patch/10261413

[PATCH v2 3/3] regmap: debugfs: Free map->debugfs_name when debugfs_create_dir() failed

[Jeffy Chen]

Free map->debugfs_name when debugfs_create_dir() failed to avoid memory
leak.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/base/regmap/regmap-debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 5479a183248f..55e862a81e82 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -582,6 +582,9 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 	map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
 	if (!map->debugfs) {
 		dev_warn(map->dev, "Failed to create debugfs directory\n");
+
+		kfree(map->debugfs_name);
+		map->debugfs_name = NULL;
 		return;
 	}
 
-- 
2.11.0

Applied "regmap: debugfs: Free map->debugfs_name when debugfs_create_dir() failed" to the regmap tree

[Mark Brown]

The patch

   regmap: debugfs: Free map->debugfs_name when debugfs_create_dir() failed

has been applied to the regmap tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 17cf46cfe975f1dd04db6bd38398923846512d49 Mon Sep 17 00:00:00 2001
From: Jeffy Chen <jeffy.chen@rock-chips.com>
Date: Tue, 6 Mar 2018 19:04:03 +0800
Subject: [PATCH] regmap: debugfs: Free map->debugfs_name when
 debugfs_create_dir() failed

Free map->debugfs_name when debugfs_create_dir() failed to avoid memory
leak.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index dd3a16894e3c..c84f5ceb015a 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -584,6 +584,9 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 	map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
 	if (!map->debugfs) {
 		dev_warn(map->dev, "Failed to create debugfs directory\n");
+
+		kfree(map->debugfs_name);
+		map->debugfs_name = NULL;
 		return;
 	}
 
-- 
2.16.2

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[JeffyChen]

+CC Mark.

even this is already fixed by a430ab205d29 ("regmap: debugfs: 
Disambiguate dummy debugfs file name")

but maybe we can still have this for a better debugfs name?

On 03/06/2018 07:04 PM, Jeffy Chen wrote:
> We are now allowing to register debugfs for syscon regmap, and not
> having a valid name will end up using "dummy" to create debugfs dir.
>
> Fixes: 9b947a13e7f6 ("regmap: use debugfs even when no device")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>   drivers/mfd/syscon.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 7eaa40bc703f..250d22f40c84 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
>   	syscon_config.reg_stride = reg_io_width;
>   	syscon_config.val_bits = reg_io_width * 8;
>   	syscon_config.max_register = resource_size(&res) - reg_io_width;
> +	syscon_config.name = of_node_full_name(np);
>
>   	regmap = regmap_init_mmio(NULL, base, &syscon_config);
>   	if (IS_ERR(regmap)) {
>

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Tue, Mar 06, 2018 at 10:58:26PM +0800, JeffyChen wrote:

> even this is already fixed by a430ab205d29 ("regmap: debugfs: Disambiguate
> dummy debugfs file name")

> but maybe we can still have this for a better debugfs name?

That's why the facility is there.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[JeffyChen]

Hi Mark,

On 03/07/2018 06:20 PM, Mark Brown wrote:
> On Tue, Mar 06, 2018 at 10:58:26PM +0800, JeffyChen wrote:
>
>> even this is already fixed by a430ab205d29 ("regmap: debugfs: Disambiguate
>> dummy debugfs file name")
>
>> but maybe we can still have this for a better debugfs name?
>
> That's why the facility is there.
>
> Please don't top post, reply in line with needed context.  This allows
> readers to readily follow the flow of conversation and understand what
> you are talking about and also helps ensure that everything in the
> discussion is being addressed.
>
ok, sorry for that :)

recently we are now allowing to register debugfs for syscon regmap, then 
i hit a lot of:
(NULL device *): Failed to create debugfs directory

so i sent this patch to fix that by using the of_node name to create 
debugfs.

but later i found we have:
commit a430ab205d29e7d1537b220fcf989b8080d8267f
Author: Fabio Estevam <fabio.estevam@nxp.com>
Date:   Mon Mar 5 15:52:09 2018 -0300

     regmap: debugfs: Disambiguate dummy debugfs file name

which would fix that in another way by naming dummy0, dummy1, dummy2.

even if the issue is fixed, i wonder could we still have this patch to 
make it more readable:

+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct 
device_node *np)
         syscon_config.reg_stride = reg_io_width;
         syscon_config.val_bits = reg_io_width * 8;
         syscon_config.max_register = resource_size(&res) - reg_io_width;
+       syscon_config.name = of_node_full_name(np);




and i've sent 2 more patches to fix the memleak there:
https://patchwork.kernel.org/patch/10261409
https://patchwork.kernel.org/patch/10261411

and sorry again, i forgot to add a cover letter, and i should CC you the 
whole series.

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On Wed, Mar 07, 2018 at 06:50:08PM +0800, JeffyChen wrote:

> so i sent this patch to fix that by using the of_node name to create
> debugfs.

> but later i found we have:
> commit a430ab205d29e7d1537b220fcf989b8080d8267f
> Author: Fabio Estevam <fabio.estevam@nxp.com>
> Date:   Mon Mar 5 15:52:09 2018 -0300

>     regmap: debugfs: Disambiguate dummy debugfs file name

> which would fix that in another way by naming dummy0, dummy1, dummy2.

> even if the issue is fixed, i wonder could we still have this patch to make
> it more readable:

Yes, like I say I think that's a good idea anyway - that sort of usage
is the reason we've got the facility to set an explicit name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Lee Jones]

On Wed, 07 Mar 2018, Mark Brown wrote:

> On Wed, Mar 07, 2018 at 06:50:08PM +0800, JeffyChen wrote:
> 
> > so i sent this patch to fix that by using the of_node name to create
> > debugfs.
> 
> > but later i found we have:
> > commit a430ab205d29e7d1537b220fcf989b8080d8267f
> > Author: Fabio Estevam <fabio.estevam@nxp.com>
> > Date:   Mon Mar 5 15:52:09 2018 -0300
> 
> >     regmap: debugfs: Disambiguate dummy debugfs file name
> 
> > which would fix that in another way by naming dummy0, dummy1, dummy2.
> 
> > even if the issue is fixed, i wonder could we still have this patch to make
> > it more readable:
> 
> Yes, like I say I think that's a good idea anyway - that sort of usage
> is the reason we've got the facility to set an explicit name.

Is that an Ack?

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Wed, Mar 07, 2018 at 04:54:05PM +0000, Lee Jones wrote:
> On Wed, 07 Mar 2018, Mark Brown wrote:
> > On Wed, Mar 07, 2018 at 06:50:08PM +0800, JeffyChen wrote:

> > Yes, like I say I think that's a good idea anyway - that sort of usage
> > is the reason we've got the facility to set an explicit name.

> Is that an Ack?

No, it looked like the change was only for MFD?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Lee Jones]

On Wed, 07 Mar 2018, Mark Brown wrote:

> On Wed, Mar 07, 2018 at 04:54:05PM +0000, Lee Jones wrote:
> > On Wed, 07 Mar 2018, Mark Brown wrote:
> > > On Wed, Mar 07, 2018 at 06:50:08PM +0800, JeffyChen wrote:
> 
> > > Yes, like I say I think that's a good idea anyway - that sort of usage
> > > is the reason we've got the facility to set an explicit name.
> 
> > Is that an Ack?
> 
> No, it looked like the change was only for MFD?

Right, but you can still Review/Ack patches from other subsystems.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Mon, Mar 12, 2018 at 02:07:42PM +0000, Lee Jones wrote:
> On Wed, 07 Mar 2018, Mark Brown wrote:
> > On Wed, Mar 07, 2018 at 04:54:05PM +0000, Lee Jones wrote:

> > > Is that an Ack?

> > No, it looked like the change was only for MFD?

> Right, but you can still Review/Ack patches from other subsystems.

If I want to ack something I'll do so explicitly; I've not looked to see
if what's being filled in here makes sense or not.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]