[PATCH v2] ath11k: remove error checking for debugfs_create_file()

[PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Zhen Lei]

Driver ath11k can work fine even if the debugfs files fail to be created.
Therefore, the return value check of debugfs_create_file() should be
ignored, as it says.

Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/net/wireless/ath/ath11k/spectral.c | 24 ----------------------
 1 file changed, 24 deletions(-)

v1 --> v2:
Remove error checking for debugfs_create_file() instead of fixing it.

diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c
index 79e091134515b43..a6426eda023a944 100644
--- a/drivers/net/wireless/ath/ath11k/spectral.c
+++ b/drivers/net/wireless/ath/ath11k/spectral.c
@@ -925,8 +925,6 @@ void ath11k_spectral_deinit(struct ath11k_base *ab)
 
 static inline int ath11k_spectral_debug_register(struct ath11k *ar)
 {
-	int ret;
-
 	ar->spectral.rfs_scan = relay_open("spectral_scan",
 					   ar->debug.debugfs_pdev,
 					   ATH11K_SPECTRAL_SUB_BUFF_SIZE(ar->ab),
@@ -942,40 +940,18 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar)
 						    0600,
 						    ar->debug.debugfs_pdev, ar,
 						    &fops_scan_ctl);
-	if (!ar->spectral.scan_ctl) {
-		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
-			    ar->pdev_idx);
-		ret = -EINVAL;
-		goto debug_unregister;
-	}
 
 	ar->spectral.scan_count = debugfs_create_file("spectral_count",
 						      0600,
 						      ar->debug.debugfs_pdev, ar,
 						      &fops_scan_count);
-	if (!ar->spectral.scan_count) {
-		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
-			    ar->pdev_idx);
-		ret = -EINVAL;
-		goto debug_unregister;
-	}
 
 	ar->spectral.scan_bins = debugfs_create_file("spectral_bins",
 						     0600,
 						     ar->debug.debugfs_pdev, ar,
 						     &fops_scan_bins);
-	if (!ar->spectral.scan_bins) {
-		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
-			    ar->pdev_idx);
-		ret = -EINVAL;
-		goto debug_unregister;
-	}
 
 	return 0;
-
-debug_unregister:
-	ath11k_spectral_debug_unregister(ar);
-	return ret;
 }
 
 int ath11k_spectral_init(struct ath11k_base *ab)
-- 
2.34.1

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Kalle Valo]

Zhen Lei <thunder.leizhen@huawei.com> writes:

> Driver ath11k can work fine even if the debugfs files fail to be created.
> Therefore, the return value check of debugfs_create_file() should be
> ignored, as it says.
>
> Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Are you just guessing or did you confirm on a real device that ath11k
spectral really works without debugfs?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Jeff Johnson]

On 10/28/2024 7:02 AM, Kalle Valo wrote:
> Zhen Lei <thunder.leizhen@huawei.com> writes:
> 
>> Driver ath11k can work fine even if the debugfs files fail to be created.
>> Therefore, the return value check of debugfs_create_file() should be
>> ignored, as it says.
>>
>> Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Are you just guessing or did you confirm on a real device that ath11k
> spectral really works without debugfs?

The debugfs_create_file() documentation tells us:
 * NOTE: it's expected that most callers should _ignore_ the errors returned
 * by this function. Other debugfs functions handle the fact that the "dentry"
 * passed to them could be an error and they don't crash in that case.
 * Drivers should generally work fine even if debugfs fails to init anyway.

The caveat is that any driver functionality that relies upon debugfs obviously
won't work if the underlying file isn't created. Hence the language that the
driver "should generally work fine" since all functionality that isn't tied to
debugfs will still be available.

Since the relayfs functionality that spectral scan uses is dependent upon
debugfs, this functionality won't work if the debugfs operation fails. So the
question is, if that fails, do you continue running the driver that generally
works fine supporting all other wifi operations, or do you return an error,
and as part of the error handling, not init the driver and hence have no wifi
operation?

The one thing I didn't check is that although the documentation tells us that
debugfs functions handle an "error" dentry, I didn't check if relayfs handles it.

/jeff

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Leizhen (ThunderTown)]

On 2024/10/28 22:30, Jeff Johnson wrote:
> On 10/28/2024 7:02 AM, Kalle Valo wrote:
>> Zhen Lei <thunder.leizhen@huawei.com> writes:
>>
>>> Driver ath11k can work fine even if the debugfs files fail to be created.
>>> Therefore, the return value check of debugfs_create_file() should be
>>> ignored, as it says.
>>>
>>> Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> Are you just guessing or did you confirm on a real device that ath11k
>> spectral really works without debugfs?

Let's be honest, the only thing I know for sure is that debugfs_create_file()
returns an error code when it fails, not NULL. When I was locating a problem,
I found that a call to debugfs_create_file() did not process the return value
correctly. So I searched for other drivers, including ath11k. I'm not familiar
with ath11k. How to modify requires your help and decision.

> 
> The debugfs_create_file() documentation tells us:
>  * NOTE: it's expected that most callers should _ignore_ the errors returned
>  * by this function. Other debugfs functions handle the fact that the "dentry"
>  * passed to them could be an error and they don't crash in that case.
>  * Drivers should generally work fine even if debugfs fails to init anyway.
> 
> The caveat is that any driver functionality that relies upon debugfs obviously
> won't work if the underlying file isn't created. Hence the language that the
> driver "should generally work fine" since all functionality that isn't tied to
> debugfs will still be available.
> 
> Since the relayfs functionality that spectral scan uses is dependent upon
> debugfs, this functionality won't work if the debugfs operation fails. So the
> question is, if that fails, do you continue running the driver that generally
> works fine supporting all other wifi operations, or do you return an error,
> and as part of the error handling, not init the driver and hence have no wifi
> operation?

Maybe Kalle just pointed out that my description was incorrect.

Hi Kalle:
  Can you give me a clear instruction?
  1) Rollback to v1
     https://www.spinics.net/lists/linux-wireless/msg257219.html
  2) Update commit message, I think I can borrow the description above.

> 
> The one thing I didn't check is that although the documentation tells us that
> debugfs functions handle an "error" dentry, I didn't check if relayfs handles it.
> 
> /jeff
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Julian Calaby]

Hi Jeff,

On Tue, Oct 29, 2024 at 1:37 AM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 10/28/2024 7:02 AM, Kalle Valo wrote:
> > Zhen Lei <thunder.leizhen@huawei.com> writes:
> >
> >> Driver ath11k can work fine even if the debugfs files fail to be created.
> >> Therefore, the return value check of debugfs_create_file() should be
> >> ignored, as it says.
> >>
> >> Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >
> > Are you just guessing or did you confirm on a real device that ath11k
> > spectral really works without debugfs?
>
> The debugfs_create_file() documentation tells us:
>  * NOTE: it's expected that most callers should _ignore_ the errors returned
>  * by this function. Other debugfs functions handle the fact that the "dentry"
>  * passed to them could be an error and they don't crash in that case.
>  * Drivers should generally work fine even if debugfs fails to init anyway.
>
> The caveat is that any driver functionality that relies upon debugfs obviously
> won't work if the underlying file isn't created. Hence the language that the
> driver "should generally work fine" since all functionality that isn't tied to
> debugfs will still be available.

The big question for me is this:

> Since the relayfs functionality that spectral scan uses is dependent upon
> debugfs

Why?

This seems to go against the general guidance that debugfs should
essentially be "fire and forget" and return values shouldn't be
checked.

IMHO it comes down to one of two outcomes here:

1. Spectral scan isn't necessary for normal operation so we shouldn't
initialise that functionality if we can't use debugfs, which violates
the "don't check return values" guidance
2. We should break that dependency

I can envisage a lot of people making the sensible assumption that a
non-debug kernel doesn't need debugfs and therefore disabling it on
their thing, but also it seems to be enabled everywhere I have access
to: Debian Stable, WSL and Bazzite. So I could be completely missing
the point here.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Jeff Johnson]

On 10/28/2024 8:41 PM, Julian Calaby wrote:
> The big question for me is this:
> 
>> Since the relayfs functionality that spectral scan uses is dependent upon
>> debugfs
> 
> Why?
> 
> This seems to go against the general guidance that debugfs should
> essentially be "fire and forget" and return values shouldn't be
> checked.
> 
> IMHO it comes down to one of two outcomes here:
> 
> 1. Spectral scan isn't necessary for normal operation so we shouldn't
> initialise that functionality if we can't use debugfs, which violates
> the "don't check return values" guidance
> 2. We should break that dependency
> 
> I can envisage a lot of people making the sensible assumption that a
> non-debug kernel doesn't need debugfs and therefore disabling it on
> their thing, but also it seems to be enabled everywhere I have access
> to: Debian Stable, WSL and Bazzite. So I could be completely missing
> the point here.

https://docs.kernel.org/filesystems/relay.html

The relay interface needs a struct dentry * and debugfs provides that.
And as a result userspace expects the relay file to be there as well.

/jeff