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

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

[Lei Chen]

debugfs_create_dir() and debugfs_create_file() return error pointers on
failure, but mpt3sas_setup_debugfs() only checks for NULL. As a result,
setup continues after directory creation fails, and a failure to create
ioc_dump leaves the per-adapter directory behind and still allows setup
of host_recovery to proceed.

Skip per-adapter setup if the driver debugfs root is unavailable, and
use IS_ERR_OR_NULL() to check directory and file creation results so
that the existing error paths also handle error pointers.

With the corrected error check, file creation failure removes the
per-adapter directory. Clear the stored directory pointer as well so
teardown does not attempt to remove the freed dentry again.

Fixes: 2b01b293f359 ("scsi: mpt3sas: Capture IOC data for debugging purposes")
Signed-off-by: Lei Chen <lei.chen@smartx.com>
---
 drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
index 183391f6b8d1..edaeccf538f4 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)) {
 			dev_err(&ioc->pdev->dev,
 			    "Cannot create per adapter debugfs directory\n");
 			return;
@@ -134,10 +137,11 @@ mpt3sas_setup_debugfs(struct MPT3SAS_ADAPTER *ioc)
 	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)) {
 		dev_err(&ioc->pdev->dev,
 		    "Cannot create ioc_dump debugfs file\n");
 		debugfs_remove(ioc->debugfs_root);
+		ioc->debugfs_root = NULL;
 		return;
 	}
 
-- 
2.43.0

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

[sashiko-bot]

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

[PATCH v2] scsi: mpt3sas: Simplify debugfs setup

[Lei Chen]

The debugfs creation helpers return error pointers on failure, so the
existing NULL checks do not handle creation failures. There is no need
to add error handling here: debugfs accepts error pointers as parents
and debugfs_remove_recursive() safely handles error pointers and NULL.

Remove the ineffective checks, error messages and file-creation rollback.
Allow host_recovery creation to proceed independently of ioc_dump, and
leave any successfully created entries for the existing teardown path
to remove.

The setup function is called only once per adapter from probe, so create
the per-adapter directory without checking whether it already exists.

Signed-off-by: Lei Chen <lei.chen@smartx.com>
---
Changes in v2:
- Remove debugfs error checks and logging instead of adding
  IS_ERR_OR_NULL() checks.
- Leave debugfs cleanup to the existing teardown path.
- Allow host_recovery creation to proceed independently of ioc_dump.
- Remove the redundant directory existence check, since setup is called
  only once per adapter from probe.
- Reframe the patch as a cleanup and drop the Fixes tag.

Link to v1:
https://lore.kernel.org/all/20261007141951.417155-1-lei.chen@smartx.com/

 drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
index 183391f6b8d1..0e37473abbf6 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
@@ -99,8 +99,6 @@ static const struct file_operations mpt3sas_debugfs_iocdump_fops = {
 void mpt3sas_init_debugfs(void)
 {
 	mpt3sas_debugfs_root = debugfs_create_dir("mpt3sas", NULL);
-	if (!mpt3sas_debugfs_root)
-		pr_info("mpt3sas: Cannot create debugfs root\n");
 }
 
 /*
@@ -121,25 +119,12 @@ mpt3sas_setup_debugfs(struct MPT3SAS_ADAPTER *ioc)
 	char name[64];
 
 	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) {
-			dev_err(&ioc->pdev->dev,
-			    "Cannot create per adapter debugfs directory\n");
-			return;
-		}
-	}
+	ioc->debugfs_root =
+		debugfs_create_dir(name, mpt3sas_debugfs_root);
 
 	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) {
-		dev_err(&ioc->pdev->dev,
-		    "Cannot create ioc_dump debugfs file\n");
-		debugfs_remove(ioc->debugfs_root);
-		return;
-	}
 
 	snprintf(name, sizeof(name), "host_recovery");
 	debugfs_create_u8(name, 0444, ioc->debugfs_root, &ioc->shost_recovery);
-- 
2.43.0