Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <36e805fa-338d-a945-2621-75c5077572fc@intel.com>
Date:   Thu, 18 May 2023 11:15:31 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.0
Subject: Re: [PATCH] mmc: core: Fix error checking
Content-Language: en-US
To:     Yeqi Fu <asuk4.q@gmail.com>, ulf.hansson@linaro.org,
        CLoehle@hyperstone.com, avri.altman@wdc.com, axboe@kernel.dk
Cc:     linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ivan Orlov <ivan.orlov0322@gmail.com>
References: <20230517192654.367892-1-asuk4.q@gmail.com>
From:   Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <20230517192654.367892-1-asuk4.q@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/05/23 22:26, Yeqi Fu wrote:
> The functions debugfs_create_dir and debugfs_create_file_unsafe return
> ERR_PTR if an error occurs, and the appropriate way to verify for errors
> is to use the inline function IS_ERR. The patch will substitute the
> null-comparison with IS_ERR.
> 
> Suggested-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> Signed-off-by: Yeqi Fu <asuk4.q@gmail.com>
> ---
>  drivers/mmc/core/block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 00c33edb9fb9..507bebc22636 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2908,7 +2908,7 @@ static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data *md)
>  			debugfs_create_file_unsafe("status", 0400, root,
>  						   card,
>  						   &mmc_dbg_card_status_fops);
> -		if (!md->status_dentry)
> +		if (IS_ERR(md->status_dentry))
>  			return -EIO;
>  	}
>  
> @@ -2916,7 +2916,7 @@ static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data *md)
>  		md->ext_csd_dentry =
>  			debugfs_create_file("ext_csd", S_IRUSR, root, card,
>  					    &mmc_dbg_ext_csd_fops);
> -		if (!md->ext_csd_dentry)
> +		if (IS_ERR(md->ext_csd_dentry))
>  			return -EIO;
>  	}
>  

The patch is not wrong, but you also need to look at the bigger picture.
In this case, the return value is not used.  And debugfs API is designed
so that return values can be ignored - for example, it is ok to pass NULL
or an error code to debugfs_remove().  Generally we don't care if debugfs
fails, because it is only for debugging, but it only uses memory resources
so it essentially doesn't fail anyway - except when it is not compiled in.

So you could change mmc_blk_add_debugfs() to return void, and drop the error
checks entirely.

The error checks in mmc_blk_remove_debugfs() also serve no purpose.