Re: [PATCH] scsi: mpt3sas: Fix debugfs setup error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] scsi: mpt3sas: anti-pattern of checking debugfs return values
--

commit eaeb8348b8dd28fc3959554ee1eef029613c7292
Author: Lei Chen <lei.chen@smartx.com>
scsi: mpt3sas: Fix debugfs setup error handling

This commit modifies mpt3sas_setup_debugfs() to check for error pointers
using IS_ERR_OR_NULL() instead of just checking for NULL when creating
debugfs entries, and ensures the parent directory is cleared if file
creation fails.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
> index 183391f6b8d19..edaeccf538f40 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
[ ... ]
> @@ -120,11 +120,14 @@ mpt3sas_setup_debugfs(struct MPT3SAS_ADAPTER *ioc)
>  {
>  	char name[64];
>  
> +	if (IS_ERR_OR_NULL(mpt3sas_debugfs_root))
> +		return;
> +
>  	snprintf(name, sizeof(name), "scsi_host%d", ioc->shost->host_no);
>  	if (!ioc->debugfs_root) {
>  		ioc->debugfs_root =
>  		    debugfs_create_dir(name, mpt3sas_debugfs_root);
> -		if (!ioc->debugfs_root) {
> +		if (IS_ERR_OR_NULL(ioc->debugfs_root)) {

[Severity: Low]
Is it necessary to check the return value of debugfs_create_dir() in
mpt3sas_setup_debugfs() here? 

The kernel's established policy is that drivers should never check the
return values of debugfs creation functions. Since debugfs safely handles
its own failures and downstream calls accept error pointers seamlessly,
would it be better to remove these checks entirely instead of updating them?

>  			dev_err(&ioc->pdev->dev,
>  			    "Cannot create per adapter debugfs directory\n");
>  			return;
>  		}
>  	}
>  
>  	snprintf(name, sizeof(name), "ioc_dump");
>  	ioc->ioc_dump =	debugfs_create_file(name, 0444,
>  	    ioc->debugfs_root, ioc, &mpt3sas_debugfs_iocdump_fops);
> -	if (!ioc->ioc_dump) {
> +	if (IS_ERR_OR_NULL(ioc->ioc_dump)) {

[Severity: Low]
Does mpt3sas_setup_debugfs() need to manually handle the error and roll
back the parent directory when debugfs_create_file() fails?

Expanding this manual error path appears to conflict with the intended
fire-and-forget design of debugfs. The appropriate fix for flawed pointer
checks in debugfs setup is usually to remove them entirely, rather than
updating them to use IS_ERR_OR_NULL() and adding manual cleanup.

>  		dev_err(&ioc->pdev->dev,
>  		    "Cannot create ioc_dump debugfs file\n");
>  		debugfs_remove(ioc->debugfs_root);
> +		ioc->debugfs_root = NULL;
>  		return;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007141951.417155-1-lei.chen@smartx.com?part=1