[PATCH 1/1] ath11k: fix return value check in ath11k_spectral_debug_register()

[Leizhen (ThunderTown)]

On 2024/10/24 9:30, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/10/23 22:02, Jeff Johnson wrote:
>> On 10/23/2024 12:40 AM, Zhen Lei wrote:
>>> Fix the incorrect return value check for debugfs_create_file(), which
>>> returns ERR_PTR(-ERROR) instead of NULL when it fails.
>>
>> Based upon the commit text this change is incorrect.
>>
>>  * 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.
>>
>> So ath11k should not be checking the return value at all, and definitely
>> should not be returning -EINVAL since the driver should still operate even if
>> creating a debugfs file fails.
> 
> OK, so that members such as scan_ctl in struct ath11k_spectral can be removed.

Sorry, the ath11k driver code is more complex than I think. I just went through
the code, and those members still need to be kept.

> I will update accordingly in v2.>
>>
>>>
>>> Fixes: 9d11b7bff950 ("ath11k: add support for spectral scan")
>>> Signed-off-by: Zhen Lei <thunder.leizhen at huawei.com>
>>> ---
>>>  drivers/net/wireless/ath/ath11k/spectral.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c
>>> index 79e091134515b43..4c545231292142a 100644
>>> --- a/drivers/net/wireless/ath/ath11k/spectral.c
>>> +++ b/drivers/net/wireless/ath/ath11k/spectral.c
>>> @@ -942,7 +942,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar)
>>>  						    0600,
>>>  						    ar->debug.debugfs_pdev, ar,
>>>  						    &fops_scan_ctl);
>>> -	if (!ar->spectral.scan_ctl) {
>>> +	if (IS_ERR(ar->spectral.scan_ctl)) {
>>>  		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
>>>  			    ar->pdev_idx);
>>>  		ret = -EINVAL;
>>> @@ -953,7 +953,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar)
>>>  						      0600,
>>>  						      ar->debug.debugfs_pdev, ar,
>>>  						      &fops_scan_count);
>>> -	if (!ar->spectral.scan_count) {
>>> +	if (IS_ERR(ar->spectral.scan_count)) {
>>>  		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
>>>  			    ar->pdev_idx);
>>>  		ret = -EINVAL;
>>> @@ -964,7 +964,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar)
>>>  						     0600,
>>>  						     ar->debug.debugfs_pdev, ar,
>>>  						     &fops_scan_bins);
>>> -	if (!ar->spectral.scan_bins) {
>>> +	if (IS_ERR(ar->spectral.scan_bins)) {
>>>  		ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n",
>>>  			    ar->pdev_idx);
>>>  		ret = -EINVAL;
>>
>>
>> .
>>
> 

-- 
Regards,
  Zhen Lei