Re: [PATCH] scsi: megaraid_sas: 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: megaraid_sas: debugfs API violation by checking return values and logging errors
--

commit 3a04787b7089e752e49721091a64cf2824d3811c
Author: Lei Chen <lei.chen@smartx.com>

scsi: megaraid_sas: Fix debugfs setup error handling

This commit updates the debugfs setup in megaraid_sas to handle error
pointers by adding IS_ERR_OR_NULL checks. It also ensures that the
per-adapter directory is cleaned up and its pointer cleared if the raidmap
file creation fails.

> diff --git a/drivers/scsi/megaraid/megaraid_sas_debugfs.c b/drivers/scsi/megaraid/megaraid_sas_debugfs.c
> index 81698cd83f5ae..556e01fc25e8e 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_debugfs.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_debugfs.c
> @@ -126,13 +126,16 @@ megasas_setup_debugfs(struct megasas_instance *instance)

[ ... ]

>  		if (!instance->debugfs_root) {
>  			instance->debugfs_root =
>  				debugfs_create_dir(name, megasas_debugfs_root);
> -			if (!instance->debugfs_root) {
> +			if (IS_ERR_OR_NULL(instance->debugfs_root)) {
>  				dev_err(&instance->pdev->dev,
>  					"Cannot create per adapter debugfs directory\n");

[Severity: Low]
Does this violate the debugfs API guidelines?

The debugfs guidelines recommend that drivers should not check the return
values of debugfs_create_*() functions.

If debugfs fails, such as due to memory constraints or runtime parameters,
these IS_ERR_OR_NULL() checks in megasas_setup_debugfs() will evaluate to
true and print a dev_err to the kernel log, creating spurious error logs
and adding unnecessary error-handling complexity.

>  				return;
> @@ -144,10 +147,11 @@ megasas_setup_debugfs(struct megasas_instance *instance)
>  			debugfs_create_file(name, S_IRUGO,
>  					    instance->debugfs_root, instance,
>  					    &megasas_debugfs_raidmap_fops);
> -		if (!instance->raidmap_dump) {
> +		if (IS_ERR_OR_NULL(instance->raidmap_dump)) {
>  			dev_err(&instance->pdev->dev,
>  				"Cannot create raidmap debugfs file\n");

[Severity: Low]
Could this be simplified by removing the error check entirely?

Similar to the directory creation above, checking the return value of
debugfs_create_file() here in megasas_setup_debugfs() is generally
discouraged. Removing these checks completely might align better with the
subsystem guidelines.

>  			debugfs_remove(instance->debugfs_root);
> +			instance->debugfs_root = NULL;
>  			return;
>  		}
>  	}

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