[02/28] ASoC: soc-core: set component->debugfs_root NULL
diff mbox series

Message ID 87y3072iib.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series
  • ASoC: cleanup patches for soc-core
Related show

Commit Message

Kuninori Morimoto Aug. 6, 2019, 1:28 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

To be more safety code, let's set NULL to component->debugfs_root
when it was cleanuped.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-core.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Pierre-Louis Bossart Aug. 6, 2019, 2:49 p.m. UTC | #1
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 40bac40..00887f7 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -171,7 +171,10 @@ static void soc_init_component_debugfs(struct snd_soc_component *component)
>   
>   static void soc_cleanup_component_debugfs(struct snd_soc_component *component)
>   {
> +	if (!component->debugfs_root)
> +		return;

that test is redundant, it's safe to call debugfs_remove_recursive() 
without checking the argument (done internally).

>   	debugfs_remove_recursive(component->debugfs_root);
> +	component->debugfs_root = NULL;
>   }
Mark Brown Aug. 6, 2019, 4:22 p.m. UTC | #2
On Tue, Aug 06, 2019 at 09:49:20AM -0500, Pierre-Louis Bossart wrote:
> >   {
> > +	if (!component->debugfs_root)
> > +		return;

> that test is redundant, it's safe to call debugfs_remove_recursive() without
> checking the argument (done internally).

It's not completely redundant...

> >   	debugfs_remove_recursive(component->debugfs_root);
> > +	component->debugfs_root = NULL;
> >   }

...with this, the two in combination add protection against a double
free.  Not 100% sure it's worth it but I queued the patch since I
couldn't strongly convince myself it's a bad idea.
Pierre-Louis Bossart Aug. 6, 2019, 4:46 p.m. UTC | #3
On 8/6/19 11:22 AM, Mark Brown wrote:
> On Tue, Aug 06, 2019 at 09:49:20AM -0500, Pierre-Louis Bossart wrote:
>>>    {
>>> +	if (!component->debugfs_root)
>>> +		return;
> 
>> that test is redundant, it's safe to call debugfs_remove_recursive() without
>> checking the argument (done internally).
> 
> It's not completely redundant...
> 
>>>    	debugfs_remove_recursive(component->debugfs_root);
>>> +	component->debugfs_root = NULL;
>>>    }
> 
> ...with this, the two in combination add protection against a double
> free.  Not 100% sure it's worth it but I queued the patch since I
> couldn't strongly convince myself it's a bad idea.

I was only referring to the first test, which will be duplicated. see below.
The second part is just fine.

/**
  * debugfs_remove_recursive - recursively removes a directory
  * @dentry: a pointer to a the dentry of the directory to be removed. 
If this
  *          parameter is NULL or an error value, nothing will be done.
  *
  * This function recursively removes a directory tree in debugfs that
  * was previously created with a call to another debugfs function
  * (like debugfs_create_file() or variants thereof.)
  *
  * This function is required to be called in order for the file to be
  * removed, no automatic cleanup of files will happen when a module is
  * removed, you are responsible here.
  */
void debugfs_remove_recursive(struct dentry *dentry)
{
	struct dentry *child, *parent;

	if (IS_ERR_OR_NULL(dentry))
		return;
Mark Brown Aug. 6, 2019, 4:54 p.m. UTC | #4
On Tue, Aug 06, 2019 at 11:46:28AM -0500, Pierre-Louis Bossart wrote:
> On 8/6/19 11:22 AM, Mark Brown wrote:

> > ...with this, the two in combination add protection against a double
> > free.  Not 100% sure it's worth it but I queued the patch since I
> > couldn't strongly convince myself it's a bad idea.

> I was only referring to the first test, which will be duplicated. see below.
> The second part is just fine.

> {
> 	struct dentry *child, *parent;
> 
> 	if (IS_ERR_OR_NULL(dentry))
> 		return;

Oh, I see it's got a NULL test in it.  TBH I don't think it hurts to
check in the caller, it avoids someone having to check to make sure that
the NULL check is in the function.

Patch
diff mbox series

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 40bac40..00887f7 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -171,7 +171,10 @@  static void soc_init_component_debugfs(struct snd_soc_component *component)
 
 static void soc_cleanup_component_debugfs(struct snd_soc_component *component)
 {
+	if (!component->debugfs_root)
+		return;
 	debugfs_remove_recursive(component->debugfs_root);
+	component->debugfs_root = NULL;
 }
 
 static int dai_list_show(struct seq_file *m, void *v)