[alsa-devel] [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Cezary Rojewski]

On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 

This change heavily impacts user space and development kits used by us 
internally, and our clients. That is, if anything goes wrong during 
debugfs initialization process.

Currently, apps may safely assume entire debugfs tree is up and running 
once audio stack gets enumerated successfully. With your patch this is 
no longer the case and user space is forced to verify status of all 
debugfs files and/ or directories manually.

Most of this you knew already - I see Mark was very vocal about 
consequences and possible regression. Nonetheless we have had a short 
meeting with our coe-audio members regarding this change, specifically. 
Conclusion was simple: losing ability to test debugfs objects status 
during their creation e.g.: via IS_ERR family is considered harmful.

Czarek

> Cc: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Cc: Liam Girdwood <liam.r.girdwood at linux.intel.com>
> Cc: Jie Yang <yang.jie at linux.intel.com>
> Cc: Mark Brown <broonie at kernel.org>
> Cc: Jaroslav Kysela <perex at perex.cz>
> Cc: Takashi Iwai <tiwai at suse.com>
> Cc: alsa-devel at alsa-project.org
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> ---
>   sound/soc/intel/skylake/skl-debug.c | 28 +++++-----------------------
>   1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c
> index 5d7ac2ee7a3c..5481e3362414 100644
> --- a/sound/soc/intel/skylake/skl-debug.c
> +++ b/sound/soc/intel/skylake/skl-debug.c
> @@ -170,10 +170,8 @@ void skl_debug_init_module(struct skl_debug *d,
>   			struct snd_soc_dapm_widget *w,
>   			struct skl_module_cfg *mconfig)
>   {
> -	if (!debugfs_create_file(w->name, 0444,
> -				d->modules, mconfig,
> -				&mcfg_fops))
> -		dev_err(d->dev, "%s: module debugfs init failed\n", w->name);
> +	debugfs_create_file(w->name, 0444, d->modules, mconfig,
> +			    &mcfg_fops);
>   }
>   
>   static ssize_t fw_softreg_read(struct file *file, char __user *user_buf,
> @@ -230,32 +228,16 @@ struct skl_debug *skl_debugfs_init(struct skl *skl)
>   		return NULL;
>   
>   	/* create the debugfs dir with platform component's debugfs as parent */
> -	d->fs = debugfs_create_dir("dsp",
> -				   skl->component->debugfs_root);
> -	if (IS_ERR(d->fs) || !d->fs) {
> -		dev_err(&skl->pci->dev, "debugfs root creation failed\n");
> -		return NULL;
> -	}
> +	d->fs = debugfs_create_dir("dsp", skl->component->debugfs_root);
>   
>   	d->skl = skl;
>   	d->dev = &skl->pci->dev;
>   
>   	/* now create the module dir */
>   	d->modules = debugfs_create_dir("modules", d->fs);
> -	if (IS_ERR(d->modules) || !d->modules) {
> -		dev_err(&skl->pci->dev, "modules debugfs create failed\n");
> -		goto err;
> -	}
>   
> -	if (!debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
> -				 &soft_regs_ctrl_fops)) {
> -		dev_err(d->dev, "fw soft regs control debugfs init failed\n");
> -		goto err;
> -	}
> +	debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
> +			    &soft_regs_ctrl_fops);
>   
>   	return d;
> -
> -err:
> -	debugfs_remove_recursive(d->fs);
> -	return NULL;
>   }
>