linux-kernel - Re: [PATCH] dma: debug: no need to check return value of 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: <2b974ef1-4336-8187-cbc5-ba2a14691837@arm.com>
Date:   Tue, 22 Jan 2019 18:44:38 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Cc:     Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        iommu@...ts.linux-foundation.org,
        Corentin Labbe <clabbe@...libre.com>
Subject: Re: [PATCH] dma: debug: no need to check return value of
 debugfs_create functions

Hi Greg,

On 22/01/2019 15:21, 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.
> 
> Also delete the variables for the file dentries for the debugfs entries
> as they are never used at all once they are created.

Modulo one nit below, I certainly approve of the cleanup :)

Reviewed-by: Robin Murphy <robin.murphy@....com>

> Cc: Christoph Hellwig <hch@....de>
> Cc: Marek Szyprowski <m.szyprowski@...sung.com>
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: iommu@...ts.linux-foundation.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
>   kernel/dma/debug.c | 81 ++++++----------------------------------------
>   1 file changed, 10 insertions(+), 71 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 23cf5361bcf1..2f5fc8b9d39f 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -136,14 +136,6 @@ static u32 nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
>   
>   /* debugfs dentry's for the stuff above */
>   static struct dentry *dma_debug_dent        __read_mostly;

Does this actually need to be at file scope, or could it be punted to a 
local in dma_debug_fs_init() while we're here?

Robin.

> -static struct dentry *global_disable_dent   __read_mostly;
> -static struct dentry *error_count_dent      __read_mostly;
> -static struct dentry *show_all_errors_dent  __read_mostly;
> -static struct dentry *show_num_errors_dent  __read_mostly;
> -static struct dentry *num_free_entries_dent __read_mostly;
> -static struct dentry *min_free_entries_dent __read_mostly;
> -static struct dentry *nr_total_entries_dent __read_mostly;
> -static struct dentry *filter_dent           __read_mostly;
>   
>   /* per-driver filter related state */
>   
> @@ -840,66 +832,18 @@ static const struct file_operations filter_fops = {
>   	.llseek = default_llseek,
>   };
>   
> -static int dma_debug_fs_init(void)
> +static void dma_debug_fs_init(void)
>   {
>   	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
> -	if (!dma_debug_dent) {
> -		pr_err("can not create debugfs directory\n");
> -		return -ENOMEM;
> -	}
> -
> -	global_disable_dent = debugfs_create_bool("disabled", 0444,
> -			dma_debug_dent,
> -			&global_disable);
> -	if (!global_disable_dent)
> -		goto out_err;
> -
> -	error_count_dent = debugfs_create_u32("error_count", 0444,
> -			dma_debug_dent, &error_count);
> -	if (!error_count_dent)
> -		goto out_err;
> -
> -	show_all_errors_dent = debugfs_create_u32("all_errors", 0644,
> -			dma_debug_dent,
> -			&show_all_errors);
> -	if (!show_all_errors_dent)
> -		goto out_err;
> -
> -	show_num_errors_dent = debugfs_create_u32("num_errors", 0644,
> -			dma_debug_dent,
> -			&show_num_errors);
> -	if (!show_num_errors_dent)
> -		goto out_err;
> -
> -	num_free_entries_dent = debugfs_create_u32("num_free_entries", 0444,
> -			dma_debug_dent,
> -			&num_free_entries);
> -	if (!num_free_entries_dent)
> -		goto out_err;
> -
> -	min_free_entries_dent = debugfs_create_u32("min_free_entries", 0444,
> -			dma_debug_dent,
> -			&min_free_entries);
> -	if (!min_free_entries_dent)
> -		goto out_err;
> -
> -	nr_total_entries_dent = debugfs_create_u32("nr_total_entries", 0444,
> -			dma_debug_dent,
> -			&nr_total_entries);
> -	if (!nr_total_entries_dent)
> -		goto out_err;
> -
> -	filter_dent = debugfs_create_file("driver_filter", 0644,
> -					  dma_debug_dent, NULL, &filter_fops);
> -	if (!filter_dent)
> -		goto out_err;
> -
> -	return 0;
>   
> -out_err:
> -	debugfs_remove_recursive(dma_debug_dent);
> -
> -	return -ENOMEM;
> +	debugfs_create_bool("disabled", 0444, dma_debug_dent, &global_disable);
> +	debugfs_create_u32("error_count", 0444, dma_debug_dent, &error_count);
> +	debugfs_create_u32("all_errors", 0644, dma_debug_dent, &show_all_errors);
> +	debugfs_create_u32("num_errors", 0644, dma_debug_dent, &show_num_errors);
> +	debugfs_create_u32("num_free_entries", 0444, dma_debug_dent, &num_free_entries);
> +	debugfs_create_u32("min_free_entries", 0444, dma_debug_dent, &min_free_entries);
> +	debugfs_create_u32("nr_total_entries", 0444, dma_debug_dent, &nr_total_entries);
> +	debugfs_create_file("driver_filter", 0644, dma_debug_dent, NULL, &filter_fops);
>   }
>   
>   static int device_dma_allocations(struct device *dev, struct dma_debug_entry **out_entry)
> @@ -985,12 +929,7 @@ static int dma_debug_init(void)
>   		spin_lock_init(&dma_entry_hash[i].lock);
>   	}
>   
> -	if (dma_debug_fs_init() != 0) {
> -		pr_err("error creating debugfs entries - disabling\n");
> -		global_disable = true;
> -
> -		return 0;
> -	}
> +	dma_debug_fs_init();
>   
>   	nr_pages = DIV_ROUND_UP(nr_prealloc_entries, DMA_DEBUG_DYNAMIC_ENTRIES);
>   	for (i = 0; i < nr_pages; ++i)
>